Bug 25706 – Audit V8 bindings for V8Proxy::retrieveFrameForEnteredContext

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Adam Barth]

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.

[Adam Barth]

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

[Aaron Boodman]

lgtm, but I am not a reviewer.

[Adam Barth]

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.

[Adam Barth]

Created attachment 30242 [details]
patch (with tests!)

Here's the complete patch.

[Sam Weinig]

I don't feel comfortable reviewing the code change (as I don't know V8's bindings at all), but the tests look good!

[Dimitri Glazkov (Google)]

Comment on attachment 30242 [details]
patch (with tests!)

Looks good -- can you add bug URL to changelog before landing?

[Adam Barth]

Thanks.  I always forget the bug URL.  :)

I'll land this tonight when I get back to my Mac.

[Adam Barth]

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.

[Adam Barth]

Turns out the CurrentContext wasn't right either.  That gave us the context for the write() function itself.

[Adam Barth]

Created attachment 30514 [details]
Patch attempt 2

We actually want the *calling* context.

[Adam Barth]

I should say this is covered by the tests from last time.

[Dimitri Glazkov (Google)]

Comment on attachment 30514 [details]
Patch attempt 2

r=me, but only if it works! :D

[Adam Barth]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
Committed r43958