Closed Bug 943621 Opened 6 years ago Closed 6 years ago

Convert to Promise.jsm in style inspector and style editor

Categories

(DevTools :: Style Editor, defect, P2)

defect

Tracking

(firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: bbenvie, Assigned: Paolo)

References

Details

Attachments

(1 file, 7 obsolete files)

No description provided.
Attached patch promise-style.patch (obsolete) — Splinter Review
This patch switches the style editor and style inspector to using Promise.jsm.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Attached patch promise-style.patch (obsolete) — Splinter Review
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
Attachment #8338855 - Attachment is obsolete: true
Clarification: this is a timing issue I cannot reproduce locally. =(
Attached patch promise-style.patch (obsolete) — Splinter Review
I think this might fix it. https://tbpl.mozilla.org/?tree=Try&rev=095a3a0fd065
Attachment #8339565 - Attachment is obsolete: true
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
Attachment #8343435 - Flags: review?(fayearthur)
Attachment #8343435 - Flags: review?(dcamp)
Attachment #8343435 - Flags: review?(dcamp) → review+
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?
Attachment #8343435 - Flags: review?(fayearthur) → review+
It's not a current intermittent. I meant intermittent failures on try.
Attached patch promise-style.patch (obsolete) — Splinter Review
Rebases to recent landings. I just want to make sure I didn't miss anything: https://tbpl.mozilla.org/?tree=Try&rev=c72faa5242f9.
Attachment #8343435 - Attachment is obsolete: true
Attachment #8346046 - Flags: review+
Attached patch promise-style.patch (obsolete) — Splinter Review
I think this will fix it. https://tbpl.mozilla.org/?tree=Try&rev=c5cb751f7cbe
Attachment #8346046 - Attachment is obsolete: true
Attachment #8347459 - Flags: review+
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?
Attached patch promise-style.patch (obsolete) — Splinter Review
Rebase.
Attachment #8347459 - Attachment is obsolete: true
Attachment #8357292 - Flags: review+
Depends on: 950565
This patch for the style inspector and editor still shows failures:

https://tbpl.mozilla.org/?tree=Try&rev=78a173f49592
Attached patch Updated patch with failing tests (obsolete) — Splinter Review
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?
Attachment #8357292 - Attachment is obsolete: true
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!
Attached patch The patchSplinter Review
I updated and simplified the tests as needed, they pass locally. Tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=373dc0c4fd84
Assignee: bbenvie → paolo.mozmail
Attachment #8394122 - Attachment is obsolete: true
Attachment #8394818 - Flags: review?(fayearthur)
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.
Attachment #8394818 - Flags: review?(fayearthur) → review+
https://hg.mozilla.org/mozilla-central/rev/1c9236abe50f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
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
Blocks: 996671
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!
Flags: needinfo?(paolo.mozmail)
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
Attachment #8394818 - Flags: approval-mozilla-aurora?
Flags: needinfo?(paolo.mozmail)
Attachment #8394818 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.