Summary: | [V8] Events created in isolated worlds may fire in main world | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||
Component: | WebCore JavaScript | Assignee: | 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
Dimitri Glazkov (Google)
2009-12-10 11:31:18 PST
Created attachment 44629 [details]
WIP: Fix isolated worlds event story
Did you mean to mark this for review? It has "WIP" in the name. + // 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 on attachment 44629 [details]
WIP: Fix isolated worlds event story
Whoops sorry. I put this one up to generate test results on my Mac.
Created attachment 44634 [details]
Fix isolated worlds event story
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 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 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.
Landed as http://trac.webkit.org/changeset/51960. |