1060933 - Inspector doesn't work on http://smartsearch.altervista.org

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Tim Nguyen :ntim]

When opening the inspector on http://smartsearch.altervista.org , the inspector is broken. The markup view is blank, the breadcrumbs are not showing, the style sidebar is gone.

Browser console logs : 
**********************

DOMException [SyntaxError: "An invalid or illegal string was specified"
code: 12
nsresult: 0x8053000c
location: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/event-parsers.js:223] protocol.js:852

"Protocol error: SyntaxError: An invalid or illegal string was specified" protocol.js:16

"Protocol error: SyntaxError: An invalid or illegal string was specified" Promise-backend.js:868


When closing the toolbox :
**************************

"Panel inspector:" TypeError: panel is undefined
Stack trace:
Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1492:9

Comment 1

[Tim Nguyen :ntim]

Since it's related to event parsers, it might be the recent Jquery event support that broke the inspector (the page uses Jquery).

Comment 2

[Tim Nguyen :ntim]

I can't seem to reproduce this *just* by using illegal strings into a Jquery selector.

Comment 3

[Tim Nguyen :ntim]

Huh, using the selector : "body >button[data-type='\|&;\$%@<>\(\)\+,']:not(body)" doesn't fail. I wonder what's wrong in the code.

Comment 4

[Alice0775 White]

[Tracking Requested - why for this release]: regression

Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=b48dd68342d7&tochange=9add1ec0251d

Regressed by
9add1ec0251d	Michael Ratcliffe — Bug 1044932 - Add JQuery support to visual events r=pbrosset

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We need to make this bomb proof.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This is caused by invalid pseudos e.g.
let node = document.body;
node.matches(".toolbar-button:first"); // An invalid or illegal string was specified

Comment 7

[Tim Nguyen :ntim]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> This is caused by invalid pseudos e.g.
> let node = document.body;
> node.matches(".toolbar-button:first"); // An invalid or illegal string was
> specified

This is a valid pseudo element in Jquery. Also, the way this is checked isn't bullet proof since Jquery has many non standard extensions. You might want to check for event.target instead.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: inspector-broke-on-smartsearch-altavista-org.patch

(In reply to Tim Nguyen [:ntim] from comment #7)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> > This is caused by invalid pseudos e.g.
> > let node = document.body;
> > node.matches(".toolbar-button:first"); // An invalid or illegal string was
> > specified
> 
> This is a valid pseudo element in Jquery.

It is valid in CSS3 but we just don't support it yet.

>  Also, the way this is checked
> isn't bullet proof since Jquery has many non standard extensions. You might
> want to check for event.target instead.

That would work for jQuery events but this is the check for jQuery Live events. This is a single event on the document node that bubbles to all other nodes. When the selector matches a node it is raised on that node so we need the selector.

Patch notes:
- Skips the event in the case of an invalid / unsupported selector.
- If getListeners, hasListeners or normalizeHandler throws an exception then silently continue.

try / catch in our JS engine is very low cost so this seems like the best solution... especially considering that there will be add-ons that could break the inspector by not covering every situation in their event parsers.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8482268 [details] [diff] [review]
inspector-broke-on-smartsearch-altavista-org.patch

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

Yeah, no reason to throw here when dealing extension-like code.  Will you (a) add a test for this error and (b) update the commit message to be a bit more descriptive?

::: toolkit/devtools/event-parsers.js
@@ +221,5 @@
>          }
>  
> +        let matches;
> +        try {
> +          matches = node.matches && node.matches(selector);

I think you could do CSS.escape (see css-logic.js) instead of try/catch.  Pretty sure that is the only case where this function throws, and it would bring support to elements like <div id="1"></div>

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: inspector-broke-on-smartsearch-altavista-org.patch

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Comment on attachment 8482268 [details] [diff] [review]
> inspector-broke-on-smartsearch-altavista-org.patch
> 
> Review of attachment 8482268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, no reason to throw here when dealing extension-like code.  Will you
> (a) add a test for this error

Added $(document).on( "dragout", "#livediv:xxxxx", handler10);

> and (b) update the commit message to be a bit
> more descriptive?
> 

Bug 1060933 - Prevent jQuery Live event bubbles from throwing on invalid selector r=bgrins

> ::: toolkit/devtools/event-parsers.js
> @@ +221,5 @@
> >          }
> >  
> > +        let matches;
> > +        try {
> > +          matches = node.matches && node.matches(selector);
> 
> I think you could do CSS.escape (see css-logic.js) instead of try/catch. 
> Pretty sure that is the only case where this function throws, and it would
> bring support to elements like <div id="1"></div>

CSS.escape is only to be used on parts of CSS selectors e.g. an id such as "someid". It escapes "#livediv" as "\#livediv" and still throws on invalid selectors. The selectors themselves come from user code so we have no control over them.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=dcc14c6f46aa
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dcc14c6f46aa

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> > ::: toolkit/devtools/event-parsers.js
> > @@ +221,5 @@
> > >          }
> > >  
> > > +        let matches;
> > > +        try {
> > > +          matches = node.matches && node.matches(selector);
> > 
> > I think you could do CSS.escape (see css-logic.js) instead of try/catch. 
> > Pretty sure that is the only case where this function throws, and it would
> > bring support to elements like <div id="1"></div>
> 
> CSS.escape is only to be used on parts of CSS selectors e.g. an id such as
> "someid". It escapes "#livediv" as "\#livediv" and still throws on invalid
> selectors. The selectors themselves come from user code so we have no
> control over them.

OK, sounds good

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8482770 [details] [diff] [review]
inspector-broke-on-smartsearch-altavista-org.patch

https://hg.mozilla.org/integration/fx-team/rev/72d19d0827de

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72d19d0827de

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8482770 [details] [diff] [review]
inspector-broke-on-smartsearch-altavista-org.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1044932 
[User impact if declined]: Broken inspector on some sites
[Describe test coverage new/current, TBPL]: On m-c for a while now, covered by tests.
[Risks and why]: Low
[String/UUID change made/needed]: Nope

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8482770 [details] [diff] [review]
inspector-broke-on-smartsearch-altavista-org.patch

Aurora+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/651090c12c72

Comment 19

[Andrei Vaida [:avaida]]

Reproduced on Nightly 34.0a1 (2014-08-31) using Windows 7 64-bit.

Verified fixed on Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5, using:
 * Firefox 34.0b1 (build2: 20141014134955),
 * Aurora 35.0a2 (2014-10-14),
 * Nightly 35.0a1 (2014-10-13).

Please note that there are two similar errors thrown in the Browser Console once you close Developer Tools: http://pastebin.mozilla.org/6792615 in protocol.js:16 and in deprecated-sync-thenables.js:48.
 * note: these two errors are encountered across platforms

Michael, any thoughts on this? Also, if there are any other similar websites that could be used to confirm the fix, please let me know, I only checked with the one from Comment 0.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Andrei Vaida, QA [:avaida] from comment #19)
> Reproduced on Nightly 34.0a1 (2014-08-31) using Windows 7 64-bit.
> 
> Verified fixed on Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X
> 10.9.5, using:
>  * Firefox 34.0b1 (build2: 20141014134955),
>  * Aurora 35.0a2 (2014-10-14),
>  * Nightly 35.0a1 (2014-10-13).
> 
> Please note that there are two similar errors thrown in the Browser Console
> once you close Developer Tools: http://pastebin.mozilla.org/6792615 in
> protocol.js:16 and in deprecated-sync-thenables.js:48.
>  * note: these two errors are encountered across platforms
> 
> Michael, any thoughts on this? Also, if there are any other similar websites
> that could be used to confirm the fix, please let me know, I only checked
> with the one from Comment 0.

If you have a crash it must be due to a new problem.

Can you try again and ensure that E10S is disabled... our tools are not yet fully compatible with E10S.

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #20)
> If you have a crash it must be due to a new problem.
There is no crash involved, just these two similar errors thrown in the Browser Console once the 
Developer Tools are closed.

> Can you try again and ensure that E10S is disabled... our tools are not yet
> fully compatible with E10S.
This issue is reproducible on Firefox 34.0b1 (build2: 20141014134955) where e10s is disabled by default. e10s was also disabled at the time of testing on both Aurora and Nightly builds mentioned in Comment 19.

Sorry for the late reply, I was caught up with something else.