Closed Bug 1441531 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ochameau, Assigned: yulia)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1441531 - Make waitForMultipleChildrenUpdates an async function
59 bytes, text/x-review-board-request
Details 
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: 6 years ago
status-firefox60: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: