Last Comment Bug 665880 - [highlighter] If a <embed> node is highlighted, a "node.getSVGDocument() is null" exception is raised
: [highlighter] If a <embed> node is highlighted, a "node.getSVGDocument() is n...
Status: RESOLVED FIXED
[fixed-in-devtools]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Paul Rouget [:paul]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 663830
  Show dependency treegraph
 
Reported: 2011-06-21 07:09 PDT by Paul Rouget [:paul]
Modified: 2011-07-04 11:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.06 KB, patch)
2011-06-21 15:38 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.1 (with test) (5.14 KB, patch)
2011-06-28 06:55 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
patch v1.2 (5.17 KB, patch)
2011-06-28 07:34 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v1.3 (3.68 KB, patch)
2011-06-29 01:55 PDT, Paul Rouget [:paul]
gavin.sharp: review-
Details | Diff | Splinter Review
[in-devtools] patch v1.4 (3.48 KB, patch)
2011-06-29 13:30 PDT, Paul Rouget [:paul]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-06-21 07:09:19 PDT
For example: http://www.nytimes.com - it happens while highlighting the top left and top right corners.
Comment 1 Paul Rouget [:paul] 2011-06-21 07:10:52 PDT
... the top left and top right *ads*.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-06-21 07:45:19 PDT
happens in the test page if you run this in a Web Console / Scratchpad:

document.getElementsByTagName("embed")[0] instanceof GetSVGDocument

returns true.

I think this may be a bug in GetSVGDocument.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-06-21 07:46:10 PDT
happens in Fx4 and up.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-06-21 09:27:06 PDT
Why is this a DOM bug, exactly?

The nsIDOMGetSVGDocument interface is implemented by <embed>, but it only returns non-null when the <embed> is showing an SVG document.

Therefore this code in browser/base/content/inspector.js:

526     if (node instanceof GetSVGDocument) {
527       // then the node is a frame
....
531         let skipChild = node.getSVGDocument().documentElement;

is bogus, as is the comment.  I have no idea what it's trying to do, but it's just wrong.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-06-21 11:11:00 PDT
ok. I suspect that's a firebug-ism we ported over. I suspect the intention was to try and detect if a given node was an SVG document and if so, deal with that case in the tree construction.

Paul, we'll have to try debugging this to see what's really happening here.

Thanks for the tip, Boris.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-21 11:15:07 PDT
Seems like you just want a null check.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-06-21 11:18:31 PDT
Indeed, comment 6 is what I was about to say.
Comment 8 Paul Rouget [:paul] 2011-06-21 15:38:38 PDT
Created attachment 540915 [details] [diff] [review]
patch v1

null check
Comment 9 Rob Campbell [:rc] (:robcee) 2011-06-22 07:36:19 PDT
Comment on attachment 540915 [details] [diff] [review]
patch v1

This looks like it should fix, but we are going to need a unittest for this, which could be tricky.
Comment 10 Paul Rouget [:paul] 2011-06-27 09:05:56 PDT
What should we test here?
Comment 11 Rob Campbell [:rc] (:robcee) 2011-06-27 09:41:43 PDT
(In reply to comment #10)
> What should we test here?

09:39 <@robcee> paul: so, bug 665880, what's the failure condition when we have
                that <embed>?
09:39 <@robcee> did it just not show up in the html tree?
09:40 < paul> exception in the Error console, and, iirc, element not selected
09:40 <@robcee> ah, right
09:40 <@robcee> easiest and most direct is probable verifying that the element
                does get selected
09:40 <@robcee> it should fail without your fix applied and then pass when it is
Comment 12 Paul Rouget [:paul] 2011-06-28 06:55:08 PDT
Created attachment 542450 [details] [diff] [review]
patch v1.1 (with test)

Just added a test.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-06-28 07:04:20 PDT
Comment on attachment 542450 [details] [diff] [review]
patch v1.1 (with test)

+    try {
+      InspectorUI.inspectNode(objectNode);
+    } catch(e) {
+      ok(false, "Exception raised from InspectorUI.inspectNode(): " + e);
+    }
+  });

I think I'd prefer to see a variable (something like let safeInspect = true;) that would get set to false in the catch block of the try.

Then you can move the ok check outside of the catch block and have ok(safeInspect, "node inspected") or similar so the test shows both pass and failure conditions.

otherwise good!
Comment 14 Paul Rouget [:paul] 2011-06-28 07:34:27 PDT
Created attachment 542466 [details] [diff] [review]
patch v1.2

Addressed Rob's comments.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-28 10:42:25 PDT
Comment on attachment 542466 [details] [diff] [review]
patch v1.2

>diff -r bb9d484ddc7e browser/base/content/test/inspector/browser_inspector_bug_665880.js

You can use the shorter/simpler Public Domain license header for this test instead, if you prefer:
https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

In general I prefer tests that make use of closures rather than global variables, e.g. browser_loadDisallowInherit.js.

>+function runObjectInspectionTest()

>+    try {
>+      InspectorUI.inspectNode(objectNode);
>+    } catch(e) {
>+      safeInspect = false;
>+    }

I don't think the test needs to explicitly check for exceptions thrown by inspectNode here. Does the test fail reasonably without this (and without the fix applied)?

>+function finishUp() {
>+  InspectorUI.closeInspectorUI();

Do you need to wait for INSPECTOR_NOTIFICATIONS.CLOSED before finish()ing the test?
Comment 16 Paul Rouget [:paul] 2011-06-29 01:42:10 PDT
(In reply to comment #15)
> Comment on attachment 542466 [details] [diff] [review] [review]
> patch v1.2
> 
> >diff -r bb9d484ddc7e browser/base/content/test/inspector/browser_inspector_bug_665880.js
> 
> You can use the shorter/simpler Public Domain license header for this test
> instead, if you prefer:
> https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

Ok.

> In general I prefer tests that make use of closures rather than global
> variables, e.g. browser_loadDisallowInherit.js.

Ok.

> >+function runObjectInspectionTest()
> 
> >+    try {
> >+      InspectorUI.inspectNode(objectNode);
> >+    } catch(e) {
> >+      safeInspect = false;
> >+    }
> 
> I don't think the test needs to explicitly check for exceptions thrown by
> inspectNode here. Does the test fail reasonably without this (and without
> the fix applied)?

This exception is raised:
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_bug_665880.js | Console message: [JavaScript Error: "node.getSVGDocument() is null" {file: "chrome://browser/content/browser.js" line: 1791}]

I realize that performTestComparison is not called because INSPECTOR_NOTIFICATIONS.HIGHLIGHTING is not fired
because of the exception. So I have to keep this try/catch and add an explicit call to performTestComparison.

> >+function finishUp() {
> >+  InspectorUI.closeInspectorUI();
> 
> Do you need to wait for INSPECTOR_NOTIFICATIONS.CLOSED before finish()ing
> the test?

You're right.
Comment 17 Paul Rouget [:paul] 2011-06-29 01:55:03 PDT
Created attachment 542746 [details] [diff] [review]
Patch v1.3

Changes in the test:
× public domain license
× got rid of global variables
× kept the try/catch and added an explicit call to performTestComparison() in catch{}
× wait for INSPECTOR_NOTIFICATIONS.CLOSED before finishing the test
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-29 11:36:02 PDT
Comment on attachment 542746 [details] [diff] [review]
Patch v1.3

>diff -r 91aa42463755 browser/base/content/test/inspector/browser_inspector_bug_665880.js

>+  function runObjectInspectionTest()

>+    Services.obs.removeObserver(runObjectInspectionTest,
>+      INSPECTOR_NOTIFICATIONS.OPENED, false);

nit: removeObserver doesn't take a third argument (applies to multiple calls in this test)

>+      try {
>+        InspectorUI.inspectNode(objectNode);
>+      } catch(e) {
>+        safeInspect = false;
>+        performTestComparison();
>+      }

I still don't really like this. If you just avoid the try/catch, presumably the failure case is that the test prints out the exception (as an INFO message) and the test times out, right? I think that's perfectly fine - as this is it's overly specific to inspectNode throwing in a certain way. Otherwise this looks good. Ping me on IRC if I'm not being clear!
Comment 19 Paul Rouget [:paul] 2011-06-29 13:30:41 PDT
Created attachment 542922 [details] [diff] [review]
[in-devtools] patch v1.4

(In reply to comment #18)
> nit: removeObserver doesn't take a third argument (applies to multiple calls
> in this test)

Fixed.

> >+      try {
> >+        InspectorUI.inspectNode(objectNode);
> >+      } catch(e) {
> >+        safeInspect = false;
> >+        performTestComparison();
> >+      }
> 
> I still don't really like this. If you just avoid the try/catch, presumably
> the failure case is that the test prints out the exception (as an INFO
> message) and the test times out, right? I think that's perfectly fine - as
> this is it's overly specific to inspectNode throwing in a certain way.
> Otherwise this looks good. Ping me on IRC if I'm not being clear!

Timeout confirmed. I removed the try/catch.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-06-30 06:24:06 PDT
Comment on attachment 542922 [details] [diff] [review]
[in-devtools] patch v1.4

http://hg.mozilla.org/projects/devtools/rev/fae9a993f933
Comment 21 Rob Campbell [:rc] (:robcee) 2011-07-04 11:16:19 PDT
Comment on attachment 542922 [details] [diff] [review]
[in-devtools] patch v1.4

http://hg.mozilla.org/mozilla-central/rev/fae9a993f933

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