This patch switches the style editor and style inspector to using Promise.jsm.

I'm hoping this fixes the issue. I can't reproduce this locally, presumably because it's a very close timing issue. https://tbpl.mozilla.org/?tree=Try&rev=817ced5b7838

Clarification: this is a timing issue I cannot reproduce locally. =(

I think this might fix it. https://tbpl.mozilla.org/?tree=Try&rev=095a3a0fd065

Comment on attachment 8343435 [details] [diff] [review] promise-style.patch If you two fine people would be able to review this, I would be most appreciative! harth for styleeditor changes dcamp for styleinspector changes

There's no comment on this bug, what's the reason for these changes?

(In reply to Heather Arthur [:harth] from comment #7) > There's no comment on this bug, what's the reason for these changes? Most of the discussion is in the meta bug (bug 881050). This converts from the addon-sdk's Promise implementation to Promise.jsm. The difference is that the addon-sdk resolves promises synchronously when it can. Promise.jsm (and every other Promise library) force asynchronous resolution. The difference can be seen in the following example: > console.log(1); > promise.resolve().then(() => console.log(2)); > console.log(3); The addon-sdk Promise implementation will print 1, 2, 3. Promise.jsm (and DOM Promises) will print 1, 3, 2. The changes in this patch are fixes for race conditions where the synchronous resolution was implicitly being depended on. The general pattern is: > someSyncPromise.then(() => { > /* do important stuff here */ > }); > this.emit("something-just-happened"); For addon-sdk Promises, the stuff inside the .then callback would be executed before the event is emitted. With Promise.jsm the event is emitted *before* the callback is executed, so the object is an unexpected state when the event is emitted.

Comment on attachment 8343435 [details] [diff] [review] promise-style.patch Review of attachment 8343435 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the explanation. I'm still wondering about the necessity for the other Style Editor changes: ::: browser/devtools/styleeditor/StyleEditorUI.jsm @@ +322,5 @@ > + editorPromise = promise.resolve(); > + } > + > + editorPromise.then(() => { > + this.emit("editor-added", editor); I'm still a bit confused about these editor selection changes. The previous code didn't touch promises at all. So why the changes? ::: browser/devtools/styleeditor/StyleSheetEditor.jsm @@ -233,5 @@ > > this.emit("source-editor-load"); > }); > - > - sourceEditor.on("change", this._onPropertyChange); Heads up: this is gonna conflict with a recent landing in bug 935176

(In reply to Heather Arthur [:harth] from comment #9) > Thanks for the explanation. I'm still wondering about the necessity for the > other Style Editor changes: > > ::: browser/devtools/styleeditor/StyleEditorUI.jsm > @@ +322,5 @@ > > + editorPromise = promise.resolve(); > > + } > > + > > + editorPromise.then(() => { > > + this.emit("editor-added", editor); > > I'm still a bit confused about these editor selection changes. The previous > code didn't touch promises at all. So why the changes? It does actually involve promises now since we changed to CodeMirror; initialization of an Editor (specifically Editor#appendTo which creates a new iframe and attaches it to a parent node) involves a promise. StyleEditorUI#_selectEditor (through StyleSheetEditor#getSourceEditor) returns these promises. The "editor-added" event shouldn't be emitted before these promises resolve or else there's a race condition where listeners may be called before the editor has finished initializing. It's an intermittent failure when I test locally and a perma-orange on try-server (presumably due to the computers being slower).

Comment on attachment 8343435 [details] [diff] [review] promise-style.patch (In reply to Brandon Benvie [:benvie] from comment #10) > The "editor-added" event shouldn't be emitted before these promises resolve > or else there's a race condition where listeners may be called before the > editor has finished initializing. It's an intermittent failure when I test > locally and a perma-orange on try-server (presumably due to the computers > being slower). Thanks for explaining. Is there a bug for that orange?

It's not a current intermittent. I meant intermittent failures on try.

Rebases to recent landings. I just want to make sure I didn't miss anything: https://tbpl.mozilla.org/?tree=Try&rev=c72faa5242f9.

Let's try again. https://tbpl.mozilla.org/?tree=Try&rev=c50315b7a10c

I think this will fix it. https://tbpl.mozilla.org/?tree=Try&rev=c5cb751f7cbe

I see that is orange. I encountered a race condition when running the tests locally when I fixing another bug. It might have been the same issue. Can you see if the patch in bug 949678 fixes your problem?

Rebase.

This patch for the style inspector and editor still shows failures: https://tbpl.mozilla.org/?tree=Try&rev=78a173f49592

This is another strange failure. If you apply this updated patch and run: ./mach mochitest-browser browser/devtools/styleeditor/test/browser_styleeditor_autocomplete.js You will see that, at the point where the test stops, there are two views on the same source file in the style editor (looking like a sort of vertical split view). This is curious and may be a new race condition in the actual view initialization code, or maybe I'm doing something wrong with the new test functions. Ideas?

Can it be because of : + editor.load(inputElement); + yield editor.load(inputElement); two calls ?

(In reply to Girish Sharma [:Optimizer] from comment #20) > Can it be because of : > > + editor.load(inputElement); > + yield editor.load(inputElement); > > two calls ? Yes that was it, good catch! Thanks!

I updated and simplified the tests as needed, they pass locally. Tryserver build: https://tbpl.mozilla.org/?tree=Try&rev=373dc0c4fd84

Comment on attachment 8394818 [details] [diff] [review] The patch Review of attachment 8394818 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. I rebased it (one line didn't apply) and I got a test failure locally the first time I ran it (related orange: bug 969111). But it passes now so fingers crossed.

The following changeset is now in Firefox Nightly: > 1c9236abe50f Bug 943621 - Convert to Promise.jsm in style inspector and style editor. r=dcamp, r=harth Nightly Build Information: ID: 20140402030201 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central Download Links: > Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2 > Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2 > Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2 > Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg > Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe > Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe Previous Nightly Build Information: ID: 20140401030203 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

Paolo, can you please request Aurora approval for this patch when you get a minute? It's needed for me to be able to land some other test cleanups needed for enabling mochitest-bc chunking and the devtools split there. Thanks!

Comment on attachment 8394818 [details] [diff] [review] The patch (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27) > Paolo, can you please request Aurora approval for this patch when you get a > minute? It's needed for me to be able to land some other test cleanups > needed for enabling mochitest-bc chunking and the devtools split there. > Thanks! [Approval Request Comment] Bug caused by (feature/regressing bug #): N/A User impact if declined: None, required for test infrastructure cleanup Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): We changed the code that opens the styleeditor, I think that any glitches we could have introduced would have been detected in Nightly by now. String or IDL/UUID changes made by this patch: None