Bug 26366 – [V8] Teach V8 bindings about isolated worlds

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Adam Barth]

To make chromium extensions more secure, they want to run extension user scripts in their own "isolated world" with their own DOM wrappers.  This patch teaches V8DOMMap how to keep separate DOM wrappers for different worlds.  (There is a downstream patch that actually hooks this into the extension system.)

Patch forthcoming.

[Adam Barth]

Created attachment 31227 [details]
Patch

I'll add a ChangeLog before landing.  (I created this patch in my Chromium tree, which doesn't have WebKitTools.)

Some preliminary review comments here: http://codereview.chromium.org/118189

[Adam Barth]

The downstream half of this patch is under review here: http://codereview.chromium.org/118188

[Dimitri Glazkov (Google)]

Comment on attachment 31227 [details]
Patch

Holy moly! You rewrote the world! I think I understand what the patch is doing, especially with the Reitveld companion.

My concerns are:

* v8_isolated_world should live upstream, in bindings/v8.
* How does this affect bindings performance?
* Make sure there are no layout test regressions.

Review comments:

No ChangeLog? :)

> +    private:
> +      DOMData* m_domData;

4 spaces?

> +    static void handleWeakObject(DOMDataStore::DOMWrapperMapType mapType,
> +                                 v8::Handle<v8::Object> v8Object,
> +                                 T* domObject);

No need to wrap here -- it's WebKit-land :)

> +DOMDataStoreHandle::DOMDataStoreHandle()
> +    : m_store(new ScopedDOMDataStore(DOMData::getCurrent()))  // TODO: Fix!

FIXME: and What?

> +void visitDOMNodesInCurrentThread(DOMWrapperMap<Node>::Visitor* visitor) {

Brace on new line.

> +void visitDOMObjectsInCurrentThread(DOMWrapperMap<void>::Visitor* visitor) {

Ditto.

> +void visitActiveDOMObjectsInCurrentThread(DOMWrapperMap<void>::Visitor* visitor) {

Ditto.

> +void visitDOMSVGElementInstancesInCurrentThread(DOMWrapperMap<SVGElementInstance>::Visitor* visitor) {

Ditto.

> +void visitSVGObjectsInCurrentThread(DOMWrapperMap<void>::Visitor* visitor) {

Ditto.

[Adam Barth]

(In reply to comment #3)
> Holy moly! You rewrote the world!

Yeah, sorry about that.  It's possible that we could have hacked this in with a smaller change, but it seemed better to separate the concerns more clearly (i.e., a thread as a bundle of isolated wrapper maps).  I'm hoping to spend some more time beautifying this code once we've got Aaron's use cases down.

> * v8_isolated_world should live upstream, in bindings/v8.

Yes.  I'll move this in a follow-up patch.

> * How does this affect bindings performance?

As we discussed on gchat, I'll run some benchmarks on Thursday.

> * Make sure there are no layout test regressions.

I ran the Chromium test_shell_tests in Debug mode and they passed.  If I've screwed things up, the tree will go red and I'll revert.  :)

> No ChangeLog? :)

I'll add a ChangeLog before landing.  I don't have WebKitTools up and running in my Chromium tree yet, but I'll have them when I land through my webkit.org tree.

> > +    private:
> > +      DOMData* m_domData;
> 
> 4 spaces?

Fixed.

> > +    static void handleWeakObject(DOMDataStore::DOMWrapperMapType mapType,
> > +                                 v8::Handle<v8::Object> v8Object,
> > +                                 T* domObject);
> 
> No need to wrap here -- it's WebKit-land :)

Fixed.

> > +DOMDataStoreHandle::DOMDataStoreHandle()
> > +    : m_store(new ScopedDOMDataStore(DOMData::getCurrent()))  // TODO: Fix!
> 
> FIXME: and What?

Oops.  This was a not to myself to test this code.  It's working properly, so I've removed the comment.

> > +void visitDOMNodesInCurrentThread(DOMWrapperMap<Node>::Visitor* visitor) {
> 
> Brace on new line.

Fixed x5.

Thanks!

[Adam Barth]

Created attachment 31319 [details]
updated patch

Here's the updated patch.  I'm not marking this for review until we can run the benchmarks (hopefully Thursday).

[Adam Barth]

Created attachment 31640 [details]
updated patch

This patch passes the benchmarks fine.

[Adam Barth]

To save you some work Dimitri, this patch is virtually identical to second patch (which addressed your comments from the first patch).  The changes are:

1) Now with ChangeLog that doesn't link to the bug.  (I'll fix this before landing.)
2) V8IsolatedWorld is now in this patch instead of the downstream patch.
3) V8IsolatedWorld::getEntered() is now optimized for the "no isolated worlds" case.

[Dimitri Glazkov (Google)]

Comment on attachment 31640 [details]
updated patch

Just a few styling nits:

> +static char kIsolatedWorldKey[] = "IsolatedWorld";

You don't have to have prefixes (k* or g_*) for statics.

> +static int g_isolatedWorldCount = 0;

> +    // Using the default security token means that the canAccess is always
> +    // called, which is slow.
> +    // TODO(aa): Use tokens where possible. This will mean keeping track of all
> +    // created contexts so that they can all be updated when the document domain
> +    // changes.

FIXME?

> +    // TODO(abarth): Move this statement above proxy->evaluate?  It seems like
> +    //               we should set up the token before running the script.

FIXME?

> +V8IsolatedWorld* V8IsolatedWorld::getEntered()
> +        return NULL;

return 0;

> +        return NULL;

ditto.

> +        return NULL;

ditto.

[Adam Barth]

I'll land this once the integration builder is green.  It's been red all day.  :(

[Adam Barth]

This landed, but i forgot to mark the bug as fixed.