How often does it happen that Blink reads properties off an object without any other script on the stack? I can't think of any API off the top of my head that does such a thing. If there were no such cases, then at least as far as the Blink -> V8 boundary is concerned, this question is academic (since the call depth would never be 0 at the end of the property read).

There is, of course, a spec question here as well, so CCing domenic@.

This happens sometimes in specs. And in specs we always surround it with appropriate setup/teardown steps, the latter of which run microtasks if the stack is empty of user code. Failing to add the appropriate setup/teardown steps is a bug.

The most common example is addEventListener("...", { handleEvent() { ... } }). If the event is fired with no user code on the stack, then you end up in https://heycam.github.io/webidl/#call-a-user-objects-operation. Step 7/8 and 16.1/16.2 are the setup/teardown I refer to, whereas steps 9.2.1, 13, and 15 are where user code gets hit.

It happens all over the place, however, the majority of cases I went through so far can probably be classified as unit test or setting up wrappers. If it helps, I can try to identify a "real" case.

> If the event is fired with no user code on the stack, then you end up in

Well, you end up there regardless. But rather, the teardown steps only run microtasks if there is no user code on the stack. (See https://html.spec.whatwg.org/#clean-up-after-running-script)

That spec behavior is interesting, in that it does two distinct script operations before running the teardown steps: the Get(O, "handleEvent") followed by the Call(X).

If we were to make every call to v8::Object::Get() run microtasks, then we wouldn't get the specced behavior: instead, we'd run microtasks after Get(), and again after Call().

I think this happens a lot in the current V8 bindings because ScriptState::Scope doesn't enter the microtask checkpoint. EventListener's handleEvent is one example.

...;
ScriptState::Scope scope;
v8Object->Get(); // This is called without setting any microtask checkpoint.
v8Function->Call(); // This enters the microtask checkpoint though.

it sounds a bit like ScriptState::Scope should also put a microtask scope on the stack?

Yes, that's my proposal.

Controlling the microtask execution at ScriptState::Scope *should* work because you need to enter a correct ScriptSatte::Scope before calling V8 APIs that may execute an arbitrary user script.

I'm still wondering what the spec is saying though. Should we simply set the microtask checkpoint at all ScriptState::Scopes?

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c30f09304a88c981c8637462c257add88ae09f90

commit c30f09304a88c981c8637462c257add88ae09f90
Author: Jochen Eisinger <jochen@chromium.org>
Date: Fri Jun 02 12:16:19 2017

Introduce a flag to control microtask scope consistency checking

We want to be stricter about checking in the future, so give embedders a
way to disable checking while they fix their microtasks scopes.

BUG=chromium:728583
R=machenbach@chromium.org

Change-Id: I443575bf6820b432def59cbbd4d048b2007573c8
Reviewed-on: https://chromium-review.googlesource.com/522604
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45677}
[modify] https://crrev.com/c30f09304a88c981c8637462c257add88ae09f90/BUILD.gn
[modify] https://crrev.com/c30f09304a88c981c8637462c257add88ae09f90/gypfiles/features.gypi
[modify] https://crrev.com/c30f09304a88c981c8637462c257add88ae09f90/gypfiles/standalone.gypi
[modify] https://crrev.com/c30f09304a88c981c8637462c257add88ae09f90/src/api.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8be7d37bae53e8224cbd4ca210d34b2b5580d7c8

commit 8be7d37bae53e8224cbd4ca210d34b2b5580d7c8
Author: Jochen Eisinger <jochen@chromium.org>
Date: Mon Jun 05 07:42:56 2017

Turn of microtasks scopes checking

This will enable V8 to add checks for correct microtasks handling, while
Blink works on putting the scopes in place.

BUG=728583
R=haraken@chromium.org,machenbach@chromium.org

Change-Id: Idf71ecf98dd730b12273e7a83353013cd62c14ad
Reviewed-on: https://chromium-review.googlesource.com/522703
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476957}
[modify] https://crrev.com/8be7d37bae53e8224cbd4ca210d34b2b5580d7c8/.gn

Another thing came up while trying to land the V8 side CL.

Not only does blink not set the microtask scopes correctly, we also invoke a lot of APIs that potentially invoke user script during script forbidden scopes :/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2865ab16e98fa7ac6892ea0a753ec2f6213c1710

commit 2865ab16e98fa7ac6892ea0a753ec2f6213c1710
Author: Jochen Eisinger <jochen@chromium.org>
Date: Wed Jun 21 15:22:47 2017

Temporarily disable script forbidden scope checks

While fixing the V8 side to invoke the callback correctly, I temporarily
disable the check on the blink side until we've correctly cleaned up all
callsites that execute script, and can re-enable then check

BUG=728583
R=haraken@chromium.org

Change-Id: Ia32abee6485124766d73f69cdb6cc3350bffe3b4
Reviewed-on: https://chromium-review.googlesource.com/542815
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481211}
[modify] https://crrev.com/2865ab16e98fa7ac6892ea0a753ec2f6213c1710/third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3612350c5ccba69249d4e3fdb124c742d344ec73

commit 3612350c5ccba69249d4e3fdb124c742d344ec73
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Mon Sep 04 02:56:04 2017

Supports property deletion with ENTER_V8_NO_SCRIPT.

As Blink needs a way to delete a property without running a script,
make Object::Delete use ENTER_V8_NO_SCRIPT if the receiver object is
not a JSProxy.  Also makes Object::DeletePrivate use
ENTER_V8_NO_SCRIPT, too.

Bug: chromium:728583
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Ib37959764b99a68d730d1bbc6dba410106d4f452
Reviewed-on: https://chromium-review.googlesource.com/608348
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47779}
[modify] https://crrev.com/3612350c5ccba69249d4e3fdb124c742d344ec73/src/api.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/014bbbb65ef9fed41c95613f56225758445d54f8

commit 014bbbb65ef9fed41c95613f56225758445d54f8
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Tue Sep 05 03:34:15 2017

v8binding: Enables ScriptForbiddenScope check.

Enables the ScriptForbiddenScope checks so that Blink crashes
when it's about to run a script inside a ScriptForbiddenScope.

Bug: 728583
Change-Id: I44bd07855e4746c6081456f336a09a954cdc1b5e
Reviewed-on: https://chromium-review.googlesource.com/648373
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499550}
[modify] https://crrev.com/014bbbb65ef9fed41c95613f56225758445d54f8/third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp

I think that we can now close this issue as Fixed, but I defer the decision to adamk@.

I don't think this is fixed yet, as we still don't trigger microtasks for all calls to Object::Get().

Per the spec (https://html.spec.whatwg.org/multipage/webappapis.html#calling-scripts), wouldn't it be sufficient to run microtasks at the end of the event loop?

Do we need to invoke microtasks at the end of each V8 API that may run an arbitrary user script?

Well, that's the question Jochen posed at the beginning, and Domenic gives in example in #2.

There are a variety of places that call into https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script, including https://heycam.github.io/webidl/#get-a-user-objects-attribute-value. I'm not sure where the latter operation is called form, but it definitely sounds like it's intended to run microtasks after getting a property.

There's two ameliorating factors, however:

- Clean up after running script only runs a microtask checkpoint if the stack is empty, not every time.
- Getting a user object's attribute value is kind of rare; it only applies to callback interface types (e.g. EventHandlers).

I'm not sure what a good systematic way to find all these cases or enforce them are. But things are probably not that bad right now, would be my guess...

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/eeadb48c284e76dd16effce5c3ccd6b712f674e8

commit eeadb48c284e76dd16effce5c3ccd6b712f674e8
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Thu Sep 07 05:18:38 2017

Supports Object::DefineOwnProperty with ENTER_V8_NO_SCRIPT.

As Blink needs a way to define a property without running a script,
make Object::DefineOwnProperty use ENTER_V8_NO_SCRIPT if the
receiver object is not a JSProxy.  Quite similar to
https://crrev.com/c/v8/v8/+/608348

Bug: chromium:728583,  chromedriver:1978 ,  chromium:762385 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: If358bf0d156139c456de369ac04da2be6e626143
Reviewed-on: https://chromium-review.googlesource.com/651949
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47862}
[modify] https://crrev.com/eeadb48c284e76dd16effce5c3ccd6b712f674e8/src/api.cc

FYI, I associated the following chrome crashes with this issue:

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%27%20%20AND%20custom_data.ChromeCrashProto.channel%3D%27canary%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27renderer%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27blink%3A%3ABeforeCallEnteredCallback%27%20AND%20product.Version%3D%2763.0.3207.0%27&sql_dialect=dremelsql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&unnest=#samplereports

I expect them to disappear when yukishiino's CL hits canary.

If it's really only for the "handleEvent" method, this strikes me as something like a P4 spec compliance issue (and should probably be a separate bug). So I'd be happy to close this one out.

Yeah, in theory it's for every Get() that's made, but very few of those exist that end up being done on a clean stack; handleEvent might be the only one. I guess also it applies to Call()s of plain-function event listeners.

Another case where we call author JS code with a clean stack is custom element construction, but I know the microtasks are working correctly there.

I don't think there are *many* more... but as I said, I don't feel like we have a comprehensive way of tracking this.

Users experienced this crash on the following builds:

Linux Dev 63.0.3236.7 -  1.35 CPM, 6 reports, 1 clients (signature blink::BeforeCallEnteredCallback)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

Users experienced this crash on the following builds:

Mac Canary 64.0.3250.0 -  0.39 CPM, 1 reports, 1 clients (signature blink::BeforeCallEnteredCallback)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

Just to update:
blink::BeforeCallEnteredCallback

Still seeing crash instances on latest dev-64.0.3278.0 & still seeing 4 crash instances on 4 clients so far.

64.0.3278.0	44.44%	4	-Dev
64.0.3277.0	11.11%	1	

Link to the list of builds:
-------------------------
https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27extension%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27blink%3A%3ABeforeCallEnteredCallback%27&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D

Thanks..!

@haraken, do you have a good feeling for whether there's important work to be done on this issue soon? Per comments 23 and 24, I'm inclined to file a new bug for surrounding the EventListener "handleEvent" lookup with the right scoping, and close this one out.

Re #28: Sounds reasonable to me.

Filed issue 798845, closing this one.

We still have

  # TODO(jochen): Remove this. http://crbug.com/v8/5830,
  # http://crbug.com/728583.
  v8_check_microtasks_scopes_consistency = false


in src.git/.gn

Can that be deleted now?

yes, that should be removed (not sure it can)