Bug 77898 – DOM mutations should not be delivered on worker threads

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

DOM mutations should not be delivered on worker threads

[Adam Klein]

Created attachment 125704 [details]
Patch

[Eric Seidel (no email)]

Would be best if one of the worker folks would comment.

[Dmitry Titov]

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?

[Adam Klein]

Created attachment 126136 [details]
Patch

[Adam Klein]

(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.

[Dmitry Titov]

Comment on attachment 126136 [details]
Patch

Looks good... Thanks for changes!

[WebKit Review Bot]

Comment on attachment 126136 [details]
Patch

Clearing flags on attachment: 126136

Committed r107170: <http://trac.webkit.org/changeset/107170>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

+webkit gardeners for a potential perf regression.

[Ryosuke Niwa]

Actually never mind. It's more likely to be caused by http://trac.webkit.org/changeset/107166/trunk/Source/WebKit/chromium/DEPS.