Bug 109268 – [v8] isolate parameter added to all v8::peristent calls

• Component: New Bugs
• Status: RESOLVED FIXED

[Dan Carney]

[v8] isolate parameter added to all v8::peristent calls

[Dan Carney]

Created attachment 187256 [details]
Patch

[Dan Carney]

forgot to rebase bindings tests. wiil do that before commit

[Kentaro Hara]

Comment on attachment 187256 [details]
Patch

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

> Source/WebCore/bindings/v8/NPV8Object.cpp:95
> +    v8NpObject->v8Object.Dispose(v8::Isolate::GetCurrent());

Can we avoid calling Isolate::GetCurrent() by passing an Isolate around?

> Source/WebCore/bindings/v8/NPV8Object.cpp:169
> +    v8npObject->v8Object = v8::Persistent<v8::Object>::New(v8::Isolate::GetCurrent(), object);

Ditto.

> Source/WebCore/bindings/v8/ScheduledAction.cpp:57
> +    v8::Isolate* isolate = context->GetIsolate();

Nit: context => m_context (just for consistency)

> Source/WebCore/bindings/v8/ScopedPersistent.h:46
> +        : m_handle(v8::Persistent<T>::New(v8::Isolate::GetCurrent(), handle))

This would be OK.

> Source/WebCore/bindings/v8/ScopedPersistent.h:60
>      void set(v8::Handle<T> handle)

This should receive an Isolate. If call sites are too many, you can fix it in a follow-up patch (please just add FIXME).

> Source/WebCore/bindings/v8/ScopedPersistent.h:77
> +        m_handle.Dispose(v8::Isolate::GetCurrent());

Ditto.

> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:76
> +    return v8::Persistent<v8::String>::New(v8::Isolate::GetCurrent(), v8::String::NewSymbol(key));

I think this can be fixed.

[Dan Carney]

(In reply to comment #3)
> (From update of attachment 187256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187256&action=review
> 
> > Source/WebCore/bindings/v8/NPV8Object.cpp:95
> > +    v8NpObject->v8Object.Dispose(v8::Isolate::GetCurrent());
> 
> Can we avoid calling Isolate::GetCurrent() by passing an Isolate around?


I'll see if any of these are easily fixable now, but we are going to remove all instances of GetCurrent() in followup patches, so there is no need for 100s or FIXMEs throughout the code.

[Dan Carney]

Comment on attachment 187256 [details]
Patch

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

>> Source/WebCore/bindings/v8/NPV8Object.cpp:95
>> +    v8NpObject->v8Object.Dispose(v8::Isolate::GetCurrent());
> 
> Can we avoid calling Isolate::GetCurrent() by passing an Isolate around?

All np stuff is on the main isolate. I'm going to add a static variable for the main world isolate, which is why I don't want to pass parameters around here. Again, no need for zillions of fixmes here either

[Kentaro Hara]

Understood. No FIXME is OK. But I'm not quite happy with a lot of Isolate::GetCurrent()s with "let's fix it later". Alternately, you can first land patches that pass an Isolate around so that you don't need to write Isolate::GetCurrent() too much.

[Dan Carney]

(In reply to comment #6)
> Understood. No FIXME is OK. But I'm not quite happy with a lot of Isolate::GetCurrent()s with "let's fix it later". Alternately, you can first land patches that pass an Isolate around so that you don't need to write Isolate::GetCurrent() too much.

We'd prefer a mechanical replacement of all calls with a Isolate::GetCurrent() unless an isolate parameter is already available.  After all calls are replaced, then it will be easier to analyze which calls need an isolate parameter, and which classes need an isolate member added.  Doing it in stages might not be optimal and will slow down this work.  Adding Isolate::GetCurrent() adds no overhead, and we WILL remove all calls to GetCurrent() in the future, since it will no longer do what it used to do once the v8 api is changed.

[Dan Carney]

Comment on attachment 187256 [details]
Patch

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

>> Source/WebCore/bindings/v8/ScopedPersistent.h:60
>>      void set(v8::Handle<T> handle)
> 
> This should receive an Isolate. If call sites are too many, you can fix it in a follow-up patch (please just add FIXME).

I checked. Too many calls site for this patch.

>> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:76
>> +    return v8::Persistent<v8::String>::New(v8::Isolate::GetCurrent(), v8::String::NewSymbol(key));
> 
> I think this can be fixed.

Too many call sites for this patch.

[Kentaro Hara]

Comment on attachment 187256 [details]
Patch

Makes sense to me.

[Dan Carney]

Created attachment 187264 [details]
Patch

[Dan Carney]

Comment on attachment 187264 [details]
Patch

addressed nit. added rebased bindings tests

[WebKit Review Bot]

Comment on attachment 187264 [details]
Patch

Clearing flags on attachment: 187264

Committed r142250: <http://trac.webkit.org/changeset/142250>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.