The default bug view has changed. See this FAQ.

[highlighter] If a <embed> node is highlighted, a "node.getSVGDocument() is null" exception is raised

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-devtools])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
For example: http://www.nytimes.com - it happens while highlighting the top left and top right corners.
(Assignee)

Comment 1

6 years ago
... the top left and top right *ads*.
(Assignee)

Updated

6 years ago
Blocks: 663830
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.
Component: Developer Tools → DOM
Product: Firefox → Core
QA Contact: developer.tools → general
happens in Fx4 and up.
OS: Linux → All
Hardware: x86 → All
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.
Component: DOM → Developer Tools
Product: Core → Firefox
QA Contact: general → developer.tools
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.
Seems like you just want a null check.
Indeed, comment 6 is what I was about to say.
(Assignee)

Comment 8

6 years ago
Created attachment 540915 [details] [diff] [review]
patch v1

null check
Assignee: nobody → paul
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #540915 - Flags: review?(rcampbell)
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.
(Assignee)

Comment 10

6 years ago
What should we test here?
(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
(Assignee)

Comment 12

6 years ago
Created attachment 542450 [details] [diff] [review]
patch v1.1 (with test)

Just added a test.
Attachment #540915 - Attachment is obsolete: true
Attachment #542450 - Flags: review?(rcampbell)
Attachment #540915 - Flags: review?(rcampbell)
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!
Attachment #542450 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 542466 [details] [diff] [review]
patch v1.2

Addressed Rob's comments.
(Assignee)

Updated

6 years ago
Attachment #542466 - Flags: review?(gavin.sharp)
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?
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
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
Attachment #542450 - Attachment is obsolete: true
Attachment #542466 - Attachment is obsolete: true
Attachment #542746 - Flags: review?
Attachment #542466 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #542746 - Flags: review? → review?(gavin.sharp)
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!
Attachment #542746 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 19

6 years ago
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.
Attachment #542746 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #542922 - Flags: review?(gavin.sharp)
Attachment #542922 - Flags: review?(gavin.sharp) → review+
Whiteboard: [fixed-in-devtools]
Version: unspecified → Trunk
Comment on attachment 542922 [details] [diff] [review]
[in-devtools] patch v1.4

http://hg.mozilla.org/projects/devtools/rev/fae9a993f933
Attachment #542922 - Attachment description: patch v1.4 → [in-devtools] patch v1.4
Comment on attachment 542922 [details] [diff] [review]
[in-devtools] patch v1.4

http://hg.mozilla.org/mozilla-central/rev/fae9a993f933
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.