Last Comment Bug 702861 - browser chrome mochitests trigger uncaught JS exception in CssHtmlTree.jsm
: browser chrome mochitests trigger uncaught JS exception in CssHtmlTree.jsm
Status: RESOLVED FIXED
[styleinspector][has-patch][fixed-in-...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
:
Mentors:
Depends on:
Blocks: 702050 704132
  Show dependency treegraph
 
Reported: 2011-11-15 18:30 PST by Cameron McCormack (:heycam)
Modified: 2011-12-16 11:19 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
window.onerror error fix (1.41 KB, patch)
2011-11-16 01:57 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Stop on destroy (1.91 KB, patch)
2011-11-29 06:04 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Stop on destroy (2.67 KB, patch)
2011-12-07 07:41 PST, Michael Ratcliffe [:miker] [:mratcliffe]
dcamp: review+
Details | Diff | Splinter Review

Description Cameron McCormack (:heycam) 2011-11-15 18:30:56 PST
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.)
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-16 01:57:11 PST
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.
Comment 2 Cameron McCormack (:heycam) 2011-11-16 12:30:56 PST
I tested this patch (with my local changes also) and it fixes the test failures.
Comment 3 Dave Camp (:dcamp) 2011-11-17 09:39:16 PST
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.
Comment 4 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-29 05:24:47 PST
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.
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-29 06:04:59 PST
Created attachment 577583 [details] [diff] [review]
Stop on destroy
Comment 6 Cameron McCormack (:heycam) 2011-12-01 16:31:55 PST
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
Comment 7 Cameron McCormack (:heycam) 2011-12-01 16:32:46 PST
(As part of bug 703176, rather.)
Comment 8 Dave Camp (:dcamp) 2011-12-02 10:45:04 PST
Why isn't the update cancelled in the same place the propertyContainer is destroyed?
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-12-07 07:41:03 PST
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.
Comment 10 Mihai Sucan [:msucan] 2011-12-13 10:53:13 PST
Shouldn't this be [land-in-fx-team]? The patch has r+.

(and assigned to mike?)
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2011-12-13 11:13:44 PST
Yes, it should ... done.
Comment 12 Rob Campbell [:rc] (:robcee) 2011-12-16 06:03:12 PST
https://hg.mozilla.org/integration/fx-team/rev/18d3b3a1f605
Comment 13 Rob Campbell [:rc] (:robcee) 2011-12-16 11:19:20 PST
https://hg.mozilla.org/mozilla-central/rev/18d3b3a1f605

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