browser_markup_load_01.js fails when converting waitForMultipleChildrenUpdates from Task.jsm to async/await

RESOLVED FIXED in Firefox 60

Status

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: ochameau, Assigned: yulia)

Tracking

unspecified
Firefox 60
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Here is an easy way to reproduce the exception:

$ ./mach mochitest devtools/client/inspector/markup/test/browser_markup_load_01.js

TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_load_01.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/test/head.js:537 - TypeError: inspector.markup is null
Stack trace:
    waitForMultipleChildrenUpdates<@chrome://mochitests/content/browser/devtools/client/inspector/test/head.js:537:1
    _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
    TaskImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:272:3
    asyncFunction@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:246:14
    @chrome://mochitests/content/browser/devtools/client/inspector/markup/test/browser_markup_load_01.js:52:33
    Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1067:21
    Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:958:9
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

And a try run highlighting the failures with this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2686a2beaf3163577ac884347c5b646e28dcc8a6&selectedJob=164613113

Be careful about bug 1441528, waitForMultipleChildrenUpdates is duplicated right now, but browser_markup_load_01.js ends up using the one from devtools/client/inspector/test/head.js
Assignee: nobody → ystartsev
Comment on attachment 8954691 [details]
Bug 1441531 - Convert waitForMultipleChildrenUpdates from Task.jsm to async/await.

https://reviewboard.mozilla.org/r/223806/#review229824


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/test/head.js:542
(Diff revision 1)
>    // As long as child updates are queued up while we wait for an update already
>    // wait again
>    if (inspector.markup._queuedChildUpdates &&
>          inspector.markup._queuedChildUpdates.size) {
> -    yield waitForChildrenUpdated(inspector);
> -    return yield waitForMultipleChildrenUpdates(inspector);
> +    await waitForChildrenUpdated(inspector);
> +    return await waitForMultipleChildrenUpdates(inspector);

Error: Redundant use of `await` on a return value. [eslint: no-return-await]
It looks like what is happening here is that task.jsm is lazy and async/await is eager. This also sheds some light on why we might have had issues in other task.jsm replacements. We can see that here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/test/browser_markup_load_01.js#52-61

>  let multipleChildrenUpdates = waitForMultipleChildrenUpdates(inspector);
>  yield chooseWithInspectElementContextMenu("img", tab);
>
>  info("Wait for load");
>  yield pageLoaded;
>
>  info("Wait for markup-loaded after element inspection");
>  yield markupLoaded;
>  info("Wait for multiple children updates after element inspection");
>  yield multipleChildrenUpdates;

In the previous version of the code, waitForMultipleChildrenUpdates would be setup on line 52 (the first line above), but it would not execute anything until line 61 (the last line). We can see this because `inspector.markup` which is used in waitForMultipleChildrenUpdates is null before `yield markupLoaded` completes. Given that this is the case, we might want to remove line 52, and replace line 61 with `yield waitForMultipleChildrenUpdates(inspector)` and it would be equivalent
Attachment #8954702 - Flags: review?(poirot.alex)
Blocks: 1442153
Comment on attachment 8954702 [details]
Bug 1441531 - Make waitForMultipleChildrenUpdates an async function

Looks good to me but I would like feedback from an inspector expert :)
Attachment #8954702 - Flags: review?(poirot.alex) → review?(pbrosset)
Comment on attachment 8954702 [details]
Bug 1441531 - Make waitForMultipleChildrenUpdates an async function

https://reviewboard.mozilla.org/r/223822/#review232330

::: commit-message-52975:1
(Diff revision 2)
> +Bug 1441531 - browser_markup_load_01.js fails when converting waitForMultipleChildrenUpdates from Task.jsm to async/await

Can you please rephrase so it summarizes what you changed rather than just re-states the title of the bug?
The title of the bug usually says what the problem is (often from the user's standpoint), while a commit message (and there can be several per bug) says what was changed in the code within this commit.

I suggest "Make waitForMultipleChildrenUpdates an async function"

And if you want to say more about the test changes, you can always add more description on new lines below that.
Attachment #8954702 - Flags: review?(pbrosset) → review+
Attachment #8954691 - Attachment is obsolete: true
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/112a94492fee
Make waitForMultipleChildrenUpdates an async function r=pbro
https://hg.mozilla.org/mozilla-central/rev/112a94492fee
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.