browser chrome mochitests trigger uncaught JS exception in CssHtmlTree.jsm

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: heycam, Assigned: miker)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [styleinspector][has-patch][fixed-in-fx-team])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
browser_bug683672.js, browser_styleinspector.js and browser_styleinspector_webconsole.js trigger an exception in CssHtmlTree.jsm:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug683672.js | an unexpected uncaught JS exception reported through window.onerror - this.propertyContainer is undefined at resource:///modules/devtools/CssHtmlTree.jsm:299
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector.js | an unexpected uncaught JS exception reported through window.onerror - this.propertyContainer is undefined at resource:///modules/devtools/CssHtmlTree.jsm:299
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | an unexpected uncaught JS exception reported through window.onerror - this.propertyContainer is undefined at resource:///modules/devtools/CssHtmlTree.jsm:299

(These are not reported as TEST-UNEXPECTED-FAILs yet, just with my local patches.)
Created attachment 574851 [details] [diff] [review]
window.onerror error fix

propertyContainer is defined as:
<div id="propertyContainer">
</div>

We get it when CssHtmlTree is instantiated: 
this.propertyContainer = this.styleDocument.getElementById("propertyContainer");

In the highlight method the following will always work unless the window is in the process of closing:
this.propertyContainer.appendChild(fragment);

This is most likely window.onerror catching errors from JS running when the window is in the process of closing. If this is the problem here then this patch should fix it.
Attachment #574851 - Flags: review?(dcamp)
(Reporter)

Comment 2

6 years ago
I tested this patch (with my local changes also) and it fixes the test failures.

Comment 3

6 years ago
Is the load being triggered after destroy() is called, or is an in-progress refresh completing?

Seems like destroy() should properly cancel an in-progress refresh and prevent new refreshes from being scheduled, or we'll see other similar bugs crop up.
Attachment #574851 - Flags: review?(dcamp)
It seems that on my windows box this error is breaking tests. We need to cancel the panel updates etc. if the panel is destroyed.
Created attachment 577583 [details] [diff] [review]
Stop on destroy
Attachment #574851 - Attachment is obsolete: true
Attachment #577583 - Flags: review?(dcamp)
Blocks: 704132
(Reporter)

Comment 6

6 years ago
As part of this patch can you please remove these two calls to ignoreAllUncaughtExceptions(), which I just added as part of bug 670857?

http://hg.mozilla.org/mozilla-central/file/tip/browser/devtools/styleinspector/test/browser/browser_bug683672.js#l17

http://hg.mozilla.org/mozilla-central/file/tip/browser/devtools/styleinspector/test/browser/browser_styleinspector.js#l82
(Reporter)

Comment 7

6 years ago
(As part of bug 703176, rather.)

Comment 8

6 years ago
Why isn't the update cancelled in the same place the propertyContainer is destroyed?
Whiteboard: [styleinspector]
Created attachment 579687 [details] [diff] [review]
Stop on destroy

(In reply to Cameron McCormack (:heycam) from comment #6)
> As part of this patch can you please remove these two calls to
> ignoreAllUncaughtExceptions(), which I just added as part of bug 670857?
> 

Done

(In reply to Dave Camp (:dcamp) from comment #8)
> Why isn't the update cancelled in the same place the propertyContainer is
> destroyed?

Yup, that is a better place ... done.
Attachment #577583 - Attachment is obsolete: true
Attachment #577583 - Flags: review?(dcamp)
Attachment #579687 - Flags: review?(dcamp)
Whiteboard: [styleinspector] → [styleinspector][has-patch]

Updated

6 years ago
Attachment #579687 - Flags: review?(dcamp) → review+
Shouldn't this be [land-in-fx-team]? The patch has r+.

(and assigned to mike?)
Yes, it should ... done.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Whiteboard: [styleinspector][has-patch] → [styleinspector][has-patch][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/18d3b3a1f605
Whiteboard: [styleinspector][has-patch][land-in-fx-team] → [styleinspector][has-patch][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/18d3b3a1f605
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
You need to log in before you can comment on or make changes to this bug.