Bug 25706 - Audit V8 bindings for V8Proxy::retrieveFrameForEnteredContext
Summary: Audit V8 bindings for V8Proxy::retrieveFrameForEnteredContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-11 14:40 PDT by Adam Barth
Modified: 2009-05-20 21:57 PDT (History)
5 users (show)

See Also:


Attachments
patch (no tests) (1.30 KB, patch)
2009-05-12 09:47 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
patch (with tests!) (19.39 KB, patch)
2009-05-12 10:56 PDT, Adam Barth
dglazkov: review+
Details | Formatted Diff | Diff
Patch attempt 2 (2.46 KB, patch)
2009-05-20 15:52 PDT, Adam Barth
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-05-11 14:40:01 PDT
In the V8 bindings, we use V8Proxy::retrieveFrameForEnteredContext in a bunch of places were we should be using retrieveFrameForCurrentContext.  We should audit all these call sites and make sure they match JavaScriptCore.  Match in this context is:

retrieveFrameForEnteredContext <-> exec->dynamicGlobalObject
retrieveFrameForCurrentContext <-> exec->lexicalGlobalObject

Getting this right is important for security and subtle for compatibility.
Comment 1 Adam Barth 2009-05-12 09:47:22 PDT
Created attachment 30238 [details]
patch (no tests)

Turns out most of these match JSC.  I think some of the JSC ones are wrong, but I'll deal with that in a separate patch.  This patch makes V8 match JSC.  No tests yet.  (I don't have a machine that can both compile V8 and make LayoutTests.)
Comment 2 Aaron Boodman 2009-05-12 09:50:49 PDT
lgtm, but I am not a reviewer.
Comment 3 Adam Barth 2009-05-12 10:08:10 PDT
Thanks, but the patch isn't quite ready for review.  It lacks tests and Changelog entries.  I'll post an updated version in a bit.
Comment 4 Adam Barth 2009-05-12 10:56:05 PDT
Created attachment 30242 [details]
patch (with tests!)

Here's the complete patch.
Comment 5 Sam Weinig 2009-05-12 11:06:22 PDT
I don't feel comfortable reviewing the code change (as I don't know V8's bindings at all), but the tests look good!
Comment 6 Dimitri Glazkov (Google) 2009-05-12 11:33:23 PDT
Comment on attachment 30242 [details]
patch (with tests!)

Looks good -- can you add bug URL to changelog before landing?
Comment 7 Adam Barth 2009-05-12 12:42:22 PDT
Thanks.  I always forget the bug URL.  :)

I'll land this tonight when I get back to my Mac.
Comment 8 Adam Barth 2009-05-12 20:13:59 PDT
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical-expected.txt
Adding         LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical.html
Adding         LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-write-lexical-expected.txt
Adding         LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-write-lexical.html
Adding         LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-writeln-lexical-expected.txt
Adding         LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-writeln-lexical.html
Sending        WebCore/ChangeLog
Sending        WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
Transmitting file data .........
Committed revision 43604.

Comment 9 Adam Barth 2009-05-20 15:51:11 PDT
Turns out the CurrentContext wasn't right either.  That gave us the context for the write() function itself.
Comment 10 Adam Barth 2009-05-20 15:52:48 PDT
Created attachment 30514 [details]
Patch attempt 2

We actually want the *calling* context.
Comment 11 Adam Barth 2009-05-20 15:53:33 PDT
I should say this is covered by the tests from last time.
Comment 12 Dimitri Glazkov (Google) 2009-05-20 15:56:17 PDT
Comment on attachment 30514 [details]
Patch attempt 2

r=me, but only if it works! :D
Comment 13 Adam Barth 2009-05-20 21:57:48 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
Committed r43958