Bug 77898 - DOM mutations should not be delivered on worker threads
Summary: DOM mutations should not be delivered on worker threads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-06 13:57 PST by Adam Klein
Modified: 2012-02-09 02:08 PST (History)
11 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2012-02-06 14:01 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (15.66 KB, patch)
2012-02-08 13:01 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.