Refactor V8 bindings to allow content scripts to access subframes

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Dan Carney]

see http://code.google.com/p/chromium/issues/detail?id=20773

[Dan Carney]

Created attachment 157527 [details]
initial patch to show direction of change

[Dan Carney]

Comment on attachment 157527 [details]
initial patch to show direction of change

this patch works in the sense that it compiles and the example extension in http://code.google.com/p/chromium/issues/detail?id=20773 works.  It's nowhere near cleaned up though, and I've probably introduced some memory leaks.

[Dan Carney]

Created attachment 157696 [details]
mostly the same as first patch rebased after V8PerContextData rename

[Dan Carney]

Created attachment 160690 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160690 [details]
Patch

Attachment 160690 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13616143

New failing tests:
http/tests/appcache/different-origin-manifest.html
accessibility/aria-checkbox-checked.html
accessibility/aria-labelledby-on-input.html
http/tests/appcache/document-write-html-element.html
animations/3d/matrix-transform-type-animation.html
accessibility/aria-readonly.html
http/tests/appcache/cyrillic-uri.html
accessibility/canvas-fallback-content.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/fail-on-update-2.html
animations/animation-direction-alternate-reverse.html
http/tests/appcache/access-via-redirect.php
accessibility/aria-hidden.html
http/tests/appcache/destroyed-frame.html
http/tests/appcache/disabled.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/aria-scrollbar-role.html
http/tests/appcache/crash-when-navigating-away-then-back.html
canvas/philip/tests/2d.canvas.reference.html
animations/3d/state-at-end-event-transform.html
accessibility/aria-labelledby-stay-within.html
animations/animation-direction-reverse-hardware.html
accessibility/aria-help.html
animations/animation-direction-reverse-fill-mode.html
accessibility/adjacent-continuations-cause-assertion-failure.html
animations/animation-direction-reverse-timing-functions-hardware.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[WebKit Review Bot]

Created attachment 160694 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adam Barth]

Comment on attachment 160690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160690&action=review

Looks like you have some test failures.  This patch looks very promising.  I need to study it in more detail.

Is it possible to make this change in smaller steps?  There's a lot to digest in this patch.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:69
> +    v8::Persistent<v8::Context> localCopy = v8::Persistent<v8::Context>::New(context);

Can we store this Persistent handle as a ScopedPersistent on DOMWrapperWorld?

[Adam Barth]

Comment on attachment 160690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160690&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:338
> +    m_isolatedWorlds.set(worldId, isolatedWorldShell.get());

Can we key m_isolatedWorlds off of DOMWrapperWorld rather than worldId?  I'd like to remove the concept of worldId from WebCore eventually and have that just be a concern of the WebKit-layer (i.e., code in Source/WebKit/chromium).

> Source/WebCore/bindings/v8/ScriptController.cpp:443
> +            isolatedShell = windowShell(isolatedShell->world());
> +            // FIXME: need to set security token here
> +            isolatedShell->initContextIfNeeded();

For example, this work can be in a separate patch.  This is a big behavior change, and it would be nice to make that change separately from all the refactorings that make it possible.

> Source/WebCore/bindings/v8/ScriptController.h:69
>      V8DOMWindowShell* windowShell() const { return m_windowShell.get(); }

We probably want to delete this function and have all the callers use windowShell(mainWold()) or whatever.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:90
> +static v8::Handle<v8::Object> getGlobalObject(v8::Handle<v8::Context> context)
> +{
> +    return v8::Handle<v8::Object>::Cast(context->Global()->GetPrototype());
> +}

getGlobalObject is a bit of a misnomer.  How about toInnerGlobalObject(v8::Handle<v8::Context>)

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:97
> +V8DOMWindowShell* V8DOMWindowShell::isolatedWorldContext()

This function should have the term "entered" in its name somewhere because it's calling v8::Context::GetEntered

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:220
> +    : m_frame(frame),
> +      m_world(world),

These commas go on the next line under the ":".  See Other Punctuation in http://www.webkit.org/coding/coding-style.html

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:298
> +static void isolatedContextWeakCallback(v8::Persistent<v8::Value> object, void* parameter)
> +{
> +    object.Dispose();
> +    reinterpret_cast<V8DOMWindowShell*>(parameter)->deref();
> +}
> +
> +static void registerWeakHandler(v8::Handle<v8::Context> context, V8DOMWindowShell* shell)
> +{
> +    v8::Persistent<v8::Context>::New(context).MakeWeak(shell, isolatedContextWeakCallback);
> +    shell->ref();
> +}

Can we hold this Persistent handle as a ScopedPersistent on V8DOMWindowShell ?  Doesn't it already have a ScopedPersistent to a v8::Context?

[Dan Carney]

(In reply to comment #7)
> (From update of attachment 160690 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160690&action=review
> 
> Looks like you have some test failures.  This patch looks very promising.  I need to study it in more detail.
> 
> Is it possible to make this change in smaller steps?  There's a lot to digest in this patch.

It's not so easy.  There are lots of refactors after this that need to be done, but they all assume the isolated context is gone, and I'm not sure I can really do that in multiple steps.  I moved the patch to https://bugs.webkit.org/show_bug.cgi?id=95735 since it no longer fixes this issue.

[Dan Carney]

Created attachment 175963 [details]
Patch

[Dan Carney]

Comment on attachment 175963 [details]
Patch

This patch currently handles all user scripts except those injected by WebView.addUserScript, which I believe may just be apps, but I'm not sure. All extensions that I invent seem to work correctly.

[Adam Barth]

That's great!  I will study the patch carefully.

Does this mean http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/DOMWrapperWorld.cpp#L85 is now called multiple times per DOMWrapperWorld, based on the number of Frames that the world gets booted up in?  If you've already gone to sleep, I can puzzle that out for myself.

[Dan Carney]

(In reply to comment #12)
> That's great!  I will study the patch carefully.
> 
> Does this mean http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/DOMWrapperWorld.cpp#L85 is now called multiple times per DOMWrapperWorld, based on the number of Frames that the world gets booted up in?  If you've already gone to sleep, I can puzzle that out for myself.

it's pretty early to go to sleep. i'll explain:

i've avoided dealing with the types of worlds that have called makeContextWeak in this patch, so they don't have cross frame access yet.  I'll deal with that in a different patch. I wanted to confer with you on that first. We have 2 options:

get rid of the weak contexts which could mean a lot more mem consumption for whatever actually uses them

do a complicated thing where the a new hidden object in javascript that represents the DOMWrapperWorld has a weak handle on the context and the context a weak handle on the world js object so that they all die at once

I'm inclined to do the second, but it would be a lot easier just to remove a few lines of code than do a bunch of v8 api magic to make this work

wdyt?

[Dan Carney]

Comment on attachment 175963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175963&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:459
> +    // FIXME: Need to handle weak isolated worlds correctly.

This is where i explicit stop the weak contexts from being accessible. Otherwise the frame context or it's parent could die in background and would be reinit'ed with an new window context which would be bad.

[Adam Barth]

Comment on attachment 175963 [details]
Patch

This patch is great and exactly how I'd hoped we'd be able to implement this feature.

My only real comment is stylistic.  I'd add a helper function align the lines of

inline v8::Local<v8::Context> ScriptController::contextForWorld(DOMWrapperWorld* world)
{
    return v8::Local<v8::Context>::New(windowShell(world)->context());
}

since you use that idiom a bunch.

Once this rolls into Chromium, would you be willing to add extension API test so we can test the integration with the extension system?

[Dan Carney]

(In reply to comment #15)
> (From update of attachment 175963 [details])
> This patch is great and exactly how I'd hoped we'd be able to implement this feature.
> 
> My only real comment is stylistic.  I'd add a helper function align the lines of
> 
> inline v8::Local<v8::Context> ScriptController::contextForWorld(DOMWrapperWorld* world)
> {
>     return v8::Local<v8::Context>::New(windowShell(world)->context());
> }

Okay, will add. I was inlining those since i figured the first couple branches at least got hit a lot in the perf tests.

> 
> since you use that idiom a bunch.
> 
> Once this rolls into Chromium, would you be willing to add extension API test so we can test the integration with the extension system?

sure. i've put it on my todo list.

[Adam Barth]

> Okay, will add. I was inlining those since i figured the first couple branches at least got hit a lot in the perf tests.

You can ask the compiler to inline it rather than doing it manually.

[Dan Carney]

Comment on attachment 175963 [details]
Patch

I'll add that inline change tomorrow, to avoid the timezone wait.

[Adam Barth]

Comment on attachment 175963 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 175963 [details]
Patch

Clearing flags on attachment: 175963

Committed r135765: <http://trac.webkit.org/changeset/135765>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r135791%20(4011)/results.html

 http/tests/security/isolatedWorld/world-reuse.html failing on Apple Mac and Windows ports after this patch.

[Adam Barth]

We should probably update the baseline for now and then have Dan look at why it might be failing on JSC.

[Dan Carney]

(In reply to comment #23)
> We should probably update the baseline for now and then have Dan look at why it might be failing on JSC.

Looking into it

[Dan Carney]

(In reply to comment #23)
> We should probably update the baseline for now and then have Dan look at why it might be failing on JSC.

I've submitted a patch that fixes the test to pass on safari and chrome builds.

https://bugs.webkit.org/show_bug.cgi?id=103385

it looks like that if you run evaluateScriptInIsolatedWorld in safari, it's run in the main window's frame's context. If you do the same on chrome, it runs in the context of the current frame.