[Webkit-unassigned] [Bug 95735] Removed V8IsolatedContext

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=95735





--- Comment #3 from Dan Carney <dcarney at google.com>  2012-09-04 13:35:03 PST ---
(From update of attachment 162013)
View in context: https://bugs.webkit.org/attachment.cgi?id=162013&action=review

>> Source/WebCore/bindings/v8/ScriptController.cpp:376
>> +        worldID = DOMWrapperWorld::uninitializedWorldId;
> 
> Should we change the test runner so that the testing environment is more like the production environment?

It's used all over the place.  I'd be okay with changing it later, but you said you wanted to do away with worldId, which seemed like a much better fix for this problem.

>> Source/WebCore/bindings/v8/ScriptController.h:200
>> +    typedef HashMap<int, RefPtr<SecurityOrigin> > IsolatedWorldSecurityOriginMap;
> 
> I wonder if we should move the SecurityOrigin pointer into V8DOMWindowShell.  We can do that in a separate patch though.

I think it's possible to set an origin for a shell that doesn't exist yet, but I'm not sure.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:253
>> +            if (m_world->isMainWorld()) {
> 
> Do we ever call this function with weak and !m_world->isMainWorld() ?

No.  The weak flag is just for the temporary worlds that survive one call to evaluateInIsolatedWorld.  I tried to give everything the same lifecycle, but it's just not possible this iteration.  Even so, i can move it out of the else clause.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:564
>> +        return;
> 
> Is this a bad merge?  I thought I deleted this code from trunk.

Totally possible.  I've rebased this code dozens of times.  I'll get rid of it.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:651
>> +        InspectorInstrumentation::didCreateIsolatedContext(m_frame, scriptState, securityOrigin.get());
> 
> Why does setIsolatedWorldSecurityOrigin call didCreateIsolatedContext ?  I would assume that would be the responsibility of the create function, not of a function that just sets the security origin.

No idea.  This code is copied from V8IsolatedContext directly.  There were a number of issues around the way the inspector interacted with V8IsolatedContext.  I can add a FIXME to remember to figure out why it's there in the future.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.h:132
>> +    v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object> global);
> 
> createNewContext -> createContext
> 
> Why does this function return a v8::Persistent<v8::Context> ?  It seems like the result could just be stored in m_context.

It can.  I was just trying to keep the changes to a minimum, although that's a joke given the size of the patch.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.h:146
>> +    RefPtr<SecurityOrigin> m_isolatedWorldShellSecurityOrigin;
> 
> So, we keep a SecurityOrigin pointer here and in ScriptController?  Do we need to have both of them?  Do they play different roles?

This copy seems only to be around to make its getter fast.  It's just an artefact of the merge.  I'll see if I can delete it easily.

>> Source/WebCore/bindings/v8/WorldContextHandle.cpp:58
>> +    m_context = SharedPersistent<v8::Context>::create(v8::Persistent<v8::Context>::New(context));
> 
> Here is the v8::Persistent<v8::Context>::Dispose() that balances this New() ?

Yikes.  That's a huge problem.  I was using ScopedPersistent earlier, which didn't require it, but that had copy constructor issues.  I'll add the Dispose() to the SharedPersistent destructor, since the class is only used here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.