Bug 95735 – Removed V8IsolatedContext

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dan Carney]

Removed V8IsolatedContext

[Dan Carney]

Created attachment 162013 [details]
Patch

[Adam Barth]

Comment on attachment 162013 [details]
Patch

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

This is a big step forward.  Below are a bunch of minor comments.  The only one that's really important is the question about line 58 of WorldContextHandle.cpp.  I'd like to see another iteration of this patch, if that ok with you.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:98
> +    if (worldId == mainWorldId) {
> +        ASSERT_NOT_REACHED();
> +        return mainThreadNormalWorld();
> +    }

Why not leave this as an ASSERT?

> Source/WebCore/bindings/v8/ScriptController.cpp:329
> +    if (UNLIKELY(worldId == DOMWrapperWorld::mainWorldId)) {
> +        ASSERT_NOT_REACHED();
> +        return m_windowShell;
> +    }

Why not just ASSERT this?

> Source/WebCore/bindings/v8/ScriptController.cpp:352
> +    if (UNLIKELY(!world)) {
> +        ASSERT_NOT_REACHED();
> +        return 0;
> +    }

This should just be an ASSERT.

> Source/WebCore/bindings/v8/ScriptController.cpp:376
> +    // Except in the test runner, worldID should be non 0 as it conflicts with the mainWorldId.
> +    if (UNLIKELY(!worldID))
> +        worldID = DOMWrapperWorld::uninitializedWorldId;

Should we change the test runner so that the testing environment is more like the production environment?

> Source/WebCore/bindings/v8/ScriptController.h:200
> +    typedef HashMap<int, RefPtr<V8DOMWindowShell> > IsolatedWorldMap;
> +    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.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:234
> +    if (UNLIKELY(m_context.get().IsWeak())) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:253
> +            if (m_world->isMainWorld()) {

Do we ever call this function with weak and !m_world->isMainWorld() ?

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:525
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:560
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:564
> +    // FIXME: This shouldn't be possible anymore.
> +    if (!m_frame->document())
> +        return;

Is this a bad merge?  I thought I deleted this code from trunk.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:594
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:612
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:633
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:646
> +    if (m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

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

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

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:133
> +    bool installDOMWindow(v8::Handle<v8::Context>, DOMWindow*);

Now that this function isn't static, it doesn't need to take a v8::Handle<v8::Context> parameter.  It can just use m_context.

> 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?

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:53
> +        ASSERT_NOT_REACHED();

Please just use ASSERT rather than trying to handle these impossible cases.

> 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() ?

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:70
> +    if (m_context->get().IsEmpty()) {
> +        ASSERT_NOT_REACHED();
> +        // FIXME: This is problematic, but an empty context is worse.
>          return script->mainWorldContext();
> +    }

Please just ASSERT

[Dan Carney]

Comment on attachment 162013 [details]
Patch

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.

[Adam Barth]

Another option option is to make WorldContextHandle RefCounted and then to just use ScopedPersistent.  I'd like to get rid of SharedPersistent if possible.

[Dan Carney]

(In reply to comment #4)
> Another option option is to make WorldContextHandle RefCounted and then to just use ScopedPersistent.  I'd like to get rid of SharedPersistent if possible.

I had tried that, but it was more change than I wanted to make.  I've marked it as FIXME and will remove it after this patch stabilizes.

[Dan Carney]

Created attachment 162221 [details]
Patch

[Dan Carney]

(In reply to comment #6)
> Created an attachment (id=162221) [details]
> Patch

This patch covers the above concerns.  Mostly, I've marked everything up with FIXMEs which I'll deal with once this patch is stable.  I've also moved all isolated shells back to using the weak destruction method to minimize change.

[WebKit Review Bot]

Comment on attachment 162221 [details]
Patch

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

New failing tests:
http/tests/misc/window-open-then-write.html

[Adam Barth]

Comment on attachment 162221 [details]
Patch

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

This looks great.  Is that test failure really caused by this patch?  Let's try running it through the commit-queue.

> Source/WebCore/bindings/v8/ScriptController.cpp:336
> +            ASSERT(iter->second->world()->worldId() == worldId);
> +            ASSERT(iter->second->world()->extensionGroup() == extensionGroup);
> +           return iter->second;

Bad indent.

> Source/WebCore/bindings/v8/SharedPersistent.h:47
> +            return adoptRef(new SharedPersistent<T>(v8::Persistent<v8::Context>::New(value)));

I would have just make this class hold a ScopedPersistent, but let's not worry about that since we're going to remove it shortly.

[WebKit Review Bot]

Comment on attachment 162221 [details]
Patch

Rejecting attachment 162221 [details] from commit-queue.

New failing tests:
http/tests/misc/window-open-then-write.html
Full output: http://queues.webkit.org/results/13769075

[Dan Carney]

Created attachment 163077 [details]
Patch

[Dan Carney]

(In reply to comment #10)
> (From update of attachment 162221 [details])
> Rejecting attachment 162221 [details] from commit-queue.
> 
> New failing tests:
> http/tests/misc/window-open-then-write.html
> Full output: http://queues.webkit.org/results/13769075

Reintroduced hack to fix test timeout.  Marked as FIXME.

[WebKit Review Bot]

Comment on attachment 163077 [details]
Patch

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

New failing tests:
http/tests/media/media-source/seek-to-end-after-duration-change.html

[Adam Barth]

Comment on attachment 163077 [details]
Patch

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

There's some more polishing that we could do on this patch, but I'd like to get this landed.  We can iterate after landing it.

> Source/WebCore/bindings/v8/ScriptController.cpp:379
> +        V8DOMWindowShell* isolatedWorldShell = getOrCreateIsolatedWorldContext(worldID, extensionGroup);

nit: getOrCreateIsolatedWorldContext -> ensureIsolatedWindowShell(worldID, extensionGroup) ?

We typically use the word "ensure" to mean "get or create".

> Source/WebCore/bindings/v8/ScriptController.cpp:470
> -    v8::Handle<v8::Context> v8Context = ScriptController::mainWorldContext(frame);
> +    v8::Handle<v8::Context> v8Context = mainWorldContext(frame);

I'd change the type of this handle to Local to document that it is a local handle.

> Source/WebCore/bindings/v8/ScriptController.h:72
> +    // FIXME: Replace existingWindowShell with existingWindowShellInternal see comment in V8DOMWindowShell::initializeIfNeeded.
> +    ScriptController* existingWindowShell(DOMWrapperWorld*) { return this; }

Why does existingWindowShell return a ScriptController* rather than a V8DOMWindowShell* ?

> Source/WebCore/bindings/v8/ScriptController.h:201
> +    typedef HashMap<int, V8DOMWindowShell*> IsolatedWorldMap;

I see.  We're still using the V8 garbage collector to manage the lifetime of the V8DOMWindowShell in the case of isolated worlds...  Interesting.  I'm not sure that's what we want in the long term, but I think its ok for now.

> Source/WebCore/bindings/v8/SharedPersistent.h:47
> +            return adoptRef(new SharedPersistent<T>(v8::Persistent<v8::Context>::New(value)));

Technically this should be v8::Persistent<T>::New since we don't know that T is v8::Context.

> Source/WebCore/bindings/v8/SharedPersistent.h:49
> +        ~SharedPersistent()

I thought we were just going to have this class hold a ScopedPersistent rather than a v8::Persistent.  That way we'd get the calls to New and Dispose for free.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:181
> +    return reinterpret_cast<V8DOMWindowShell*>(toInnerGlobalObject(v8::Context::GetEntered())->GetPointerFromInternalField(V8DOMWindow::enteredIsolatedWorldIndex));

reinterpret_cast -> static_cast

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:219
> +    delete reinterpret_cast<V8DOMWindowShell*>(parameter);

reinterpret_cast -> static_cast

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:108
> +        // This is a temporary performance optimization. Essentially,
> +        // GetHiddenValue is too slow for this code path. We need to get the
> +        // V8 team to add a real property to v8::Context for isolated worlds.
> +        // Until then, we optimize the common case of not having any isolated
> +        // worlds at all.

I'd get rid of this comment.  It's not really temporary!

[Adam Barth]

Lets do one more round addressing the (minor) comments above and then lets land this patch.

[Adam Barth]

Comment on attachment 163077 [details]
Patch

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

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:237
> +        m_context.get().MakeWeak(this, isolatedContextWeakCallback);

We should probably 0 out m_frame at this point so we don't have a dangling reference to the Frame.

[Dan Carney]

Comment on attachment 163077 [details]
Patch

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

>> Source/WebCore/bindings/v8/ScriptController.cpp:379
>> +        V8DOMWindowShell* isolatedWorldShell = getOrCreateIsolatedWorldContext(worldID, extensionGroup);
> 
> nit: getOrCreateIsolatedWorldContext -> ensureIsolatedWindowShell(worldID, extensionGroup) ?
> 
> We typically use the word "ensure" to mean "get or create".

ok

>> Source/WebCore/bindings/v8/ScriptController.h:72
>> +    ScriptController* existingWindowShell(DOMWrapperWorld*) { return this; }
> 
> Why does existingWindowShell return a ScriptController* rather than a V8DOMWindowShell* ?

I'll fix that after this patch lands.  There's a weird reliance on existingWindowShell never returning a main world shell.  I think it only caused issues with one test.

>> Source/WebCore/bindings/v8/ScriptController.h:201
>> +    typedef HashMap<int, V8DOMWindowShell*> IsolatedWorldMap;
> 
> I see.  We're still using the V8 garbage collector to manage the lifetime of the V8DOMWindowShell in the case of isolated worlds...  Interesting.  I'm not sure that's what we want in the long term, but I think its ok for now.

I tried to minimize change.  For all but the worlds that are created for one call, I believe I can switch to having the lifecycle identical to that of the main window shell.

>> Source/WebCore/bindings/v8/SharedPersistent.h:47
>> +            return adoptRef(new SharedPersistent<T>(v8::Persistent<v8::Context>::New(value)));
> 
> Technically this should be v8::Persistent<T>::New since we don't know that T is v8::Context.

yep.  copy and paste failure.

>> Source/WebCore/bindings/v8/SharedPersistent.h:49
>> +        ~SharedPersistent()
> 
> I thought we were just going to have this class hold a ScopedPersistent rather than a v8::Persistent.  That way we'd get the calls to New and Dispose for free.

This class will be deleted shortly, but I'll change it.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:181
>> +    return reinterpret_cast<V8DOMWindowShell*>(toInnerGlobalObject(v8::Context::GetEntered())->GetPointerFromInternalField(V8DOMWindow::enteredIsolatedWorldIndex));
> 
> reinterpret_cast -> static_cast

ok

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:219
>> +    delete reinterpret_cast<V8DOMWindowShell*>(parameter);
> 
> reinterpret_cast -> static_cast

ok

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:237
>> +        m_context.get().MakeWeak(this, isolatedContextWeakCallback);
> 
> We should probably 0 out m_frame at this point so we don't have a dangling reference to the Frame.

ok

[Dan Carney]

Created attachment 163295 [details]
Patch

[Dan Carney]

Created attachment 163301 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163301 [details]
Patch

Clearing flags on attachment: 163301

Committed r128159: <http://trac.webkit.org/changeset/128159>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.