Use DOM Promise scheduling in Promise.jsm

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

Trunk
mozilla36
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 856878
No longer blocks: jsdownloads
Assignee

Updated

5 years ago
Depends on: 1094248
Assignee

Comment 2

5 years ago
Posted 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+
Assignee

Updated

5 years ago
Depends on: 1095443
Assignee

Comment 4

5 years ago
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
Status: NEW → ASSIGNED
Attachment #8518873 - Flags: review?(mconley)
Assignee

Comment 5

5 years ago
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)
Assignee

Comment 6

5 years ago
Attachment #8517512 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Iteration: --- → 36.2
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Assignee

Updated

5 years ago
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+
Assignee

Updated

5 years ago
Blocks: 1098255
Assignee

Comment 10

5 years ago
(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.
Assignee

Comment 11

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/7a19f76e912e
https://hg.mozilla.org/mozilla-central/rev/d187d7f83b90
https://hg.mozilla.org/mozilla-central/rev/fd4bd4b3fbf8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Updated

5 years ago
Depends on: 1099095
Depends on: 1128747
You need to log in before you can comment on or make changes to this bug.