Closed Bug 1002280 Opened 10 years ago Closed 10 years ago

ID starting with numeric value crashes the devtools DOM inspector

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Version:
29 Branch
Type:
defect

Tracking

(firefox31 affected, firefox32 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 32
Tracking Flags:
Tracking Status
firefox31 --- affected
firefox32 --- verified

People

(Reporter: geo.demoulin, Assigned: bgrins)

References

Details

Attachments

(2 files, 6 obsolete files)

css-logic-escape.patch
24.17 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
test.html
86 bytes, text/html
Details 
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140421221237

Steps to reproduce:

1./ Create a simple HTML file (test.html) with the following content:
<!DOCTYPE HTML>
<html>
<head>
</head>
<body>
<div id="1"></div>
</body>
</html>

2./ Open test.html in FF then press F12 (opens the dev tools)
3./ Expand the DOM tree and select the div with id="1"
4./ Press F5 to refresh the page


Actual results:

After selection of the element starting with a numeric value and refreshing the page, the inspector window goes blank (the DOM tree is hidden).

The browser console displays:

DOMException {code: 12, message: "An invalid or illegal string was specified", result: 2152923148, name: "SyntaxError", filename: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js", lineNumber: 1398, columnNumber: 0, inner: null, data: null, location: XPCWrappedNative_NoHelper} protocol.js:817
10:48:47.058 "unknownError" protocol.js:20


Expected results:

The DOM Tree should still be there. Non-HTML 5 ids shouldn't start with a number but HTML 5 is more permissive and this id should be allowed. In any case it shouldn't crash.
Severity: normal → critical
Component: Untriaged → Developer Tools: Inspector
OS: Windows 8 → All
Priority: -- → P1
Hardware: x86_64 → All
Flags: needinfo?(nobody)
Reproducible on firefox 29 release.
I did try:
- safe mode
- different computer
- new, fresh installation after profiles cleanup (removal)
Discussion at https://support.mozilla.org/en-US/questions/998743?esab=a&s=&r=0&as=s

cor-el digged a bit more and came up with what is generating the exception (https://support.mozilla.org/en-US/questions/998743#answer-568916):

The part that is crashing is document.querySelector("#1");

Found bug 718326 describing this problem. 
Basically, querySelector is using CSS selector syntax that doesn't support IDs starting with a numeric value unless escaped like this: document.querySelector('#\\31')

Does this means the method used in the DOM inspector to select an element have to be changed or as cor-el suggests, just add exception handling and ignore the problem?
For info only, document.querySelector("[id='1']") also works properly.
I'll quote cor-el post for detailed information:

There is an exception thrown, so the execution is aborted and only a try and catch could prevent this issue.

In a new tab paste the following (html document, base64-encoded):
data:text/html;charset=utf-8;base64,PCFET0NUWVBFIEhUTUw+CjxodG1sPgo8aGVhZD4KPC9oZWFkPgo8Ym9keT4KPGRpdiBpZD0iMSI+PC9kaXY+CjwvYm9keT4KPC9odG1sPgo=

Paste in the console:

document.querySelector("#1");

try {document.querySelector("#1");} catch(e){console.log(e)}

DOMException [SyntaxError: "An invalid or illegal string was specified"
code: 12
nsresult: 0x8053000c
location: debugger eval code:2]

    line 1311: resource://gre/modules/devtools/server/actors/inspector.js 

  querySelector: method(function(baseNode, selector) {
    if (!baseNode) {
      return {}
    };
    let node = baseNode.rawNode.querySelector(selector);

    if (!node) {
      return {}
    };

Is this enough info?
Confirmed FF 29, Win 7 x64.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Geoffrey, thanks for digging into this!

Yep, passing arbitrary strings you don't control to querySelector is not going to work.

Luckily, CSS.escape() exists precisely to handle this need.  Unfortunately, I can't find where devtools set up this "#1" string...
Flags: needinfo?(nobody)
Summary: ID starting with numeric value crashes the DOM inspector → ID starting with numeric value crashes the devtools DOM inspector
I think the buggy code here is toolkit/devtools/server/actors/script.js and in particular the findCssSelector function.  It needs to CSS.escape() the IDs and classes it uses.
Or maybe it's the CssLogic_findCssSelector over in toolkit/devtools/styleinspector/css-logic.js.  Why do we have multiple functions for doing this?

Or perhaps findCssSelector in toolkit/devtools/gcli/source/lib/gcli/util/util.js.
Alright, so one problem is that in whatever scope css-logic.js runs in there is no "CSS" on the global.  Bobby, can we just expose that across the board?
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] from comment #9)
> Alright, so one problem is that in whatever scope css-logic.js runs in there
> is no "CSS" on the global.  Bobby, can we just expose that across the board?

It should use Cu.importGlobalProperties(['CSS']).
Flags: needinfo?(bobbyholley)
Attached patch foo.patch (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 8418978 [details] [diff] [review]
foo.patch

This fixes things, but the xpconnect bits need improvement.

It also fixes what is imo a preexisting bug with SVG elements that don't have all-lowercase localNames.

This code seems to date back to bug 701419.  Joe, do you know who can pick this up and in particular add tests to exercise this stuff?

        Attachment #8418978 -
        Attachment filename: foo.patch → Hack fix
Flags: needinfo?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        With importGlobalProperties (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8418978 -
        Attachment is obsolete: true

    
    

    
  


  
I don't see why we can't use CssLogic in script.js.  The comment indicates that it is 'copied verbatim from css-logic.js, until we create a server-friendly helper module'.  Looks like this was done in Bug 897275.

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8418978 [details] [diff] [review]
> foo.patch
> 
> This fixes things, but the xpconnect bits need improvement.
> 
> It also fixes what is imo a preexisting bug with SVG elements that don't
> have all-lowercase localNames.
> 
> This code seems to date back to bug 701419.  Joe, do you know who can pick
> this up and in particular add tests to exercise this stuff?

I will add tests for this
Flags: needinfo?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        css-logic-escape.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Removes duplicate findCssSelector logic from script.js and adds a new mochitest-chrome test for testing css-logic functionality.  Right now it is only covering some common cases of findCssSelector, I'm still working to expand the coverage.

        Attachment #8419028 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8419078 [details] [diff] [review]
css-logic-escape.patch

Review of attachment 8419078 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/gcli/source/lib/gcli/util/util.js
@@ +16,5 @@
>  
>  'use strict';
>  
> +const { Cc, Ci, Cu } = require("chrome");
> +Cu.importGlobalProperties(['CSS'])

So everything inside toolkit/devtools/gcli/source is written to purely web APIs because it was originally used in Bespin, and is now used in Orion.

The best escape hatch for this is probably gcli/util/host which has platform specific code. So you could create a host.cssEscape, or you could just create a follow-up bug and assign it to me.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        css-logic-escape.patch (obsolete)
        — 
          — Splinter Review
      

    


  
OK, found 2 more bugs here when writing tests:

1) If there are multiple elements with the same ID on the page, findCssSelector returns "#id" for the first one (since it matches getElementById("id")).  In this case, it breaks the contract we are making, which is that calling querySelectorAll(findCssSelector(node)) will always return a single node.  In this case, querySelectorAll("#id") returns two elements in the NodeList.
2) If an element without a parentNode gets passed in, we log out a warning but proceed anyway and hit an error later in the function.  If this element is not attached to document but does have a parent node, then we happily return a selector that will of course not work.  We should return (or throw) right away in this case.  I've got it checking document.contains and returning an empty string first thing now.

        Attachment #8419078 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8419393 [details] [diff] [review]
css-logic-escape.patch

> The best escape hatch for this is probably gcli/util/host which has platform
> specific code. So you could create a host.cssEscape, or you could just
> create a follow-up bug and assign it to me.

Joe, I've made this change, but I'm guessing it isn't working yet as I haven't found a way to test this functionality in gcli.  In fact, when I search the folder the only reference to the function is the function definition and in a test file.  Is this still in use within gcli or can it be removed entirely?

        Attachment #8419393 -
        Flags: feedback?(jwalker)

    
  
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

    
    

    
  
> is written to purely web APIs

CSS.escape is a purely web API.  Is this code running in a Window or in a sandbox?  If the former, then it doesn't need the import stuff; I assumed it was running in a sandbox, like css-logic.js.
Flags: needinfo?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        css-logic-escape.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I talked with Joe, and we decided to remove the findCssSelector implementation in gcli since it was no longer in use.  So now there is one implementation, and it has been patched to resolve 4 issues:

1) The original reported bug which breaks the inspector when reloading a page with a unique selector that needs to be escaped
2) Handling non-lowercase tag names as bz pointed out in the original patch
3) Handling multiple elements with the same id correctly
4) Return when trying to find a selector for an element not contained within the document 

Joe, can you review the code in toolkit and browser?

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=af2429033eec

        Attachment #8419393 -
        Attachment is obsolete: true

        Attachment #8419393 -
        Flags: feedback?(jwalker)

        Attachment #8419621 -
        Flags: review?(jwalker)
Flags: needinfo?(jwalker)

    
    

    
  
Should we move the xpconnect changes into a separate patch or keep them here?
Flags: needinfo?(bzbarsky)

    
    

    
  
Probably fine either way; just ask bholley to review them!
Flags: needinfo?(bzbarsky)

    
  

        Attachment #8419001 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8419621 [details] [diff] [review]
css-logic-escape.patch

Review of attachment 8419621 [details] [diff] [review]:
-----------------------------------------------------------------

Bobby, could you review the changes in Sandbox.cpp and xpcprivate.h?

        Attachment #8419621 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8419621 [details] [diff] [review]
css-logic-escape.patch

Review of attachment 8419621 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a test for this in js/xpconnect/tests/unit, along the lines of test_url.js.

r=bholley with that fixed and the comment removed.

::: js/xpconnect/src/xpcprivate.h
@@ +3290,5 @@
>  
>  struct GlobalProperties {
>      GlobalProperties(bool aPromise) {
>        mozilla::PodZero(this);
> +      // XXXbz why are we ignoring aPromise?

Because the argument was made that Promises are really ES objects rather than DOM objects, and thus should be subject to the ES exposure policy.

        Attachment #8419621 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
> and thus should be subject to the ES exposure policy.

Can we just nix the arg then?

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #26)
> > and thus should be subject to the ES exposure policy.
> 
> Can we just nix the arg then?

Well, we currently allow people to switch it off by passing "-Promise" to SandboxOptions. I don't know how important that really is though.

    
    

    
  
Passing -Promise doesn't use that constructor arg, though.

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #28)
> Passing -Promise doesn't use that constructor arg, though.

I'm really fine with whatever if someone writes a patch.

    
    

    
  
Comment on attachment 8419621 [details] [diff] [review]
css-logic-escape.patch

Review of attachment 8419621 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/styleinspector/css-logic.js
@@ +867,5 @@
>  CssLogic.findCssSelector = function CssLogic_findCssSelector(ele) {
>    var document = ele.ownerDocument;
> +  if (!document.contains(ele)) {
> +    console.error('findCssSelector received element not inside document');
> +    return '';

We can throw here, right? I don't think anything can depend on getting a string back from a detached element, can it? (given that in many cases it would have excepted before)

        Attachment #8419621 -
        Flags: review?(jwalker) → review+

    
    

    
  


  
Added test_css.js for testing importGlobalProperties(["CSS"])
Throw on unattached node in findCssSelector

Try push: https://tbpl.mozilla.org/?tree=Try&rev=09e4fe2fbc1a

        Attachment #8420094 -
        Flags: review+

    
  

        Attachment #8419621 -
        Attachment is obsolete: true

    
    

    
  
Eddy, I should check to make sure that the require in script.js will not interfere with what you are doing with worker debugging.  It now requires css-logic (http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/styleinspector/css-logic.js) after landing this patch: https://hg.mozilla.org/integration/fx-team/rev/9e37eb7fbde3#l9.12.  Is this fine, or is there a change needed to support worker debugging?
Flags: needinfo?(ejpbruel)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9e37eb7fbde3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #34)
> Is this fine, or is there a change needed to support worker debugging?

Probably switching from require() to lazyRequireGetter() from bug 991904 should suffice.

    
    

    
  

      
      
      
      

        Attached file
          
        test.html
        — 
      

    


  

    
    

    
  
Verified fixed FF 32.0a1 (2014-05-11), Win 7 x64
Status: RESOLVED → VERIFIED

          status-firefox32:
          --- → verified

    
    

    
  
I believe we already discussed this on irc. Clearing the needinfo.
Flags: needinfo?(ejpbruel)
Blocks: 1014459

    
    

    
  
Comment on attachment 8420094 [details] [diff] [review]
css-logic-escape.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 701419
User impact if declined: DevTools inspector breaks if page is reloaded when certain elements are highlighted.  Elements that are affected have a class or ID selector that needs to be escaped for CSS.
Testing completed (on m-c, etc.): On m-c since 05-09.  Patch includes test coverage
Risk to taking this patch (and alternatives if risky): Mostly DevTools only.  This patch also allows the CSS global to be imported via Cu.importGlobalProperties(["CSS"])
String or IDL/UUID changes made by this patch:

        Attachment #8420094 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8420094 [details] [diff] [review]
css-logic-escape.patch

Aurora approval granted. Please land this week so that we get at least a week of bake time on Aurora before uplifting to Beta.

        Attachment #8420094 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Backed out for mochitest-dt bustage. Please provide a branch patch for Aurora uplift.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c17cea2508b

          status-firefox31:
          fixed → affected
Keywords: branch-patch-needed

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #43)
> Backed out for mochitest-dt bustage. Please provide a branch patch for
> Aurora uplift.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/7c17cea2508b

This is caused by the error: 'redeclaration of const CssLogic' in script.js.  I actually have green tests by switching this to a let, but I have no idea why (it isn't defined anywhere else in the file).  It seems like script.js has been reorganized a bit in between 31 and 32, and I'm not going to have time to fully investigate this week.  Given this fact and that a week of bake time in Aurora was requested I'm not going to plan to uplift this.
Keywords: branch-patch-needed

    
  

        Attachment #8420094 -
        Flags: approval-mozilla-aurora+
Depends on: 1087801

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: