Disallow certain blocking DOM calls during microtask execution.

Disallow certain blocking DOM calls during microtask execution.

BUG=605517
R=adamk@chromium.org,esprehn@chromium.org,haraken@chromium.org

Committed: https://crrev.com/0f5b86386accbd17d06a26475df674a2e1df8bf2
Cr-Commit-Position: refs/heads/master@{#391640}

[haraken, 2016-05-03 12:10:50]

Do you know what other browsers are doing here?

As Elliott mentioned in the bug, I'm a bit afraid that this will break a web
page that uses a Promise API can calls alert().

https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/methods.cpp:415: return;

As Elliott is suggesting in the bug, would it be better to add a console message
here?

[jochen (gone - plz use gerrit), 2016-05-03 12:18:15]

On 2016/05/03 at 12:10:50, haraken wrote:
> Do you know what other browsers are doing here?
> 
> As Elliott mentioned in the bug, I'm a bit afraid that this will break a web
page that uses a Promise API can calls alert().

at least FF allows alerts()

I'm fine with breaking those pages :)

> 
>
https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/templates/methods.cpp:415: return;
> 
> As Elliott is suggesting in the bug, would it be better to add a console
message here?

we can console.log something, and measure it. but the key is that we don't
create a nested message loop during microtasks.

[haraken, 2016-05-03 12:22:14]

On 2016/05/03 12:18:15, jochen wrote:
> On 2016/05/03 at 12:10:50, haraken wrote:
> > Do you know what other browsers are doing here?
> > 
> > As Elliott mentioned in the bug, I'm a bit afraid that this will break a web
> page that uses a Promise API can calls alert().
> 
> at least FF allows alerts()
> 
> I'm fine with breaking those pages :)
> 
> > 
> >
>
https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/templates/methods.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/templates/methods.cpp:415: return;
> > 
> > As Elliott is suggesting in the bug, would it be better to add a console
> message here?
> 
> we can console.log something, and measure it. but the key is that we don't
> create a nested message loop during microtasks.

Just help me understand: What's an issue of running alert() during microtasks?
I'm wondering why it's valid to run alert() in a normal JS context but invalid
to run alert() in a microtask. (My question will boil down to: when can the
microtask be triggered?)

[jochen (gone - plz use gerrit), 2016-05-03 12:26:07]

On 2016/05/03 at 12:22:14, haraken wrote:
> On 2016/05/03 12:18:15, jochen wrote:
> > On 2016/05/03 at 12:10:50, haraken wrote:
> > > Do you know what other browsers are doing here?
> > > 
> > > As Elliott mentioned in the bug, I'm a bit afraid that this will break a
web
> > page that uses a Promise API can calls alert().
> > 
> > at least FF allows alerts()
> > 
> > I'm fine with breaking those pages :)
> > 
> > > 
> > >
> >
https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/templates/methods.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1940253002/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/templates/methods.cpp:415: return;
> > > 
> > > As Elliott is suggesting in the bug, would it be better to add a console
> > message here?
> > 
> > we can console.log something, and measure it. but the key is that we don't
> > create a nested message loop during microtasks.
> 
> Just help me understand: What's an issue of running alert() during microtasks?
I'm wondering why it's valid to run alert() in a normal JS context but invalid
to run alert() in a microtask. (My question will boil down to: when can the
microtask be triggered?)

microtasks are run by definition at the end of a task of the main event loop.

What happens if you invoke alert is that we're technically outside of the main
event loop, but spin up a nested event loop. If you now resize the browser
window while the alert is displayed, resize events will be fired at the page.

That violates the DOM/ES processing model, as main event loop events are being
processed while executing microtasks.

Also, it's completely unspecified when microtasks that might get scheduled by
the resize event handler should be executed.

[jochen (gone - plz use gerrit), 2016-05-03 13:11:23]

added a console error

[haraken, 2016-05-03 13:29:32]

LGTM to try, but I'd like to hear opinions of Elliott and Adam.

[jochen (gone - plz use gerrit), 2016-05-03 14:29:32]

also throwing sync XHR into the mix...

[jochen (gone - plz use gerrit), 2016-05-03 14:30:01]

I should probably start a thread on blink-dev anyways

[haraken, 2016-05-03 15:00:04]

On 2016/05/03 14:30:01, jochen wrote:
> I should probably start a thread on blink-dev anyways

Yeah, that sounds nicer.

[jochen (gone - plz use gerrit), 2016-05-04 07:24:58]

jochen@chromium.org changed reviewers:
+ holte@chromium.org

[jochen (gone - plz use gerrit), 2016-05-04 07:24:58]

+holte for histograms.xml

I turned this a runtime enabled feature, and added use counters + a deprecation
message

ptal

[esprehn, 2016-05-04 07:43:32]

https://codereview.chromium.org/1940253002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1940253002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:74:
DisableBlockingMethodsDuringMicrotasks status=experimental
This ships this to all users with the experimental web platform features flag
on. I'd rather not do that yet.

At the very least it should be a separate patch.

[jochen (gone - plz use gerrit), 2016-05-04 07:52:27]

what about status=test?

[jochen (gone - plz use gerrit), 2016-05-04 07:59:52]

ptal

[esprehn, 2016-05-04 09:24:25]

Lgtm

[Steven Holte, 2016-05-04 19:07:53]

histograms lgtm

[jochen (gone - plz use gerrit), 2016-05-04 19:40:35]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-05-04 19:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1940253002/#ps100001
(title: "updates")

[commit-bot: I haz the power, 2016-05-04 19:41:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940253002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940253002/100001

[commit-bot: I haz the power, 2016-05-04 21:04:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-04 21:06:21]

Description was changed from

==========
Disallow certain blocking DOM calls during microtask execution.

BUG=605517
R=adamk@chromium.org,esprehn@chromium.org,haraken@chromium.org
==========

to

==========
Disallow certain blocking DOM calls during microtask execution.

BUG=605517
R=adamk@chromium.org,esprehn@chromium.org,haraken@chromium.org

Committed: https://crrev.com/0f5b86386accbd17d06a26475df674a2e1df8bf2
Cr-Commit-Position: refs/heads/master@{#391640}
==========

[commit-bot: I haz the power, 2016-05-04 21:06:22]

Patchset 6 (id:??) landed as
https://crrev.com/0f5b86386accbd17d06a26475df674a2e1df8bf2
Cr-Commit-Position: refs/heads/master@{#391640}