Bug 1002280 - ID starting with numeric value crashes the devtools DOM inspector;r=jwalker,bholley

browser/devtools/commandline/test/browser.ini

@@ -80,4 +80,3 @@ skip-if = true || e10s # Disabled until TZ bug is fixed
 [browser_gcli_tokenize.js]
 [browser_gcli_tooltip.js]
 [browser_gcli_types.js]
-[browser_gcli_util.js]

browser/devtools/commandline/test/browser_gcli_util.js

@@ -1,60 +0,0 @@
-/*
- * Copyright 2012, Mozilla Foundation and contributors
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-'use strict';
-// <INJECTED SOURCE:START>
-
-// THIS FILE IS GENERATED FROM SOURCE IN THE GCLI PROJECT
-// DO NOT EDIT IT DIRECTLY
-
-var exports = {};
-
-var TEST_URI = "data:text/html;charset=utf-8,<p id='gcli-input'>gcli-testUtil.js</p>";
-
-function test() {
-  return Task.spawn(function() {
-    let options = yield helpers.openTab(TEST_URI);
-    yield helpers.openToolbar(options);
-    gcli.addItems(mockCommands.items);
-
-    yield helpers.runTests(options, exports);
-
-    gcli.removeItems(mockCommands.items);
-    yield helpers.closeToolbar(options);
-    yield helpers.closeTab(options);
-  }).then(finish, helpers.handleError);
-}
-
-// <INJECTED SOURCE:END>
-
-// var assert = require('../testharness/assert');
-var util = require('gcli/util/util');
-
-exports.testFindCssSelector = function(options) {
-  if (options.isPhantomjs || options.isNoDom) {
-    assert.log('Skipping tests due to issues with querySelectorAll.');
-    return;
-  }
-
-  var nodes = options.window.document.querySelectorAll('*');
-  for (var i = 0; i < nodes.length; i++) {
-    var selector = util.findCssSelector(nodes[i]);
-    var matches = options.window.document.querySelectorAll(selector);
-
-    assert.is(matches.length, 1, 'multiple matches for ' + selector);
-    assert.is(matches[0], nodes[i], 'non-matching selector: ' + selector);
-  }
-};

browser/devtools/inspector/test/browser_inspector_reload.js

@@ -21,7 +21,10 @@ function test() {
     }, content);
   }, true);
 
-  content.location = "data:text/html,<p>p</p>";
+  // Reload should reselect the currently selected markup view element.
+  // This should work even when an element whose selector needs escaping
+  // is selected (bug 100228).
+  content.location = "data:text/html,<p id='1'>p</p>";
 
   function startInspectorTests(aToolbox)
   {

js/xpconnect/src/Sandbox.cpp

@@ -30,6 +30,7 @@
 #include "XPCWrapper.h"
 #include "XrayWrapper.h"
 #include "mozilla/dom/BindingUtils.h"
+#include "mozilla/dom/CSSBinding.h"
 #include "mozilla/dom/indexedDB/IndexedDatabaseManager.h"
 #include "mozilla/dom/PromiseBinding.h"
 #include "mozilla/dom/TextDecoderBinding.h"
@@ -978,6 +979,8 @@ xpc::GlobalProperties::Parse(JSContext *cx, JS::HandleObject obj)
         NS_ENSURE_TRUE(name, false);
         if (promise && !strcmp(name.ptr(), "-Promise")) {
             Promise = false;
+        } else if (!strcmp(name.ptr(), "CSS")) {
+            CSS = true;
         } else if (!strcmp(name.ptr(), "indexedDB")) {
             indexedDB = true;
         } else if (!strcmp(name.ptr(), "XMLHttpRequest")) {
@@ -1003,6 +1006,9 @@ xpc::GlobalProperties::Parse(JSContext *cx, JS::HandleObject obj)
 bool
 xpc::GlobalProperties::Define(JSContext *cx, JS::HandleObject obj)
 {
+    if (CSS && !dom::CSSBinding::GetConstructorObject(cx, obj))
+        return false;
+
     if (Promise && !dom::PromiseBinding::GetConstructorObject(cx, obj))
         return false;
 

js/xpconnect/src/xpcprivate.h

@@ -3295,6 +3295,7 @@ struct GlobalProperties {
     }
     bool Parse(JSContext *cx, JS::HandleObject obj);
     bool Define(JSContext *cx, JS::HandleObject obj);
+    bool CSS : 1;
     bool Promise : 1;
     bool indexedDB : 1;
     bool XMLHttpRequest : 1;

js/xpconnect/tests/unit/test_css.js

@@ -0,0 +1,10 @@
+function run_test() {
+  var Cu = Components.utils;
+  var sb = new Cu.Sandbox('http://www.example.com',
+                          { wantGlobalProperties: ["CSS"] });
+  sb.do_check_eq = do_check_eq;
+  Cu.evalInSandbox('do_check_eq(CSS.escape("$"), "\\\\$");',
+                   sb);
+  Cu.importGlobalProperties(["CSS"]);
+  do_check_eq(CSS.escape("$"), "\\$");
+}

js/xpconnect/tests/unit/xpcshell.ini

@@ -74,6 +74,7 @@ fail-if = os == "android"
 [test_promise.js]
 [test_textDecoder.js]
 [test_url.js]
+[test_css.js]
 [test_sandbox_atob.js]
 [test_isProxy.js]
 [test_getObjectPrincipal.js]

toolkit/devtools/gcli/source/lib/gcli/util/util.js

@@ -547,19 +547,6 @@ exports.isXmlDocument = function(doc) {
   return false;
 };
 
-/**
- * Find the position of [element] in [nodeList].
- * @returns an index of the match, or -1 if there is no match
- */
-function positionInNodeList(element, nodeList) {
-  for (var i = 0; i < nodeList.length; i++) {
-    if (element === nodeList[i]) {
-      return i;
-    }
-  }
-  return -1;
-}
-
 /**
  * We'd really like to be able to do 'new NodeList()'
  */
@@ -570,68 +557,6 @@ exports.createEmptyNodeList = function(doc) {
   return doc.querySelectorAll('x>:root');
 };
 
-/**
- * Find a unique CSS selector for a given element
- * @returns a string such that ele.ownerDocument.querySelector(reply) === ele
- * and ele.ownerDocument.querySelectorAll(reply).length === 1
- */
-exports.findCssSelector = function(ele) {
-  var document = ele.ownerDocument;
-  if (ele.id && document.getElementById(ele.id) === ele) {
-    return '#' + ele.id;
-  }
-
-  // Inherently unique by tag name
-  var tagName = ele.tagName.toLowerCase();
-  if (tagName === 'html') {
-    return 'html';
-  }
-  if (tagName === 'head') {
-    return 'head';
-  }
-  if (tagName === 'body') {
-    return 'body';
-  }
-
-  if (ele.parentNode == null) {
-    console.log('danger: ' + tagName);
-  }
-
-  // We might be able to find a unique class name
-  var selector, index, matches;
-  if (ele.classList.length > 0) {
-    for (var i = 0; i < ele.classList.length; i++) {
-      // Is this className unique by itself?
-      selector = '.' + ele.classList.item(i);
-      matches = document.querySelectorAll(selector);
-      if (matches.length === 1) {
-        return selector;
-      }
-      // Maybe it's unique with a tag name?
-      selector = tagName + selector;
-      matches = document.querySelectorAll(selector);
-      if (matches.length === 1) {
-        return selector;
-      }
-      // Maybe it's unique using a tag name and nth-child
-      index = positionInNodeList(ele, ele.parentNode.children) + 1;
-      selector = selector + ':nth-child(' + index + ')';
-      matches = document.querySelectorAll(selector);
-      if (matches.length === 1) {
-        return selector;
-      }
-    }
-  }
-
-  // So we can be unique w.r.t. our parent, and use recursion
-  index = positionInNodeList(ele, ele.parentNode.children) + 1;
-  selector = exports.findCssSelector(ele.parentNode) + ' > ' +
-          tagName + ':nth-child(' + index + ')';
-
-  return selector;
-};
-
-
 //------------------------------------------------------------------------------
 
 /**

toolkit/devtools/server/actors/script.js

@@ -15,6 +15,7 @@ const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
 const { dbg_assert, dumpn, update } = DevToolsUtils;
 const { SourceMapConsumer, SourceMapGenerator } = require("source-map");
 const { defer, resolve, reject, all } = require("devtools/toolkit/deprecated-sync-thenables");
+const {CssLogic} = require("devtools/styleinspector/css-logic");
 
 Cu.import("resource://gre/modules/NetUtil.jsm");
 
@@ -1803,7 +1804,7 @@ ThreadActor.prototype = {
         }
 
         // There will be no tagName if the event listener is set on the window.
-        let selector = node.tagName ? findCssSelector(node) : "window";
+        let selector = node.tagName ? CssLogic.findCssSelector(node) : "window";
         let nodeDO = this.globalDebugObject.makeDebuggeeValue(node);
         listenerForm.node = {
           selector: selector,
@@ -5506,83 +5507,6 @@ function reportError(aError, aPrefix="") {
   dumpn(msg);
 }
 
-// The following are copied here verbatim from css-logic.js, until we create a
-// server-friendly helper module.
-
-/**
- * Find a unique CSS selector for a given element
- * @returns a string such that ele.ownerDocument.querySelector(reply) === ele
- * and ele.ownerDocument.querySelectorAll(reply).length === 1
- */
-function findCssSelector(ele) {
-  var document = ele.ownerDocument;
-  if (ele.id && document.getElementById(ele.id) === ele) {
-    return '#' + ele.id;
-  }
-
-  // Inherently unique by tag name
-  var tagName = ele.tagName.toLowerCase();
-  if (tagName === 'html') {
-    return 'html';
-  }
-  if (tagName === 'head') {
-    return 'head';
-  }
-  if (tagName === 'body') {
-    return 'body';
-  }
-
-  if (ele.parentNode == null) {
-    console.log('danger: ' + tagName);
-  }
-
-  // We might be able to find a unique class name
-  var selector, index, matches;
-  if (ele.classList.length > 0) {
-    for (var i = 0; i < ele.classList.length; i++) {
-      // Is this className unique by itself?
-      selector = '.' + ele.classList.item(i);
-      matches = document.querySelectorAll(selector);
-      if (matches.length === 1) {
-        return selector;
-      }
-      // Maybe it's unique with a tag name?
-      selector = tagName + selector;
-      matches = document.querySelectorAll(selector);
-      if (matches.length === 1) {
-        return selector;
-      }
-      // Maybe it's unique using a tag name and nth-child
-      index = positionInNodeList(ele, ele.parentNode.children) + 1;
-      selector = selector + ':nth-child(' + index + ')';
-      matches = document.querySelectorAll(selector);
-      if (matches.length === 1) {
-        return selector;
-      }
-    }
-  }
-
-  // So we can be unique w.r.t. our parent, and use recursion
-  index = positionInNodeList(ele, ele.parentNode.children) + 1;
-  selector = findCssSelector(ele.parentNode) + ' > ' +
-          tagName + ':nth-child(' + index + ')';
-
-  return selector;
-};
-
-/**
- * Find the position of [element] in [nodeList].
- * @returns an index of the match, or -1 if there is no match
- */
-function positionInNodeList(element, nodeList) {
-  for (var i = 0; i < nodeList.length; i++) {
-    if (element === nodeList[i]) {
-      return i;
-    }
-  }
-  return -1;
-}
-
 /**
  * Make a debuggee value for the given object, if needed. Primitive values
  * are left the same.

toolkit/devtools/server/tests/mochitest/chrome.ini

@@ -17,6 +17,7 @@ support-files =
 [test_Debugger.Source.prototype.element.html]
 [test_Debugger.Script.prototype.global.html]
 [test_connection-manager.html]
+[test_css-logic.html]
 [test_device.html]
 [test_inspector-changeattrs.html]
 [test_inspector-changevalue.html]

toolkit/devtools/server/tests/mochitest/test_css-logic.html

@@ -0,0 +1,145 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug </title>
+
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+  <script type="application/javascript;version=1.8" src="inspector-helpers.js"></script>
+  <script type="application/javascript;version=1.8">
+Cu.import("resource://gre/modules/devtools/Loader.jsm");
+const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
+
+const {CssLogic} = devtools.require("devtools/styleinspector/css-logic");
+Cu.importGlobalProperties(['CSS']);
+
+window.onload = function() {
+  SimpleTest.waitForExplicitFinish();
+  runNextTest();
+}
+
+addTest(function findAllCssSelectors() {
+  var nodes = document.querySelectorAll('*');
+  for (var i = 0; i < nodes.length; i++) {
+    var selector = CssLogic.findCssSelector(nodes[i]);
+    var matches = document.querySelectorAll(selector);
+
+    is(matches.length, 1, 'There is a single match: ' + selector);
+    is(matches[0], nodes[i], 'The selector matches the correct node: ' + selector);
+  }
+
+  runNextTest();
+});
+
+addTest(function findCssSelectorNotContainedInDocument() {
+
+  var unattached = document.createElement("div");
+  unattached.id = "unattached";
+  try {
+    CssLogic.findCssSelector(unattached);
+    ok (false, "Unattached node did not throw")
+  } catch(e) {
+    ok(e, "Unattached node throws an exception");
+  }
+
+  var unattachedChild = document.createElement("div");
+  unattached.appendChild(unattachedChild);
+  try {
+    CssLogic.findCssSelector(unattachedChild);
+    ok (false, "Unattached child node did not throw")
+  } catch(e) {
+    ok(e, "Unattached child node throws an exception");
+  }
+
+  var unattachedBody = document.createElement("body");
+  try {
+    CssLogic.findCssSelector(unattachedBody);
+    ok (false, "Unattached body node did not throw")
+  } catch(e) {
+    ok(e, "Unattached body node throws an exception");
+  }
+
+  runNextTest();
+});
+
+addTest(function findCssSelector() {
+
+  let data = [
+    "#one",
+    "#" + CSS.escape("2"),
+    ".three",
+    "." + CSS.escape("4"),
+    "#find-css-selector > div:nth-child(5)",
+    "#find-css-selector > p:nth-child(6)",
+    ".seven",
+    ".eight",
+    ".nine",
+    ".ten",
+    "div.sameclass:nth-child(11)",
+    "div.sameclass:nth-child(12)",
+    "div.sameclass:nth-child(13)",
+    "#" + CSS.escape("!, \", #, $, %, &, ', (, ), *, +, ,, -, ., /, :, ;, <, =, >, ?, @, [, \\, ], ^, `, {, |, }, ~"),
+  ];
+
+  let container = document.querySelector("#find-css-selector");
+  is (container.children.length, data.length, "Container has correct number of children.");
+
+  for (let i = 0; i < data.length; i++) {
+    let node = container.children[i];
+    is (CssLogic.findCssSelector(node), data[i], "matched id for index " + (i-1));
+  }
+
+  runNextTest();
+});
+
+addTest(function getBackgroundImageUriFromProperty() {
+
+  let data = [
+    ["background: url(foo.png);", "foo.png"],
+    ["background: url(\"foo.png\") ", "foo.png"],
+    ["background: url('foo.png') ; ", "foo.png"],
+    ["background: foo.png", null],
+    ["background: url()", ""],
+  ];
+
+  for (let i = 0; i < data.length; i++) {
+    let prop = data[i][0];
+    let result = data[i][1];
+    is (CssLogic.getBackgroundImageUriFromProperty(prop), result,
+      "Background image matches for index " + i);
+  }
+
+  runNextTest();
+});
+
+  </script>
+</head>
+<body>
+  <div id="find-css-selector">
+    <div id="one"></div> <!-- Basic ID -->
+    <div id="2"></div> <!-- Escaped ID -->
+    <div class="three"></div> <!-- Basic Class -->
+    <div class="4"></div> <!-- Escaped Class -->
+    <div attr="5"></div>  <!-- Only an attribute -->
+    <p></p> <!-- Nothing unique -->
+    <div class="seven seven"></div> <!-- Two classes with same name -->
+    <div class="eight eight2"></div> <!-- Two classes with different names -->
+
+    <!-- Two elements with the same id - should not use ID -->
+    <div class="nine" id="nine-and-ten"></div>
+    <div class="ten" id="nine-and-ten"></div>
+
+    <!-- Three elements with the same id - should use class and nth-child instead -->
+    <div class="sameclass" id="11-12-13"></div>
+    <div class="sameclass" id="11-12-13"></div>
+    <div class="sameclass" id="11-12-13"></div>
+
+    <!-- Special characters -->
+    <div id="!, &quot;, #, $, %, &amp;, ', (, ), *, +, ,, -, ., /, :, ;, <, =, >, ?, @, [, \, ], ^, `, {, |, }, ~"></div>
+  </div>
+</body>
+</html>

toolkit/devtools/styleinspector/css-logic.js

@@ -50,6 +50,7 @@ const RX_PSEUDO = /\s*:?:([\w-]+)(\(?\)?)\s*/g;
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.importGlobalProperties(['CSS']);
 
 function CssLogic()
 {
@@ -865,12 +866,17 @@ function positionInNodeList(element, nodeList) {
  */
 CssLogic.findCssSelector = function CssLogic_findCssSelector(ele) {
   var document = ele.ownerDocument;
-  if (ele.id && document.getElementById(ele.id) === ele) {
-    return '#' + ele.id;
+  if (!document.contains(ele)) {
+    throw new Error('findCssSelector received element not inside document');
+  }
+
+  // document.querySelectorAll("#id") returns multiple if elements share an ID
+  if (ele.id && document.querySelectorAll('#' + CSS.escape(ele.id)).length === 1) {
+    return '#' + CSS.escape(ele.id);
   }
 
   // Inherently unique by tag name
-  var tagName = ele.tagName.toLowerCase();
+  var tagName = ele.localName;
   if (tagName === 'html') {
     return 'html';
   }
@@ -881,16 +887,12 @@ CssLogic.findCssSelector = function CssLogic_findCssSelector(ele) {
     return 'body';
   }
 
-  if (ele.parentNode == null) {
-    console.log('danger: ' + tagName);
-  }
-
   // We might be able to find a unique class name
   var selector, index, matches;
   if (ele.classList.length > 0) {
     for (var i = 0; i < ele.classList.length; i++) {
       // Is this className unique by itself?
-      selector = '.' + ele.classList.item(i);
+      selector = '.' + CSS.escape(ele.classList.item(i));
       matches = document.querySelectorAll(selector);
       if (matches.length === 1) {
         return selector;