943621 - Convert to Promise.jsm in style inspector and style editor

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P2)

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-style.patch

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

Comment 2

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=ed4336777694

Comment 3

[Brandon Benvie [:benvie]]

Attachment: promise-style.patch

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

Comment 4

[Brandon Benvie [:benvie]]

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

Comment 5

[Brandon Benvie [:benvie]]

Attachment: promise-style.patch

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

Comment 6

[Brandon Benvie [:benvie]]

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

Comment 7

[Heather Arthur [:harth]]

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

Comment 8

[Brandon Benvie [:benvie]]

(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 9

[Heather Arthur [:harth]]

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

Comment 10

[Brandon Benvie [:benvie]]

(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 11

[Heather Arthur [:harth]]

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?

Comment 12

[Brandon Benvie [:benvie]]

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

Comment 13

[Brandon Benvie [:benvie]]

Attachment: promise-style.patch

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

Comment 14

[Brandon Benvie [:benvie]]

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

Comment 15

[Brandon Benvie [:benvie]]

Attachment: promise-style.patch

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

Comment 16

[Heather Arthur [:harth]]

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?

Comment 17

[Brandon Benvie [:benvie]]

Attachment: promise-style.patch

Rebase.

Comment 18

[:Paolo Amadini]

This patch for the style inspector and editor still shows failures:

https://tbpl.mozilla.org/?tree=Try&rev=78a173f49592

Comment 19

[:Paolo Amadini]

Attachment: Updated patch with failing tests

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?

Comment 20

[Girish Sharma [:Optimizer]]

Can it be because of :

+            editor.load(inputElement);
+            yield editor.load(inputElement);

two calls ?

Comment 21

[:Paolo Amadini]

(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!

Comment 22

[:Paolo Amadini]

Attachment: The patch

I updated and simplified the tests as needed, they pass locally. Tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=373dc0c4fd84

Comment 23

[Heather Arthur [:harth]]

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.

Comment 24

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/1c9236abe50f

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c9236abe50f

Comment 26

[Firefox Commit Bot]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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 28

[:Paolo Amadini]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68ee63a9ad18