Bug 32386

Summary: [V8] Events created in isolated worlds may fire in main world
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: WebCore JavaScriptAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, caseq, inferno, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
WIP: Fix isolated worlds event story
none
Fix isolated worlds event story abarth: review+

Description Dimitri Glazkov (Google) 2009-12-10 11:31:18 PST
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.
Comment 1 Dimitri Glazkov (Google) 2009-12-10 11:42:34 PST
Created attachment 44629 [details]
WIP: Fix isolated worlds event story
Comment 2 Adam Barth 2009-12-10 11:44:39 PST
Did you mean to mark this for review?  It has "WIP" in the name.
Comment 3 Adam Barth 2009-12-10 11:51:47 PST
+ // 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.
Comment 4 Dimitri Glazkov (Google) 2009-12-10 12:12:39 PST
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.
Comment 5 Dimitri Glazkov (Google) 2009-12-10 12:58:35 PST
Created attachment 44634 [details]
Fix isolated worlds event story
Comment 6 WebKit Review Bot 2009-12-10 13:00:14 PST
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
Comment 7 Adam Barth 2009-12-10 13:09:00 PST
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?
Comment 8 Adam Barth 2009-12-10 13:19:47 PST
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.
Comment 9 Dimitri Glazkov (Google) 2009-12-10 13:43:37 PST
Landed as http://trac.webkit.org/changeset/51960.