Bug 32386 – [V8] Events created in isolated worlds may fire in main world

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dimitri Glazkov (Google)]

The gist of this change is to add ability to carry world's context (if necessary) in the event listener. IsolatedWorldContext is an abstraction that does it in an intelligent way. Everything else is just gravy to make it work.

IsolatedWorldContext is like String in that it's an easy-copy, holds-on-to-a-ref-counted-load type of guy. The load in this case is the SharedPersistent<v8::Context>, which we grab upon instantiation -- if it's present. If it's not present, IsolatedWorldContext carries nothing.

At the time of firing an event, IsolatedWorldContext provides the right context.

[Dimitri Glazkov (Google)]

Created attachment 44629 [details]
WIP: Fix isolated worlds event story

[Adam Barth]

Did you mean to mark this for review?  It has "WIP" in the name.

[Adam Barth]

+ // FIXME: Move to its own file.

You resolved this FIXME.

This looks good.  I imagine we'll need to change things slightly when we integrate with the JSC isolated worlds implementation, but we don't want to block this patch on that effort.

[Dimitri Glazkov (Google)]

Comment on attachment 44629 [details]
WIP: Fix isolated worlds event story

Whoops sorry. I put this one up to generate test results on my Mac.

[Dimitri Glazkov (Google)]

Created attachment 44634 [details]
Fix isolated worlds event story

[WebKit Review Bot]

Attachment 44634 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/WorldContextHandle.cpp:57:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 1

[Adam Barth]

Comment on attachment 44634 [details]
Fix isolated worlds event story

 47     WorldContextHandle(WorldToUse worldToUse);
 48     v8::Local<v8::Context> adjustedContext(V8Proxy* proxy) const;

Seems like we can omit the parameter names here.

 44     if (V8IsolatedWorld* world = V8IsolatedWorld::getEntered())
 45         m_context = world->sharedContext();

Why do we capture the context this way?  The more natural thing seems like proxy->context()...  I guess the problem is we don't know what frame we're in?  The problem case is where the world->sharedContext() is for a different frame then the one with the event listener.  I'm not sure how to get that to happen at the moment.

 52     if (!m_context || m_context->get().IsEmpty())
 53         return proxy->context();

When do we ever get into this case?  It seems if we're in the UseCurrentWorld case, then m_context shouldn't be empty...

I guess I don't understand why we keep WorldToUse m_worldToUse around at all.  It seems like there are two case:

1) We want the current world, so we captured the context on construction.
2) We want the main world, so we figure that out when asked.

What's the third case where we failed to capture but we still want the current context?

[Adam Barth]

Comment on attachment 44634 [details]
Fix isolated worlds event story

Dimitri explained this to me over chat:

me: there are really three contexts
1) the main world
2) the current world when we contruct
3) the current world when we retrieve

Dimitri: yah!
you said if betterer than I could.

me: i see, we only care about (3) as an optimization to avoid holding handles

===

LGTM once you fix the stylebot nit and the param names.  Thanks so much for working on this.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/51960.