Bug 65778 – [WebWorkers][chromium] Make statics thread-safe and make sure V8 API accesses correct isolates

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dmitry Lomov]

After attempting to switch WebWorkers in-process and run tests I discovered a bunch of statics that should be made per-isolate and several cases of V8 API accessing the wrong isolate.

[Dmitry Lomov]

Created attachment 103086 [details]
Fixes.

[WebKit Review Bot]

Comment on attachment 103086 [details]
Fixes.

Attachment 103086 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9310020

[WebKit Review Bot]

Comment on attachment 103086 [details]
Fixes.

Attachment 103086 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9323039

[Dmitry Lomov]

Created attachment 103102 [details]
Release compilation fixed

[David Levin]

Comment on attachment 103102 [details]
Release compilation fixed

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

Just a few things to clear/clean up.

> Source/WebCore/ChangeLog:11
> +        (WebCore::V8BindingPerIsolateData::lazyEventListenerToStringTemplate):

Why are all of these items being moved?

Ideally they would be moved in one change and if any changes were needed, then that would happen in another change.

> Source/WebCore/bindings/v8/V8Binding.h:176
> +        inline AllowAllocation()

Isn't inline redundant here (since you put the function body in the class definition)?

> Source/WebCore/bindings/v8/V8Binding.h:183
> +        inline ~AllowAllocation()

Ditto.

> Source/WebCore/bindings/v8/V8Binding.h:199
> +      static inline v8::Local<v8::Object> newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]);

The indentation is following Chromium style as opposed to WebKit style.

> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143
>                  toStringTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(V8LazyEventListenerToString));

I wonder why this isn't part of the method lazyEventListenerToStringTemplate.

> Source/WebKit/chromium/ChangeLog:8
> +        * src/BoundObject.cpp:Headers

Would be nice to explain why this change was needed. (Now I see why bur it is still nice to add the info here to make it quicker for people to understand.)

[Dmitry Lomov]

Comment on attachment 103102 [details]
Release compilation fixed

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

>> Source/WebCore/ChangeLog:11

> 
> Why are all of these items being moved?
> 
> Ideally they would be moved in one change and if any changes were needed, then that would happen in another change.

You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy).

Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that.

>> Source/WebCore/bindings/v8/V8Binding.h:176
>> +        inline AllowAllocation()
> 
> Isn't inline redundant here (since you put the function body in the class definition)?

You are right - I'll remove this.

>> Source/WebCore/bindings/v8/V8Binding.h:199
>> +      static inline v8::Local<v8::Object> newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]);
> 
> The indentation is following Chromium style as opposed to WebKit style.

Will fix

>> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143
>>                  toStringTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(V8LazyEventListenerToString));
> 
> I wonder why this isn't part of the method lazyEventListenerToStringTemplate.

V8BindingPerIsolateData does not need to know anything about V8LazyEventListener, other than provide a container for its data. This logic really belongs here.

>> Source/WebKit/chromium/ChangeLog:8
>> +        * src/BoundObject.cpp:Headers
> 
> Would be nice to explain why this change was needed. (Now I see why bur it is still nice to add the info here to make it quicker for people to understand.)

True that) Will add comments.

[Dmitry Lomov]

Created attachment 103263 [details]
CR feedback

[David Levin]

(In reply to comment #6)
> (From update of attachment 103102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review
> 
> >> Source/WebCore/ChangeLog:11
> 
> > 
> > Why are all of these items being moved?
> > 
> > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change.
> 
> You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy).
> 
> Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that.

I won't insist. However, doing this all at once makes it much harder to determine what is being moved vs what is being changed but at the moment and later when someone goes thru diffs to understand what was done.

[WebKit Review Bot]

Comment on attachment 103263 [details]
CR feedback

Clearing flags on attachment: 103263

Committed r92619: <http://trac.webkit.org/changeset/92619>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dmitry Lomov]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 103102 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review
> > 
> > >> Source/WebCore/ChangeLog:11
> > 
> > > 
> > > Why are all of these items being moved?
> > > 
> > > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change.
> > 
> > You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy).
> > 
> > Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that.
> 
> I won't insist. However, doing this all at once makes it much harder to determine what is being moved vs what is being changed but at the moment and later when someone goes thru diffs to understand what was done.

Point taken.

[Dmitry Lomov]

Patch rolled out due to breaking chromium canary

[Dmitry Lomov]

Created attachment 103370 [details]
Fixed field initialization to unbreak chromium canary.

[WebKit Review Bot]

Comment on attachment 103370 [details]
Fixed field initialization to unbreak chromium canary.

Clearing flags on attachment: 103370

Committed r92694: <http://trac.webkit.org/changeset/92694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.