script: Don't try to evaluate xpath variable references (servo#39395)

simonwuelker · web-flow · commit bdf630563c17 · 2025-09-20T12:36:54.000Z
Variables in xpath via the javascript bindings are a bit mysterious, as there is no way that a variable can be specified. We currently panic when encountering a variable, which is not good. Instead we now throw an error. We keep parsing the variables because the code is already there and it seems realistic that their behaviour will be specified in the future. I'm fine with removing them too if that is preferred. Testing: This behaviour is unspecified and different browser produce different results. There is no "correct" way to do this, but we should not crash Part of: servo#34527 --------- Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
diff --git a/components/script/xpath/eval.rs b/components/script/xpath/eval.rs
@@ -33,9 +33,12 @@ pub(crate) enum Error {
     UnknownFunction {
         name: QualName,
     },
-    UnknownVariable {
-        name: QualName,
-    },
+    /// It is not clear where variables used in XPath expression should come from.
+    /// Firefox throws "NS_ERROR_ILLEGAL_VALUE" when using them, chrome seems to return
+    /// an empty result. We also error out.
+    ///
+    /// See <https://github.com/whatwg/dom/issues/67>
+    CannotUseVariables,
     UnknownNamespace {
         prefix: String,
     },
@@ -58,7 +61,7 @@ impl std::fmt::Display for Error {
             Error::NotANodeset => write!(f, "expression did not evaluate to a nodeset"),
             Error::InvalidPath => write!(f, "invalid path expression"),
             Error::UnknownFunction { name } => write!(f, "unknown function {:?}", name),
-            Error::UnknownVariable { name } => write!(f, "unknown variable {:?}", name),
+            Error::CannotUseVariables => write!(f, "cannot use variables"),
             Error::UnknownNamespace { prefix } => {
                 write!(f, "unknown namespace prefix {:?}", prefix)
             },
@@ -745,7 +748,7 @@ impl Evaluatable for PrimaryExpr {
     fn evaluate(&self, context: &EvaluationCtx) -> Result<Value, Error> {
         match self {
             PrimaryExpr::Literal(literal) => literal.evaluate(context),
-            PrimaryExpr::Variable(_qname) => todo!(),
+            PrimaryExpr::Variable(_qname) => Err(Error::CannotUseVariables),
             PrimaryExpr::Parenthesized(expr) => expr.evaluate(context),
             PrimaryExpr::ContextItem => Ok(Value::Nodeset(vec![context.context_node.clone()])),
             PrimaryExpr::Function(core_function) => core_function.evaluate(context),
diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json
@@ -7036,6 +7036,13 @@
     ]
    },
    "domxpath": {
+    "variables-in-expression-crash.html": [
+     "d1f15a6abe0d9c8ecae08b4cdf5dd9ef3efbbbcc",
+     [
+      null,
+      {}
+     ]
+    ],
     "xpath-evaluate-crash.html": [
      "5303d85ad31ca0ee230ac36231898342b07ad2c3",
      [
diff --git a/tests/wpt/tests/domxpath/variables-in-expression-crash.html b/tests/wpt/tests/domxpath/variables-in-expression-crash.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+
+<head>
+    <link rel="help" href="https://github.com/servo/servo/issues/36971">
+    <meta name="assert" content="Using variables in xpath expression should not crash.">
+</head>
+<body>
+    <script>
+      // The exact behaviour here is not defined. Firefox throws an error, Chrome doesn't.
+      document.evaluate("$foo", document.createElement("div"));
+    </script>
+</body>

Original file line number	Diff line number	Diff line change
`@@ -7036,6 +7036,13 @@`
`7036`	`7036`	`]`
`7037`	`7037`	`},`
`7038`	`7038`	`"domxpath": {`
	`7039`	`+ "variables-in-expression-crash.html": [`
	`7040`	`+ "d1f15a6abe0d9c8ecae08b4cdf5dd9ef3efbbbcc",`
	`7041`	`+ [`
	`7042`	`+ null,`
	`7043`	`+ {}`
	`7044`	`+ ]`
	`7045`	`+ ],`
`7039`	`7046`	`"xpath-evaluate-crash.html": [`
`7040`	`7047`	`"5303d85ad31ca0ee230ac36231898342b07ad2c3",`
`7041`	`7048`	`[`