Closed Bug 817294 Opened 13 years ago Closed 12 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: 12 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: