Bug 70289 – Add JSC support for delivering mutations when the outermost script context exits

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Adam Klein]

http://trac.webkit.org/changeset/97659 added support to the V8 bindings to deliver mutations at the end of the outermost script context. This was trivial for V8, since V8Proxy provides a single entrypoint for script execution, and already keeps track of reentrancy.  For JSC, there are many references to JSC::call in bindings/js/, so this will likely require some reorganization, either by adding a hook in JSC or by wrapping all JSC:call's.

For background, the above is necessary in order to provide delivery at the end of a "microtask", as discussed http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/1622.html (the timing is the first bullet under "Significant aspects of this design").

[Sam Weinig]

We mostly have a bottleneck for call in JSC by way of JSMainThreadExecState::call(), though it is not used everywhere (that we can fix).  I am not clear on how aggressive we have to be however.  For instance, do we need to consider calling a getter while processing an options dictionary passed to an Event constructor a script invocation?

[Sam Weinig]

Created attachment 113965 [details]
Patch in progress.

This patch removes more main thread direct calls to JSC::call.  The remaining uses are:

- JavaJSObject::call()
- _NPN_InvokeDefault
- _NPN_Invoke
Plugins and Java, not sure if these should delineate micro tasks.

- JSInjectedScriptHost::evaluate()
- InjectedScriptManager::createInjectedScript()
Inspector, not sure if these should delineate micro tasks.

- ScriptCallback::call()
This one we should fix by just removing the class an implementing MediaQuery callbacks more idiomatically.

[Ryosuke Niwa]

I'm super excited for this patch :D

[Adam Klein]

(In reply to comment #1)
> We mostly have a bottleneck for call in JSC by way of JSMainThreadExecState::call(), though it is not used everywhere (that we can fix).  I am not clear on how aggressive we have to be however.  For instance, do we need to consider calling a getter while processing an options dictionary passed to an Event constructor a script invocation?

Ideally I'd think we'd want to deliver mutations if we ever exit JS and find that it was the outermost script invocation.  In your example, the event constructor would actually have been called by script itself, so it wouldn't be affected. But I can imagine cases where a JS object's properties are accessed from C++ (maybe structured clone in IndexedDB?). For completeness, I think getters/setters would need to be covered as well, though I am having trouble coming up with a lot of cases where it matters (since most accesses of JS objects are due to calls from script themselves).

[Adam Klein]

What's the status of this patch? On the Chromium side, we're basically feature-complete and enabled in dev channel (blocking on the completed spec before we go to beta). What would need to be done to get ENABLE(MUTATION_OBSERVERS) turned on for the JSC ports? If there's something Rafael and I can do to polish off the JSC side of things we'd be eager to help.

[Geoffrey Garen]

Can WebKitMutationObserver just schedule a 0-delay delivery timer the first time a mutation record is enqueued? It's not clear why this code needs to tie into the JS engine.

[Rafael Weinstein]

The short answer is: that will be too late.

The history and rational for what was finally agreed upon as the solution in the spec starts here:

http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0780.html

and ends here:

http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/1622.html

[Adam Klein]

Created attachment 123864 [details]
Patch

[Adam Klein]

(In reply to comment #8)
> Created an attachment (id=123864) [details]
> Patch

This patch includes Sam's changes as well as the rest of the infrastructure necessary to get the tests passing when ENABLE(MUTATION_OBSERVERS) is on for WebKit/Mac.

[Darin Adler]

Comment on attachment 123864 [details]
Patch

Sam’s on vacation and he’ll be back Monday.

[Adam Klein]

Sam or Geoff, can you take a look and at least let me know if I'm going down the right path with this approach?

[Geoffrey Garen]

Looks OK to me.

[Eric Seidel (no email)]

Comment on attachment 123864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123864&action=review

This looks very reasonable to me.  I suspect the DRT Obj-c code could be slightly nicer.  I'm comfortable r+ing this based on ggaren's OKing of the patch, and Sam's original posting of the JSC ->JSMaintThreadExecState change.

I'm assuming here that you audited (or Sam audited?) all callers of JSC::call?  It seems very possible that one got forgotten or that one will be forgotten in the future.

> Tools/DumpRenderTree/mac/EventSendingController.mm:728
> +    [character release];
> +    [modifiers release];

You don't need these. :)

> Tools/DumpRenderTree/mac/EventSendingController.mm:734
> +    NSInvocation *invocation = [NSInvocation invocationWithMethodSignature:[EventSendingController instanceMethodSignatureForSelector:@selector(keyDownWrapper:withModifiers:withLocation:)]];
> +    [invocation retainArguments];

I suspect you could also do this with a block in modern Obj-C 2.  I don't know how much (if any) of Obj-C 2 we use in DRT, but I"m sure it's supported.

> Tools/DumpRenderTree/mac/EventSendingController.mm:740
> +    [invocation performSelector:@selector(invoke) withObject:nil afterDelay:0];

What about a timer?  Again, possibly using a block?

[Adam Klein]

Comment on attachment 123864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123864&action=review

>> Tools/DumpRenderTree/mac/EventSendingController.mm:728
>> +    [modifiers release];
> 
> You don't need these. :)

Will remove when I land-safely.

>> Tools/DumpRenderTree/mac/EventSendingController.mm:734
>> +    [invocation retainArguments];
> 
> I suspect you could also do this with a block in modern Obj-C 2.  I don't know how much (if any) of Obj-C 2 we use in DRT, but I"m sure it's supported.

If it's all right with you, I'd rather leave this as an NSInvocation, since it matches up with the rest of the code in this file.

>> Tools/DumpRenderTree/mac/EventSendingController.mm:740
>> +    [invocation performSelector:@selector(invoke) withObject:nil afterDelay:0];
> 
> What about a timer?  Again, possibly using a block?

Not sure if an NSTimer really helps here, I'd still need the NSInvocation to store the arguments as far as I can tell (though I'll admit that I know very little Obj-C).

[Adam Klein]

(In reply to comment #13)
> (From update of attachment 123864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123864&action=review
> 
> This looks very reasonable to me.  I suspect the DRT Obj-c code could be slightly nicer.  I'm comfortable r+ing this based on ggaren's OKing of the patch, and Sam's original posting of the JSC ->JSMaintThreadExecState change.
> 
> I'm assuming here that you audited (or Sam audited?) all callers of JSC::call?  It seems very possible that one got forgotten or that one will be forgotten in the future.

Yeah, for now I just made sure that all JSC::call invocations under Source/WebCore/ do this appropriately. It definitely seems like something could slip through in the future, though, so it would be ideal if we stopped calling JSC::call altogether in bindings/js/.

[Ryosuke Niwa]

(In reply to comment #14)
> (From update of attachment 123864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123864&action=review
> 
> >> Tools/DumpRenderTree/mac/EventSendingController.mm:728
> >> +    [modifiers release];
> > 
> > You don't need these. :)
> 
> Will remove when I land-safely.

land-safely doesn't help at all here because commit-queue uses cr-linux. It'll let JSC build failures slip through.

[Adam Klein]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 123864 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=123864&action=review
> > 
> > >> Tools/DumpRenderTree/mac/EventSendingController.mm:728
> > >> +    [modifiers release];
> > > 
> > > You don't need these. :)
> > 
> > Will remove when I land-safely.
> 
> land-safely doesn't help at all here because commit-queue uses cr-linux. It'll let JSC build failures slip through.

Compiling on JSC wouldn't matter much anyway since all the code is currently behind ENABLE(MUTATION_OBSERVERS). I've been compiling locally throughout to test that the new C++ code works.

[Adam Klein]

Committed r107008: <http://trac.webkit.org/changeset/107008>