Bug 77898

Summary: DOM mutations should not be delivered on worker threads
Product: WebKit Reporter: Adam Klein <adamk>
Component: New BugsAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dimich, eric, japhet, jchaffraix, levin, morrita, podivilov, rafaelw, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Adam Klein 2012-02-06 13:57:03 PST
DOM mutations should not be delivered on worker threads
Comment 1 Adam Klein 2012-02-06 14:01:42 PST
Created attachment 125704 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-07 14:45:01 PST
Would be best if one of the worker folks would comment.
Comment 3 Dmitry Titov 2012-02-07 15:27:41 PST
Comment on attachment 125704 [details]
Patch

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

A quick look:

> Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:86
> +    V8Proxy* proxy = V8Proxy::retrieve(scriptExecutionContext);

It seems instrumentedCallFunction does a lot more then only mutation events delivery. For example, it seems to terminate ongoing IDB transactions...

Also, the shape of the code is not really readable. For a person who didn't read the bug description, the method of invoking callback here may look mysterious.
Maybe instrumentedCallFunction() can take a scriptExecutionContext or a flag indicating it's on a worker thread or use page==0 fact to do something different inside, it could be more readable at least?
Comment 4 Adam Klein 2012-02-08 13:01:47 PST
Created attachment 126136 [details]
Patch
Comment 5 Adam Klein 2012-02-08 13:04:48 PST
(In reply to comment #3)
> (From update of attachment 125704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125704&action=review
> 
> A quick look:
> 
> > Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:86
> > +    V8Proxy* proxy = V8Proxy::retrieve(scriptExecutionContext);
> 
> It seems instrumentedCallFunction does a lot more then only mutation events delivery. For example, it seems to terminate ongoing IDB transactions...
> 
> Also, the shape of the code is not really readable. For a person who didn't read the bug description, the method of invoking callback here may look mysterious.
> Maybe instrumentedCallFunction() can take a scriptExecutionContext or a flag indicating it's on a worker thread or use page==0 fact to do something different inside, it could be more readable at least?

Agreed that this wasn't particularly clear, and you're right that the IDB transactions should still work on Worker threads (their state is kept in a thread-local).  I've updated instrumentedCallFunction to take a Frame instead of a Page, and V8RecursionScope to make sure that it's in a Document context before calling deliverAllMutations.
Comment 6 Dmitry Titov 2012-02-08 18:08:50 PST
Comment on attachment 126136 [details]
Patch

Looks good... Thanks for changes!
Comment 7 WebKit Review Bot 2012-02-08 19:34:45 PST
Comment on attachment 126136 [details]
Patch

Clearing flags on attachment: 126136

Committed r107170: <http://trac.webkit.org/changeset/107170>
Comment 8 WebKit Review Bot 2012-02-08 19:34:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryosuke Niwa 2012-02-09 02:03:34 PST
It appears that this patch has regressed Dromaeo core eval:
http://webkit-perf.appspot.com/graph.html?#tests=[[42013,2001,3001]]&sel=1328743452190,1328769266825&displayrange=7&datatype=running

The regression doesn't appear on Lion perf bot.
Comment 10 Ryosuke Niwa 2012-02-09 02:04:10 PST
+webkit gardeners for a potential perf regression.
Comment 11 Ryosuke Niwa 2012-02-09 02:08:22 PST
Actually never mind. It's more likely to be caused by http://trac.webkit.org/changeset/107166/trunk/Source/WebKit/chromium/DEPS.