Closed Bug 817294 Opened 12 years ago Closed 11 years ago

Intermittent browser_styleeditor_loading.js | content document is still loading - Didn't expect complete, but got it | style editor root element has 'loading' class name | new style sheet button is disabled | import button is disabled

Categories

(DevTools :: Style Editor, defect, P1)

Product:
DevTools
Component:
Style Editor
Type:
defect

Tracking

(firefox20 fixed, firefox21 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 21
Tracking Flags:
Tracking Status
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: RyanVM, Assigned: miker)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Final patch
27.60 KB, patch
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=17499955&tree=Mozilla-Inbound

Rev4 MacOSX Lion 10.7 mozilla-inbound opt test mochitest-browser-chrome on 2012-11-30 12:56:54 PST for push 40e050424563
slave: talos-r4-lion-044

TEST-START | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | content document is still loading - Didn't expect complete, but got it
Stack trace:
    JS frame :: chrome://mochikit/content/browser-test.js :: test_isnot :: line 478
    JS frame :: chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js :: <TOP_LEVEL> :: line 21
    JS frame :: chrome://mochikit/content/browser-test.js :: test_executeSoon/<.run :: line 503
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | style editor root element has 'loading' class name
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js :: <TOP_LEVEL> :: line 25
    JS frame :: chrome://mochikit/content/browser-test.js :: test_executeSoon/<.run :: line 503
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | new style sheet button is disabled
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js :: <TOP_LEVEL> :: line 29
    JS frame :: chrome://mochikit/content/browser-test.js :: test_executeSoon/<.run :: line 503
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | import button is disabled
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js :: <TOP_LEVEL> :: line 33
    JS frame :: chrome://mochikit/content/browser-test.js :: test_executeSoon/<.run :: line 503
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-PASS | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | content document is complete
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | style editor root element does not have 'loading' class name anymore
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | new style sheet button is enabled
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | import button is enabled
INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js | finished in 321ms
Blocks: 788977
Depends on: 816946
Priority: -- → P1
Depends on: 818431
Blocks: 820315
No longer depends on: 816946
Joe, please could you take a look at this top orange? (Presume fallout from dev tools window landing)
Flags: needinfo?(jwalker)
Thanks Ed. It's on my list.
Flags: needinfo?(jwalker)
Failure rate too high; disabled whilst Joe investigates:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e552d399e2e7
Whiteboard: [test disabled][leave open]
FWIW, this testcase was causing problems in my work on bug 715376 as well and I had disabled it locally.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
A couple of try builds that show this problem occurs about once in every 20 test runs when enabled.
OS: Mac OS X → All
In both the working and failing versions we have the following error immediately before the failing tests:

JavaScript strict warning: resource://gre/modules/devtools/StyleEditorChrome.jsm, line 286: reference to undefined property this._view
TypeError: this._view is undefined: SEC__resetChrome@resource://gre/modules/devtools/StyleEditorChrome.jsm:286
StyleEditor_reset@resource://gre/modules/devtools/StyleEditorPanel.jsm:88
EventEmitter_emit@resource:///modules/devtools/EventEmitter.jsm:100
TWPL_onStateChange@resource://gre/modules/devtools/Target.jsm:317
@chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js:18
@chrome://mochitests/content/browser/browser/devtools/styleeditor/test/head.js:47
effort@resource://gre/modules/commonjs/promise/core.js:53
resolveDeferred@resource://gre/modules/commonjs/promise/core.js:125
then@resource://gre/modules/commonjs/promise/core.js:34
resolve@resource://gre/modules/commonjs/promise/core.js:167
@resource:///modules/devtools/gDevTools.jsm:206
@resource:///modules/devtools/EventEmitter.jsm:56
EventEmitter_emit@resource:///modules/devtools/EventEmitter.jsm:100
@resource://gre/modules/devtools/Toolbox.jsm:471
effort@resource://gre/modules/commonjs/promise/core.js:53
resolveDeferred@resource://gre/modules/commonjs/promise/core.js:125
then@resource://gre/modules/commonjs/promise/core.js:34
then@resource://gre/modules/commonjs/promise/core.js:138
@resource://gre/modules/devtools/Toolbox.jsm:477
I will have to come back to this one.
Assignee: mratcliffe → nobody
Status: ASSIGNED → NEW
Followup to vidyo conversation - we also should probably remove executeSoon from browser_styleeditor_loading.js - that smells for reasons we discussed.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — — Splinter Review 
This patch ses promises so that the style editor no longer lies about being ready. In fact, as a result we no longer need the onChromeAttached event so I have removed that.

There were also a bunch of obvious off-by-one errors that I have fixed whilst I was at it.

The style editor tests all now appear solid (or at least at the time of writing):
https://tbpl.mozilla.org/?tree=Try&rev=868f9d9d936d
Attachment #709696 - Flags: review?(jwalker)
Whiteboard: [test disabled][leave open] → [test disabled][leave open][has-patch]
Comment on attachment 709696 [details] [diff] [review]
Patch

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

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ -129,5 @@
>    get styleSheetIndex()
>    {
>      let document = this.contentDocument;
>      if (this._styleSheetIndex == -1) {
> -      for (let i = 0; i < document.styleSheets.length; ++i) {

This seems too obviously wrong to be an accident. Are you sure that there isn't some logic here? (Same for other places)

::: browser/devtools/styleeditor/StyleEditorChrome.jsm
@@ +372,5 @@
>        }
>  
> +        let summary = sheet ? this.getSummaryElementForEditor(aEditor)
> +                          : this._view.getSummaryElementByOrdinal(0);
> +        this._view.activeSummary = summary;

Nit: maybe some dodgy indenting here and at this.selectStyleSheet.bind above? Feel free to ignore if you'd like.

::: browser/devtools/styleeditor/StyleEditorPanel.jsm
@@ +75,5 @@
>        let chromeRoot = this._panelDoc.getElementById("style-editor-chrome");
>        let chrome = new StyleEditorChrome(chromeRoot, contentWindow);
> +      let promise = chrome.open();
> +
> +      if (chrome) {

if chrome is falsy, then line 77 probably threw?
So we can probably remove the if, as it's always true
Attachment #709696 - Flags: review?(jwalker) → review+
Attached patch Final patch — — Splinter Review 
(In reply to Joe Walker [:jwalker] from comment #153)
> Comment on attachment 709696 [details] [diff] [review]
> Patch
> 
> Review of attachment 709696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleeditor/StyleEditor.jsm
> @@ -129,5 @@
> >    get styleSheetIndex()
> >    {
> >      let document = this.contentDocument;
> >      if (this._styleSheetIndex == -1) {
> > -      for (let i = 0; i < document.styleSheets.length; ++i) {
> 
> This seems too obviously wrong to be an accident. Are you sure that there
> isn't some logic here? (Same for other places)
> 

Turns out that preincrement works the same as postincrement in for loops. I am sure that in my Turbo C days this wasn't the case. Anyhow, i++ is clearer IMO.

> ::: browser/devtools/styleeditor/StyleEditorChrome.jsm
> @@ +372,5 @@
> >        }
> >  
> > +        let summary = sheet ? this.getSummaryElementForEditor(aEditor)
> > +                          : this._view.getSummaryElementByOrdinal(0);
> > +        this._view.activeSummary = summary;
> 
> Nit: maybe some dodgy indenting here and at this.selectStyleSheet.bind
> above? Feel free to ignore if you'd like.
> 

Now non-dodgy.

> ::: browser/devtools/styleeditor/StyleEditorPanel.jsm
> @@ +75,5 @@
> >        let chromeRoot = this._panelDoc.getElementById("style-editor-chrome");
> >        let chrome = new StyleEditorChrome(chromeRoot, contentWindow);
> > +      let promise = chrome.open();
> > +
> > +      if (chrome) {
> 
> if chrome is falsy, then line 77 probably threw?
> So we can probably remove the if, as it's always true

Agreed, removed.
Attachment #709696 - Attachment is obsolete: true
Whiteboard: [test disabled][leave open][has-patch] → [test disabled][leave open][land-in-fx-team]
Whiteboard: [test disabled][leave open][land-in-fx-team] → [test disabled][leave open][fixed-in-fx-team]
Whiteboard: [test disabled][leave open][fixed-in-fx-team] → [test disabled]
Comment on attachment 710715 [details] [diff] [review]
Final patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Style Editor
User impact if declined: None, intermittent test failures on Aurora
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): No risk, just fixes issues with the style editor's startup mechanism.
String or UUID changes made by this patch: None
Attachment #710715 - Flags: approval-mozilla-aurora?
Comment on attachment 710715 [details] [diff] [review]
Final patch

Low risk, fixes intermittent test failures on Aurora .
Attachment #710715 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [test disabled] → [land-in-fx-aurora]
Whiteboard: [land-in-fx-aurora] → [land-in-aurora]
Whiteboard: [land-in-aurora]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox20: affected → fixed
status-firefox21: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-aurora]
Target Milestone: --- → Firefox 21
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: