Bug 106771

Summary: [V8] Add m_isolate to ScriptController, WorkerScriptController and V8DOMWindowShell
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Kentaro Hara 2013-01-14 02:37:16 PST
Add m_isolate to ScriptController, WorkerScriptController and V8DOMWindowShell.
Comment 1 Kentaro Hara 2013-01-14 02:38:50 PST
Created attachment 182533 [details]
Patch
Comment 2 Adam Barth 2013-01-14 02:54:47 PST
Comment on attachment 182533 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptController.cpp:105
> +    , m_isolate(v8::Isolate::GetCurrent())

Will the isolate have been created by then?  I guess the tests will tell us!  :)

> Source/WebCore/bindings/v8/ScriptController.h:202
> +    v8::Isolate* m_isolate;

Can we hold onto the isolate forever?
Comment 3 Kentaro Hara 2013-01-14 03:02:32 PST
Comment on attachment 182533 [details]
Patch

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

>> Source/WebCore/bindings/v8/ScriptController.cpp:105
>> +    , m_isolate(v8::Isolate::GetCurrent())
> 
> Will the isolate have been created by then?  I guess the tests will tell us!  :)

Tests are passing. As far as I see, 'Isolate::New()' is found only on WorkerScriptController's constructor. Maybe the Isolate for the main thread is already initialized before ScriptController's constructor (I don't yet understand exactly).

>> Source/WebCore/bindings/v8/ScriptController.h:202
>> +    v8::Isolate* m_isolate;
> 
> Can we hold onto the isolate forever?

As long as a ScriptController is alive, a corresponding Isolate should be alive. So this is safe, isn't it?
Comment 4 WebKit Review Bot 2013-01-14 05:09:43 PST
Comment on attachment 182533 [details]
Patch

Clearing flags on attachment: 182533

Committed r139610: <http://trac.webkit.org/changeset/139610>
Comment 5 WebKit Review Bot 2013-01-14 05:09:46 PST
All reviewed patches have been landed.  Closing bug.