1094248 - Process remaining Promise microtasks before blocking inside a nested event loop in the main thread

Status: RESOLVED FIXED

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

Description

[:Paolo Amadini]

Attachment: The patch

When a nested event loop is entered on the main thread using a processNextEvent call that can block, we should process any pending Promise microtasks eagerly to avoid a potential deadlock.

The xpcshell test for this is activated by bug 1094208 and is here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_Promise.js#906

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 2

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/4d6d645e6d51

Comment 3

[:Paolo Amadini]

Hm, so this isn't the full story. This common case still causes a deadlock:

let finished = false;
Promise.resolve().then(() => { finished = true; });
while (!finished) {
  thread.processNextEvent(true);
}

This happens because, even if the "then" callback is processed, we already checked the "while" condition, and processNextEvent still blocks. This could be worked around in the callback, that is now properly called thanks to the OnProcessNextEvent change:

Promise.resolve().then(() => {
  // Post an event to unlock processNextEvent.
  thread.dispatch(() => { finished = true; }, Ci.nsIThread.DISPATCH_NORMAL);
});

However, this is not quite obvious, so even if we fix all current callers we may still see new "incorrect" loops introduced later.

Possible better solutions in OnProcessNextEvent:
- Post a dummy event if we processed any Promise microtask, to unblock ProcessNextEvent
- Introduce a new API to force ProcessNextEvent not to block, and use it we processed any Promise microtask or have any Promise microtask pending
- Don't process Promise microtasks in OnProcessNextEvent, only post a dummy event to force the processing (this has a slightly different interaction with setTimeout, but shouldn't matter much for nested event loops).

Opinions?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Why do we have people manually doing processNextEvent(true) calls?  :(

> - Post a dummy event if we processed any Promise microtask, to unblock ProcessNextEvent

At first glance I like this one, as long as we think it through carefully to make sure it won't end up ping-ponging the event loop unnecessarily (that is, not blocking for a long time when we really should block).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Microtasks aren't supposed to be processed before some next task.

window.onload = function() {
  var mo = new MutationObserver(function(records) {});
  mo.observe(document, {subtree: true, childList: true});
  // mutate document
  var xhr = new XMLHttpRequest();
  xhr.open("GET", location.href, false); // sync
  xhr.send(); // We don't want mutation observer callback to be called before send();
  ... since it should be called when load event handler has been called.
}

Comment 6

[:Paolo Amadini]

Hm, I don't see any great way to prevent this code from using maximum CPU without activating the slow script warning (though still responding to events):

condition.then(() => { finished = true; });
while (!finished) {
  thread.processNextEvent(true);
  Promise.resolve().then(() => {});
}

After all, it's similar to:

Promise.resolve().then(() => { finished = true; });
while (!finished) {
  thread.processNextEvent(false);
}

But far less obvious and can be triggered by external code. So, do we care?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Microtasks aren't supposed to be processed before some next task.
Well, that is a bit geckoism actually, since we have these sync things which actually spin event loop.
Per spec "Perform a microtask checkpoint" is there in "spin the event loop".

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Microtasks aren't supposed to be processed before some next task.

Promises don't run off microtasks per spec.  They run off ES jobs.  The real question is how ES jobs interact with both your example and the processNextEvent example.

I'm trying to understand the examples in comment 6.  For the first example, why is the Promise.resolve() call even reached?  Is it because there is already an event in the queue when we enter the loop?  But in all honesty, I don't care too much about the behavior in the first case there, because it seems like a bug in the code.

When you say "triggered by external code" do you mean web pages or extensions?

Comment 9

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #8)
> I'm trying to understand the examples in comment 6.

Ah, a last-minute edit made the example bogus :-) It should look like this:

condition.then(() => { finished = true; });
Promise.resolve().then(() => {});
while (!finished) {
  thread.processNextEvent(true);
  Promise.resolve().then(() => {});
}

> When you say "triggered by external code" do you mean web pages or
> extensions?

Both of them could have an infinite loop that would normally trigger the slow script warning, and may interact with this nested loop in some way that prevents it from happening. But thinking through it, I can't find an example that is worse than a simple recursive setTimeout(0).

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4d6d645e6d51

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to :Paolo Amadini from comment #3)
> - Introduce a new API to force ProcessNextEvent not to block, and use it we
> processed any Promise microtask or have any Promise microtask pending

Why didn't we just do this?  You just call ProcessNextEvent(false) and it doesn't block.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://people.mozilla.org/~khuey/promise-test.html is why I don't like this, btw.  This behavior is exposed to web content.

Comment 13

[:Paolo Amadini]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #11)
> (In reply to :Paolo Amadini from comment #3)
> > - Introduce a new API to force ProcessNextEvent not to block, and use it we
> > processed any Promise microtask or have any Promise microtask pending
> 
> Why didn't we just do this?  You just call ProcessNextEvent(false) and it
> doesn't block.

Not sure I understand the question. We want to support the example in comment 9, since we have many examples in tree and in add-ons of places using thread.processNextEvent(true) with that pattern, and changing them is unrealistic. The deadlock with processNextEvent(true) would also be unreliable to reproduce, as any unrelated event may break it.

One way to support that case, instead of processing microtasks eagerly, can be to introduce an API called every time we schedule a microtask, that makes it so that the next call to processNextEvent would not block, even if it was called with aMayWait set to true. It's an alternative to the solution implemented here.

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> http://people.mozilla.org/~khuey/promise-test.html is why I don't like this,
> btw.  This behavior is exposed to web content.

I don't know enough of the microtask scheduling specification to know whether this behavior is expected or not.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

HTML now specifies that nested event loops don't run microtasks at all, so everything that does processNextEvent to wait for a promise is going to have to be changed.

Comment 15

[:Paolo Amadini]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #14)
> HTML now specifies that nested event loops don't run microtasks at all

Well, maybe the specification is referring to something different with "nested event loop"? In our case, a nested event loop is something that could be triggered at the OS level by a message box, or that we would spin when displaying an in-content modal prompt ("alert"), or during AsyncShutdown processing.

> so
> everything that does processNextEvent to wait for a promise is going to have
> to be changed.

Not running microtasks in that case would mean any code using Promises would stop working, it's not necessary for the loop to wait on a Promise resolution... so I guess we're talking about something different here.