Fix asynchronous initialization

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: sjakthol, Assigned: sjakthol)

Tracking

unspecified
Firefox 39
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

The initialization process of style editor starts all kinds of asynchronous operations without waiting for them to complete. This leads to intermittent test failures as the operations may complete in any order and tests generally expect them to happen in specific order.
This patch fixes a lot of races in the style editor initialization process leading to intermittent test failures.

Basically I started to follow the code from the StyleEditorPanel.open method and chained all asynchronous operations back to the promise returned by it.

Or to be more precise, the initialization Task completes after every stylesheet returned by getStyleSheets has a properly initialized editor. This should make it possible to write tests in a more simpler and race-proof way than the current "wait for x editors to be added and hope that we didn't miss any"

The changes are mostly just tagging the methods with Task.async, indentation changes due to removing .then handlers, moving code around and adding yields.

Eddy, I'm marking you for review as I remember you did some Promise work a while back dealing with similar situations.

Try with all of the patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d7c6bcdad9d
Linux-only with bunch of retriggers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=422a71e06f19
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8583644 - Flags: review?(ejpbruel)
This patch contains fixes to those style editor tests that were directly busted by the changes of part 1. Here's a quick run-down of the changes in the patch.

head.js:
Added a new method openStyleEditorForURL which will
  1) open a new tab with the given URL and wait for it to load
  2) open the style editor and wait for all stylesheets that were there initially
     to be fully loaded in the style editor.

I also edited the addTab method to to take optional window argument and moved checkDiskCacheFor out of head.js as it was only used in one test.

browser_styleeditor_private_perwindowpb.js:
Moved checkDiskCacheFor from head.js. Also imported waitForDelayedStartupFinished to replace executeSoon used in the load handler as it's much more reliable than hoping for the window be ready after a single tick.

browser_styleeditor_init.js:
Rewrote the whole test as it was ugly and doing all kinds of weird things. Basically, I combined the two test methods into a single generic one.

browser_styleeditor_media_sidebar_sourcemaps.js
browser_styleeditor_sourcemap_large.js
browser_styleeditor_sourcemap_watching.js:
Made these use the new initialization method as the old method addTabAndOpenStyleEditors doesn't get to see the temporary editors any more which are added before mapped sources are available[1] thus causing it to never reach its target of opened editors.

browser_styleeditor_sourcemaps.js:
Fixed the detection of added editors after pref change as it waited for three editors to added but asserted there to be four editors. The "stylesheets-reset" event it now relies on is emitted once the reset process is fully complete (i.e. the new editors are fully loaded and ready to be used).

browser_styleeditor_xul.js:
The editor-added event is emitted before showToolbox resolves thus this needs to be removed.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#272-280
Attachment #8583645 - Flags: review?(ejpbruel)
This patch fixes other devtools tests that relied on the previous functionality.

styleinspector:
A few tests check that links open in the style editor and the correct editor may or may not be selected by default. First of all I had to change the event "styleeditor-ready" to "styleeditor-selected" as the former triggers a little bit too early in the initialization process causing the test to continue a bit too early if the correct editor was already selected. If the selected editor is not the expected, the method waits for "editor-selected" event before continuing.

browser_webconsole_bug_782653_CSS_links_in_Style_Editor.js:
When "styleeditor-selected" is received all of the editors are already added thus the test shouldn't wait for them to be added.
Attachment #8583648 - Flags: review?(ejpbruel)
Comment on attachment 8583644 [details] [diff] [review]
style-editor-async-init-part-1.patch

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

This patch looks good. I like that you're replacing promises with Tasks, which are much more readable.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +207,5 @@
>     *        Event name
>     * @param {StyleSheet} styleSheet
>     *        StyleSheet object for new sheet
>     */
>    _onNewDocument: function() {

Why not use yield here as well (as you do in initialize)?

@@ +321,5 @@
>      editor.on("linked-css-file-error", this._summaryChange.bind(this, editor));
>      editor.on("error", this._onError);
>  
>      this.editors.push(editor);
>  

If fetchSource returns a promise, you should be able to use yield here as well.
Attachment #8583644 - Flags: review?(ejpbruel) → review+
Thanks for the quick review.

(In reply to Eddy Bruel [:ejpbruel] from comment #4)
> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +207,5 @@
> >     *        Event name
> >     * @param {StyleSheet} styleSheet
> >     *        StyleSheet object for new sheet
> >     */
> >    _onNewDocument: function() {
> 
> Why not use yield here as well (as you do in initialize)?
Because it's only called as an event handler.

I don't know if there's a way to catch rejections inside the function body when using Task.async (other than using .then in the yielded promise). Another solution would've been do "return Task.spawn(function* () { ... }).then(null, Cu.reportError)" but that wouldn't have been any clearer than the current version.

> @@ +321,5 @@
> >      editor.on("linked-css-file-error", this._summaryChange.bind(this, editor));
> >      editor.on("error", this._onError);
> >  
> >      this.editors.push(editor);
> >  
> 
> If fetchSource returns a promise, you should be able to use yield here as
> well.

Makes sense. Changed.
Attachment #8583644 - Attachment is obsolete: true
Attachment #8583765 - Flags: review+
Comment on attachment 8583645 [details] [diff] [review]
style-editor-async-init-part-2-tests.patch

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

Patch looks good overall. I just have a couple of questions. If you can answer those, feel free to land.

::: browser/devtools/styleeditor/test/browser_styleeditor_private_perwindowpb.js
@@ +22,1 @@
>  

This works because we are now waiting for _sourceLoaded to finish before completing initialization, right?

@@ +60,2 @@
>  }
> +

This is great stuff. It's much better to wait for a specific event instead of relying on executeSoon.

::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
@@ +115,5 @@
>  
>  /* Helpers */
>  
>  function togglePref(UI) {
> +  let editorsPromise = UI.once("stylesheets-reset");

I'm not 100% sure why we use stylesheets-reset here?

::: browser/devtools/styleeditor/test/head.js
@@ -41,3 @@
>      info("URL '" + url + "' loading complete");
>      def.resolve(tab);
>    }, true);

I assume you removed this line because you can pass an initial url for the tab to gBrowser.addTab. Make sure you test this with e10s enabled.

@@ +78,5 @@
> + */
> +let openStyleEditorForURL = Task.async(function* (url, win) {
> +  let tab = yield addTab(url, win);
> +  let target = TargetFactory.forTab(tab);
> +  let toolbox = yield gDevTools.showToolbox(target, "styleeditor");

I assume this waits for all stylesheets that were there initially to be fully loaded in the style editor because of the changes you made in the previous patch, yes?
Attachment #8583645 - Flags: review?(ejpbruel) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> Comment on attachment 8583645 [details] [diff] [review]
> style-editor-async-init-part-2-tests.patch
> 
> Review of attachment 8583645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good overall. I just have a couple of questions. If you can
> answer those, feel free to land.
> 
> :::
> browser/devtools/styleeditor/test/browser_styleeditor_private_perwindowpb.js
> @@ +22,1 @@
> >  
> 
> This works because we are now waiting for _sourceLoaded to finish before
> completing initialization, right?
I'm not entirely sure which part of the test you're trying to point at but the general flow of the test goes like:
1. Open style editor and wait for the UI to initialize (see below). At this point there will be exactly one style sheet in the UI and it will have an editor (but not the actual source editor) and an item in the list of sheets.
2. Wait for the source editor to load (i.e. yield editor.getSourceEditor()). This'll make sure that the actual source editor is loaded and ready. Actually, I'm not sure if this is needed anymore as the source will be loaded before we start the test but I don't dare to remove it as it doesn't do any harm.
3. Check the cache and end the test.

> ::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
> @@ +115,5 @@
> >  
> >  /* Helpers */
> >  
> >  function togglePref(UI) {
> > +  let editorsPromise = UI.once("stylesheets-reset");
> 
> I'm not 100% sure why we use stylesheets-reset here?
The event means "all the old editors have been deleted and the new sheets are ready to be used" and that's exactly what we want after a pref is changed. Here's what happens when pref changes
1. _onNewDocument is called as the event handler
2. _onNewDocument it fetches the StyleSheetActors from the server and calls _resetStyleSheetList
3. _resetStyleSheetList removes the old editors and adds new ones by calling _addStyleSheet
4. _addStyleSheet fetches the source mapped sources (depending on the pref) and creates editors for them
5. _resetStyleSheetList emits stylesheets-reset after all new editors are added.


> ::: browser/devtools/styleeditor/test/head.js
> @@ -41,3 @@
> >      info("URL '" + url + "' loading complete");
> >      def.resolve(tab);
> >    }, true);
> 
> I assume you removed this line because you can pass an initial url for the
> tab to gBrowser.addTab. Make sure you test this with e10s enabled.
I added support for specifying the window which the tab should be opened thus doing content.location = url doesn't do much good if the tab we're trying to navigate is in a different window. I actually took this code from debugger head.js and if they work with e10s so should this.

> @@ +78,5 @@
> > + */
> > +let openStyleEditorForURL = Task.async(function* (url, win) {
> > +  let tab = yield addTab(url, win);
> > +  let target = TargetFactory.forTab(tab);
> > +  let toolbox = yield gDevTools.showToolbox(target, "styleeditor");
> 
> I assume this waits for all stylesheets that were there initially to be
> fully loaded in the style editor because of the changes you made in the
> previous patch, yes?
Yes, that was the point of the previous patch. A promise returned by the panel .open method will block the showToolbox until it is resolved.
Comment on attachment 8583648 [details] [diff] [review]
style-editor-async-init-part-3-other-tests.patch

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

LGTM
Attachment #8583648 - Flags: review?(ejpbruel) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/60a108e6feb8
https://hg.mozilla.org/mozilla-central/rev/34fab46bd28a
https://hg.mozilla.org/mozilla-central/rev/01a988be408b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Depends on: 1151381
Duplicate of this bug: 1046046
Duplicate of this bug: 1050990
Duplicate of this bug: 1102041
Duplicate of this bug: 1046308
Depends on: 1170864
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.