Closed Bug 1162013 Opened 4 years ago Closed 4 years ago

Microtask scheduled via Promise should run right at the end of the current setTimeout handler even if other setTimeout timers have also expired

Categories

(Core :: DOM: Core & HTML, defect)

37 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: victor, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached file mit-2.html
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2383.0 Safari/537.36

Steps to reproduce:

The attached file:
- run some code which enqueues a microtask ("root.mit"),
- enqeue 2 macrotasks ("mat1" & "mat2"),
- the first macrotask enqueues a microtask ("mat1.mit")

In order to enqueu a microtask, a resolved promise .then() is used, see the scheduleMicrotask function.

Firefox version: 37.0.2 (Ubuntu)


Actual results:

The microtask enqueued in the first setTimeout is not executed at the end of the macrotask but at a later point in time (even after the second setTiemout is executed).

Console log:
- "+root, -root, root.mit, +mat1, -mat1, mat2"
- "microtask" -> the microtask is executed but much after where it should have been


Expected results:

- the root code executes (a macrotask) -> "+root, - root"
- the microtask enqueued by the root code executes -> "root.mit"
- the first setTimeout executes (macrotask) -> "+mat1, -mat1"
- the microtask executes -> "mat1.mit",
- the second setTimeout executes -> "mat2"

The result should be "+root, -root, root.mit, +mat1, -mat1, mat1.mit, mat2"
Flags: needinfo?(dteller)
Product: Firefox → Core
Component: Untriaged → DOM
(Of course this is not actually defined in any spec.)
The reason this is the same across browsers is because there is no specification for how this should work:

  https://www.w3.org/Bugs/Public/show_bug.cgi?id=25981
Blocks: 885333
I don't get all the technical details but it seems like no spec defined that Promise jobs should be prioritized over Script jobs, is that right ?

If that is right it means that this is not a bug and will not change ?

Side question: I've switched to MutationObserver to enqueue microtasks, they behave as *I* would expect (as described in the "Expected results" section of the initial report). Do that mean that it could also change in the future ?

Many thanks for your help
The behavior will probably change, especially if Promises try to use microtasks, but yet behave
differently to MutationObservers.
Hopefully dteller or paolo could debug this.
(And sorry, I thought dteller fixed Bug 1013625, but it was paolo)
Flags: needinfo?(paolo.mozmail)
I don't think there's any need to debug this; it's clear what's going on.

Right now we execute promises from the main event loop, but with higher priority than normal events.  Basically, its structure looks about like so:

  while (true) {
    runPromises();
    runOneEvent();
  }

(there are some complications around whether runOneEvent can block, but we're not worried about those here).

Note that this is different from how mutation observers are handled: those can run within runOneEvent(), if runOneEvent() enters/exits script multiple times.  See more below on why we don't do that with Promise.

OK, so what happens in this testcase?  We queue up two timeouts.  Execution of the first timeout queues up some promise stuff.  But in our setTimeout implementation, when a timer for any setTimeout for a window fires we go ahead and run all the timeouts that have expired for that window.  The testcase sets up two timers, but they have the same firing time (because the resolution of the timers is not high enough to notice the difference in execution time from first to second setTimeout call), so we run both setTimeout callbacks off a single event loop event.  So no promise callback processing happens between them.

I _think_ the HTML spec expects each setTimeout to get a separate event in the event queue, but it also expects the sort of serialization across them that we get now by ensuring earlier timers also fire if a timer is firing.

There are various options here:

1)  Change nsGlobalWindow::RunTimeout to stop at aTimeout when looking for last_expired_timeout.  That would ensure ordering doesn't break (and still end up running multiple timeouts in a single event if the timer implementation does some sort of out-of-order notification), but would ensure that in this testcase the second setTimeout runs from a separate event loop event.

2)  Change setTimeout stuff to ensure that all timers are run off separate tasks.  That ensures having more trust in the OS timers (nsITimer) firing in order than I'm willing to grant, I think.

3)  Change when we Promise::PerformMicroTaskCheckpoint to do it between firing of the setTimeout handlers.  

4)  Change Promise callbacks to run off actual microtasks.... except we tried that and it's broken because promise callbacks themselves are microtasks, so we get all sorts of mis-ordering of promise stuff if promises _actually_ run off microtasks.

I suspect #3 is the simplest and most principled solution for the moment.  #1 would have some nice properties like not walking the entire timeout list every time (!).  Maybe we should in fact do both.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(dteller)
Summary: Microtask scheduled via Promise should run right at the end of the current macrotask → Microtask scheduled via Promise should run right at the end of the current setTimeout handler even if other setTimeout timers have also expired
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I'm pretty sure 4 is the intent. Do you have an example of what went wrong?
Attachment #8602303 - Flags: review?(bugs) → review+
Hmm.  I guess we're doing the promise bits in nsGlobalWindow after we've unset our "running a timeout" state, so that could break things if the promise handlers mess with the timeout list.  I can reproduce the Moth issue, and it goes away if I move the Promise thing up a bit to right after we invoke the callback and before we've unset any of our timeout-running state.

Going to push that to try to see whether the dt3 issue is gone too.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/a455c8bce215
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.