Bug 70289

Summary: Add JSC support for delivering mutations when the outermost script context exits
Product: WebKit Reporter: Adam Klein <adamk>
Component: JavaScriptCoreAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, darin, ggaren, mjs, rafaelw, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 68729, 71865    
Attachments:
Description Flags
Patch in progress.
none
Patch eric: review+

Description Adam Klein 2011-10-17 17:15:29 PDT
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").
Comment 1 Sam Weinig 2011-11-07 17:24:33 PST
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?
Comment 2 Sam Weinig 2011-11-07 17:28:28 PST
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.
Comment 3 Ryosuke Niwa 2011-11-07 21:52:14 PST
I'm super excited for this patch :D
Comment 4 Adam Klein 2011-11-08 09:48:48 PST
(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).
Comment 5 Adam Klein 2011-12-16 11:48:00 PST
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.
Comment 6 Geoffrey Garen 2011-12-19 18:22:42 PST
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.
Comment 7 Rafael Weinstein 2011-12-20 10:37:28 PST
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
Comment 8 Adam Klein 2012-01-24 18:05:31 PST
Created attachment 123864 [details]
Patch
Comment 9 Adam Klein 2012-01-24 18:07:06 PST
(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.
Comment 10 Darin Adler 2012-01-26 10:44:19 PST
Comment on attachment 123864 [details]
Patch

Sam’s on vacation and he’ll be back Monday.
Comment 11 Adam Klein 2012-02-06 14:41:52 PST
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?
Comment 12 Geoffrey Garen 2012-02-06 15:34:55 PST
Looks OK to me.
Comment 13 Eric Seidel (no email) 2012-02-07 15:02:01 PST
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?
Comment 14 Adam Klein 2012-02-07 15:24:38 PST
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).
Comment 15 Adam Klein 2012-02-07 15:25:56 PST
(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/.
Comment 16 Ryosuke Niwa 2012-02-07 15:26:30 PST
(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.
Comment 17 Adam Klein 2012-02-07 15:29:56 PST
(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.
Comment 18 Adam Klein 2012-02-07 16:40:42 PST
Committed r107008: <http://trac.webkit.org/changeset/107008>