Intent to implement and ship: disallow message loop spinning APIs during microtask execution

271 views
Skip to first unread message

Jochen Eisinger

unread,
May 3, 2016, 10:48:00 AM5/3/16
to blink-dev

Contact emails

joc...@chromium.org


Spec

I'd claim that the current spec is contradictionary here, see summary:


Summary

Some APIs, namely, alert, prompt, confirm, print, and sync XHRs will spin the message loop.


On the other hand, microtasks are supposed to be executed at the end of the task that created them.

When a web page invokes one of the former APIs during microtask execution, we might end up executing regular tasks during microtask execution, e.g., resizing the browser window will fire resize events at the window.


It's also unclear whether microtasks created during those events should be executed at the end of the task corresponding to the event, or whether they should be enqueued at the end of the outer microtask queue.


I propose to plainly disallow those APIs during microtask execution.


Is this feature supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes


Demo link

layout tests included in https://codereview.chromium.org/1940253002


Debuggability

When we block an call, a console error is logged.


Interoperability and Compatibility Risk

I checked Firefox, and they don't block those calls during microtask execution, and suffer from the same issue I described above.


I asked on #whatwg and the general feeling of all three that answered was that the risk might be low :)


OTOH, I expect this to happen rarely in practice.


OWP launch tracking bug

crbug.com/605517


Entry on the feature dashboard

none yet.

Pavel Feldman

unread,
May 3, 2016, 10:56:05 AM5/3/16
to Jochen Eisinger, blink-dev
Debugger will still spin message loop within microtasks, for example when stepping through the promise code. Since the browser would need to behave in that case, it is unclear whether we can make any assumptions or simplify the code with this intent.

Jochen Eisinger

unread,
May 3, 2016, 10:57:04 AM5/3/16
to Pavel Feldman, blink-dev
I think it's ok if the debugger behaves differently in this case.

Boris Zbarsky

unread,
May 3, 2016, 11:18:37 AM5/3/16
to Jochen Eisinger, blink-dev
On 5/3/16 10:47 AM, Jochen Eisinger wrote:
> It's also unclear whether microtasks created during those events should
> be executed at the end of the task corresponding to the event, or
> whether they should be enqueued at the end of the outer microtask queue.

Per spec, the latter.

I agree that the use of these APIs during microtask execution is
problematic (even more so in Firefox than in Chrome, actually, because
our alert() is not app-modal). If you succeed (in terms of web compat)
in making this change, I would push for Gecko to make it as well.

It seems to me that the biggest compat risk is people doing alert()
debugging in Promise handlers....

-Boris

Pavel Feldman

unread,
May 3, 2016, 11:22:25 AM5/3/16
to Boris Zbarsky, Jochen Eisinger, blink-dev
As long as we don't assume that "microtasks don't run nested loops" in our code and produce a clear error message for this case, this sounds ok debugging-wise. But I was wondering if we could come up with a solution that debugger fits natively. Otherwise, you fix it for alert(), but resize would still dispatch as you step through promises.

Debugger already does a best effort at working around the problems like the one you describe. As other modal loops, it defers loaders and suspends active DOM objects. It also prevents input events from being dispatched. As a browser event, resize should have been suspended and never dispatched, so I wonder what went wrong there. Are there many more events that misbehave?

Pavel

Pavel Feldman

unread,
May 3, 2016, 11:24:46 AM5/3/16
to Boris Zbarsky, Jochen Eisinger, blink-dev
s/and never dispatched/and dispatched later/

Jochen Eisinger

unread,
May 3, 2016, 11:34:30 AM5/3/16
to Pavel Feldman, Boris Zbarsky, blink-dev

Sync XHRs will result in progress events being dispatched while the nested message loop runs.

Elliott Sprehn

unread,
May 3, 2016, 11:51:01 AM5/3/16
to Jochen Eisinger, blink-dev

I think this is very high risk, it means using any library that contains an alert will fail when used inside a promise callback, and nearly every new feature uses Promises. For example it would mean alert works in the event version of IDB, but is broken in the Promise version of the same api.

The print one in particular is going to be very confusing for developers, it doesn't really make sense that you can invoke it from an XHR callback but not a fetch Promise. Is there an async version of print?

This would also likely break apps written with a modern framework like Polymer, but that use any old style modal (usually inside some third party library wrapped in a component) since they schedule lots of work inside microtasks as a way to defer things.

I think we need to add a UseCounter for how often this would trigger before shipping it. It seems so easy to stumble into it by mistake, left hand doing the scheduling, right hand doing the UI in your app and then be broken.

Jochen Eisinger

unread,
May 3, 2016, 12:37:35 PM5/3/16
to Elliott Sprehn, blink-dev

I'm not sure I agree - I think the overlap you describe is quite rare. But anyways, adding a use counter, and preferably a deprecation message sounds fine as a first step

PhistucK

unread,
May 3, 2016, 12:44:10 PM5/3/16
to Jochen Eisinger, Elliott Sprehn, blink-dev
Since fetch is the new good XMLHttpRequest, it makes sense that whatever people do with one, they will do with both of them. People still use alert and enemies (:)) in those APIs, unfortunately. A use counter is a good first step, yep.


PhistucK

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

Rick Byers

unread,
May 3, 2016, 3:10:23 PM5/3/16
to PhistucK, Jochen Eisinger, Elliott Sprehn, blink-dev
Yeah we definitely need some compat data here before we can make a decision.  The main question in my mind is: do we add a deprecation right away or wait until we have hard data?  We know alert and friends aren't THAT common, so I'm OK with proactively adding a deprecation message.  But can you check the numbers during beta and if it's clearly too high revert before the message hits stable?

Jochen Eisinger

unread,
Jun 6, 2016, 3:47:22 PM6/6/16
to Rick Byers, PhistucK, Elliott Sprehn, blink-dev
First beta numbers are coming in, and they're virtually zero. Looking at the raw numbers, there's an order of magnitude difference in usage between prompt < confirm < print < alert < sync XHR.

One developer reacted to the console warning, but confirmed that it's easy to change their code to not use alert() inside of microtasks.

Should we continue with the plan to remove the feature in M53? Or would we rather wait for stable numbers (and push out the deprecation to M54 accordingly)?

Elliott, I'd be especially interested in hearing your opinion.

Elliott Sprehn

unread,
Jun 6, 2016, 3:54:47 PM6/6/16
to Jochen Eisinger, PhistucK, blink-dev, Rick Byers

How easy is it to revert? I'm fine letting it go all the way to beta as long as it's a tiny low risk revert to bring it back.

Jochen Eisinger

unread,
Jun 6, 2016, 4:27:22 PM6/6/16
to Elliott Sprehn, PhistucK, blink-dev, Rick Byers
I could make it a RuntimeEnabledFlags flip only for now.

Jochen Eisinger

unread,
Jun 8, 2016, 5:48:31 AM6/8/16
to blink-dev
Now that we have numbers from beta, I'd like to go ahead and ship this.

Philip Jägenstedt

unread,
Jun 8, 2016, 11:01:03 AM6/8/16
to Jochen Eisinger, blink-dev
Looks like these are the counters:

Sync XHR has the highest usage, already visible in the overall usage, and it looks like ~0.005% on the beta channel. That's something that developers will run into once in a while and need to debug. If they depend on sync XHR then presumably there might be some hurdle in making it async or even switching to a sync request after setTimeout(0).

In terms of absolute breakage for users this will probably be OK, but it's too bad that it will forever be a quirk for developers to be bitten by. I'm still inclined to say LGTM, but what do you think about always emitting console messages when these APIs are used to warn that they are not reliable? The risk is warning fatigue, of course.

Trickier trade-offs than usual, curious to hear what others think.

Jochen Eisinger

unread,
Jun 8, 2016, 11:18:46 AM6/8/16
to Philip Jägenstedt, blink-dev

Yeah, syncXHR is the surprising outlier. I guess you can still work around this by moving the entire promise handler into a setTimeout.

I also don't think we're stuck forever with this, as we want to get rid of syncXHR, and for the dialogs we'll soon(ish) return immediately for background tabs at which point not blocking will be there rule and not the exception

Jochen Eisinger

unread,
Jun 8, 2016, 11:27:24 AM6/8/16
to Philip Jägenstedt, blink-dev

And for syncXHR's we'll throw an exception, so hopefully the existing error handing kicks in

Chris Harrelson

unread,
Jun 16, 2016, 12:52:50 AM6/16/16
to Jochen Eisinger, Philip Jägenstedt, blink-dev
LGTM2, but as noted we should be ready to revert.

Jochen Eisinger

unread,
Jun 16, 2016, 1:11:38 AM6/16/16
to Chris Harrelson, Philip Jägenstedt, blink-dev
I'm wondering whether we should wait for Avi's modal dialog thing to land. Then we wouldn't ignore the modal dialogs, but just queue them up. At least, that would be then consistent with how dialogs will work.

We'd still throw an exception for SyncXHR.

Jochen Eisinger

unread,
Jun 21, 2016, 12:45:09 PM6/21/16
to Chris Harrelson, Philip Jägenstedt, blink-dev
After talking to Avi, I guess his project won't magically resolve my issues here.

I'd then propose to extend the deprecation warning one more milestone, so we have stable channel data, and decide then.

Rick Byers

unread,
Jun 21, 2016, 2:05:47 PM6/21/16
to Jochen Eisinger, Chris Harrelson, Philip Jägenstedt, blink-dev
API owners met and discussed this today.

We agreed to extend the warning one milestone, and then (just after branch) try removal.  We'll watch for feedback and validate (in addition to the single example we have already) that affected sites can indeed be updated easily enough.

If I understood correctly the plan in the sync XHR case is to fail with an exception with a specific message / URL and work with DevRel to publish guidance on how to fix code.

But there was some discussion about the other (dialog) cases - could we just avoid pumping the message loop rather than fail?  That would mean (for example) animations/resize wouldn't run during an alert invoked from a microtask, but that's probably not a big deal.  Doesn't this have the same thread sharing deadlock issues Avi has been talking about though?

bartoszcy...@gmail.com

unread,
Jul 31, 2016, 2:06:36 AM7/31/16
to blink-dev

Jochen Eisinger

unread,
Aug 5, 2016, 7:19:56 AM8/5/16
to bartoszcy...@gmail.com, blink-dev
fyi, I rolled this back, as it appears to unnecessarily break sites :-/
Reply all
Reply to author
Forward
0 new messages