1094208 - Use DOM Promise scheduling in Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[:Paolo Amadini]

Now that bug 1013625 is fixed, we could use Promise.resolve().then() instead of a runnable to schedule our resolution loop, even before switching to the full DOM Promise implementation as a back-end for Promise.jsm.

Comment 1

[:Paolo Amadini]

Attachment: The patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b3a60103d464

Comment 2

[:Paolo Amadini]

Attachment: The patch

Attached the patch for bug 1094248 instead of bug 1094208.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8517512 [details] [diff] [review]
The patch

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

Looks good, thanks.

::: toolkit/modules/Promise-backend.js
@@ +687,5 @@
>     */
>    scheduleWalkerLoop: function()
>    {
>      this.walkerLoopScheduled = true;
> +    DOMPromise.resolve().then(this.walkerLoop.bind(this));

Apparently, `() =>` is more efficient than `bind`, both in terms of speed and memory, so could you use it?

Comment 4

[:Paolo Amadini]

Attachment: Part 1 of 3 - Fix browser_restore_isAppTab.js to work with the DOM Promise scheduling

Mike, I don't see why the new Promise scheduling would cause this test not to detect the correct value of isAppTab. I can only guess that same-process messages may be reordered either on the sending or receiving side, which is what I'm observing by using "dump" in content scripts.

This seems to happen only if the messages are posted in the same event loop tick, so I've added a wait of one tick to ensure ordering.

Comment 5

[:Paolo Amadini]

Attachment: Part 2 of 3 - Fix browser_bug562797.js to work with the DOM Promise scheduling

Dave, this test is already fragile and makes use of a lot of executeSoon calls already, so I'm just adding a few more to accommodate the new DOM Promise scheduling.

The Promise scheduling in relation to other events is deterministic: the test always fails without these calls, and always succeeds if this minimum amount of calls is added. Unfortunately this is the only approach that has taken a reasonable amount of time, a rewrite of the test to make less assumptions on scheduling would be much more work.

Comment 6

[:Paolo Amadini]

Attachment: Part 3 of 3 - Use DOM Promise scheduling in Promise.jsm

Comment 7

[:Paolo Amadini]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b6b8172550ad

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8518873 [details] [diff] [review]
Part 1 of 3 - Fix browser_restore_isAppTab.js to work with the DOM Promise scheduling

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

That's... really weird. But if this fixes some test-only code, I guess I won't sweat it too much...

It might be a good idea to file a separate bug for this message re-ordering stuff and provide a minimal test-case (if you have the cycles) - since that's pretty unexpected.

Thanks for the patch!

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/7a19f76e912e
https://hg.mozilla.org/integration/fx-team/rev/d187d7f83b90
https://hg.mozilla.org/integration/fx-team/rev/fd4bd4b3fbf8

Comment 10

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> It might be a good idea to file a separate bug for this message re-ordering
> stuff and provide a minimal test-case (if you have the cycles) - since
> that's pretty unexpected.

I filed bug 1098255 for this.

Comment 11

[:Paolo Amadini]

Hm, today I see some failed e10s tests from the try build in comment 7, that probably at the time weren't visible on try or were slow to complete:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b6b8172550ad

Since I already landed this in fx-team, let's see how this goes, and if the failures are still there I'll investigate after the backout.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a19f76e912e
https://hg.mozilla.org/mozilla-central/rev/d187d7f83b90
https://hg.mozilla.org/mozilla-central/rev/fd4bd4b3fbf8