DOM mutations should not be delivered on worker threads
Created attachment 125704 [details] Patch
Would be best if one of the worker folks would comment.
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?
Created attachment 126136 [details] Patch
(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 on attachment 126136 [details] Patch Looks good... Thanks for changes!
Comment on attachment 126136 [details] Patch Clearing flags on attachment: 126136 Committed r107170: <http://trac.webkit.org/changeset/107170>
All reviewed patches have been landed. Closing bug.
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.
+webkit gardeners for a potential perf regression.
Actually never mind. It's more likely to be caused by http://trac.webkit.org/changeset/107166/trunk/Source/WebKit/chromium/DEPS.