Closed Bug 1094208 Opened 10 years ago Closed 10 years ago

Use DOM Promise scheduling in Promise.jsm


(Toolkit :: Async Tooling, defect)

Not set





(Reporter: Paolo, Assigned: Paolo)


(Blocks 1 open bug)



(3 files, 2 obsolete files)

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.
Blocks: 856878
No longer blocks: jsdownloads
Depends on: 1094248
Attached patch The patch (obsolete) — Splinter Review
Attached the patch for bug 1094248 instead of bug 1094208.
Attachment #8517509 - Attachment is obsolete: true
Attachment #8517509 - Flags: review?(dteller)
Attachment #8517512 - Flags: review?(dteller)
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?
Attachment #8517512 - Flags: review?(dteller) → review+
Depends on: 1095443
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.
Assignee: nobody → paolo.mozmail
Attachment #8518873 - Flags: review?(mconley)
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.
Attachment #8518875 - Flags: review?(dtownsend+bugmail)
Attachment #8517512 - Attachment is obsolete: true
Iteration: --- → 36.2
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Blocks: 1030075
Iteration: 36.2 → 36.3
Attachment #8518875 - Flags: review?(dtownsend+bugmail) → review+
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!
Attachment #8518873 - Flags: review?(mconley) → review+
Blocks: 1098255
(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.
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:

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.
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1099095
Depends on: 1128747
You need to log in before you can comment on or make changes to this bug.