Run content scripts in their own isolated world.

Run content scripts in their own isolated world.  Hidden behind the --isolated-world command line argument to let us iterate on this feature.R=aaBUG=12218
TEST=None :(  (I have a testing plan we can put in place once the upstream half of this CL lands.)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19225

[Aaron Boodman, 2009-06-08 20:45:30]

More substantial comments coming. Mostly just sending this to add Mike and Feng,
who know this code better than me.

http://codereview.chromium.org/118188/diff/1008/18
File webkit/port/bindings/v8/v8_isolated_world.cpp (right):

http://codereview.chromium.org/118188/diff/1008/18#newcode46
Line 46: static IsolatedWorldMap& isolated_world_map() {
Elsewhere I've seen these static methods like GetIsolatedWorldMap().

http://codereview.chromium.org/118188/diff/1008/19
File webkit/port/bindings/v8/v8_isolated_world.h (right):

http://codereview.chromium.org/118188/diff/1008/19#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009 alone

http://codereview.chromium.org/118188/diff/1008/19#newcode20
Line 20: class V8IsolatedWorld {
This class could use detailed class-level comments.

[Aaron Boodman, 2009-06-08 22:18:43]

Ok, so I have some high-level questions:

a) You're using V8Proxy::GetEntered() to return the dom wrapper map. I don't
think this is correct because it seems possible for there to be stacks that have
mixes of worlds on them. For example a content script could cause a mutation
event that fires javascript in the page. I think that when C++ code wants a JS
wrapper, they need to provide a context and we need to find the world that goes
with that context. Using GetActive() might also work, as a default?

b) It doesn't look like you've hooked up the various DOM prototypes? (those
should be separate per-world)

c) Is it possible to put asserts someplace that worlds are never bridged?

[abarth-chromium, 2009-06-08 23:05:14]

On 2009/06/08 22:18:43, Aaron Boodman wrote:
> a) You're using V8Proxy::GetEntered() to return the dom wrapper map. I don't
> think this is correct because it seems possible for there to be stacks that
have
> mixes of worlds on them. For example a content script could cause a mutation
> event that fires javascript in the page. I think that when C++ code wants a JS
> wrapper, they need to provide a context and we need to find the world that
goes
> with that context. Using GetActive() might also work, as a default?

I was hoping to avoid altering all the call sites.  Maybe this is something we
should do in a second patch?  The mutation case is pretty narrow.  I'd like to
get something that mostly works, and then we can iterate on getting all the edge
cases right.

> b) It doesn't look like you've hooked up the various DOM prototypes? (those
> should be separate per-world)

I haven't tested this yet.  To test this, we should make sure that
document.body.__proto__ is not the same object when viewed by the page and by
the extension, right?

> c) Is it possible to put asserts someplace that worlds are never bridged?

Yeah.  I'd like to do this.  Is there some way to cache a world identifier with
V8 objects so we can assert that they match at various places?

[Aaron Boodman, 2009-06-09 06:39:36]

On 2009/06/08 23:05:14, abarth wrote:
> On 2009/06/08 22:18:43, Aaron Boodman wrote:
> > a) You're using V8Proxy::GetEntered() to return the dom wrapper map. I don't
> > think this is correct because it seems possible for there to be stacks that
> have
> > mixes of worlds on them. For example a content script could cause a mutation
> > event that fires javascript in the page. I think that when C++ code wants a
JS
> > wrapper, they need to provide a context and we need to find the world that
> goes
> > with that context. Using GetActive() might also work, as a default?
> 
> I was hoping to avoid altering all the call sites.  Maybe this is something we
> should do in a second patch?  The mutation case is pretty narrow.  I'd like to
> get something that mostly works, and then we can iterate on getting all the
edge
> cases right.

That seems reasonable.

> > b) It doesn't look like you've hooked up the various DOM prototypes? (those
> > should be separate per-world)
> 
> I haven't tested this yet.  To test this, we should make sure that
> document.body.__proto__ is not the same object when viewed by the page and by
> the extension, right?

I remembered that I used to have (what I thought was) some pretty clever testing
for content scripts. Here was the change: http://codereview.chromium.org/60112.
I had to pull it out because InProcessBrowserTests caused a lot of pain back
then, but I think I heard that they have recently been fixed?

> > c) Is it possible to put asserts someplace that worlds are never bridged?
> 
> Yeah.  I'd like to do this.  Is there some way to cache a world identifier
with
> V8 objects so we can assert that they match at various places?

We can store arbitrary data with V8 objects via "internal fields". So I guess
that is one way we could do this.

[Mike Belshe, 2009-06-09 16:06:17]

I like the v8proxy refactoring!
I'm not quite sure I understand how the isolated world will be used, but it
looks like this is a stepping stone patch.

A few comments.

http://codereview.chromium.org/118188/diff/1008/18
File webkit/port/bindings/v8/v8_isolated_world.cpp (right):

http://codereview.chromium.org/118188/diff/1008/18#newcode63
Line 63: proxy->createNewContext(v8::Handle<v8::Object>());
Do we really want this static - where you evaluate once in a isolated world and
then never use it again?  (that appears to be what happens here?)  

Won't this lead to perf troubles if an extension calls evaluate() a lot?  

I guess I don't quite know how the isolated world will be used.

http://codereview.chromium.org/118188/diff/1008/18#newcode76
Line 76: // TODO: context->UseDefaultSecurityToken();
more details on this todo.

http://codereview.chromium.org/118188/diff/1008/19
File webkit/port/bindings/v8/v8_isolated_world.h (right):

http://codereview.chromium.org/118188/diff/1008/19#newcode20
Line 20: class V8IsolatedWorld {
On 2009/06/08 20:45:30, Aaron Boodman wrote:
> This class could use detailed class-level comments.
+1

http://codereview.chromium.org/118188/diff/1008/16
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/118188/diff/1008/16#newcode389
Line 389: //EnumerateDOMObjectMap(getDOMObjectMap().impl());
don't forget to re-enable

http://codereview.chromium.org/118188/diff/1008/16#newcode396
Line 396: /*
Dead code here....

http://codereview.chromium.org/118188/diff/1008/16#newcode1009
Line 1009: /*
eliminate the dead code?

[abarth-chromium, 2009-06-09 16:37:25]

Thanks for looking at this Mike.  I have one more round of work to do (mostly to
get the rest of the GC stuff working again), but it's good to know I'm on the
right track.

http://codereview.chromium.org/118188/diff/1008/18
File webkit/port/bindings/v8/v8_isolated_world.cpp (right):

http://codereview.chromium.org/118188/diff/1008/18#newcode63
Line 63: proxy->createNewContext(v8::Handle<v8::Object>());
On 2009/06/09 16:06:18, Mike Belshe wrote:
> Do we really want this static - where you evaluate once in a isolated world
and
> then never use it again?  (that appears to be what happens here?)  
> 
> Won't this lead to perf troubles if an extension calls evaluate() a lot?  
> 
> I guess I don't quite know how the isolated world will be used.

My understanding is that an extension will call this method once per page
(around the time the page loads) and thereafter use postMessage to talk between
the extension and the user script.  If we want to have an "evalInContent" API,
then we'll need a way to find the world for that extension again.

http://codereview.chromium.org/118188/diff/1008/18#newcode76
Line 76: // TODO: context->UseDefaultSecurityToken();
On 2009/06/09 16:06:18, Mike Belshe wrote:
> more details on this todo.

Yeah, I don't quite understand this piece yet.  Aaron, you had this in the
previous version.  Should I just uncomment this, or is there more I need to do? 
(Also, why do we have this after the proxy->evaluate call?)

http://codereview.chromium.org/118188/diff/1008/19
File webkit/port/bindings/v8/v8_isolated_world.h (right):

http://codereview.chromium.org/118188/diff/1008/19#newcode20
Line 20: class V8IsolatedWorld {
On 2009/06/09 16:06:18, Mike Belshe wrote:
> On 2009/06/08 20:45:30, Aaron Boodman wrote:
> > This class could use detailed class-level comments.
> +1

Will do.  :)

http://codereview.chromium.org/118188/diff/1008/16
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/118188/diff/1008/16#newcode396
Line 396: /*
On 2009/06/09 16:06:18, Mike Belshe wrote:
> Dead code here....

Yes.  I need to get this code working again.  My plan is to use the visitor
approach, as above.

http://codereview.chromium.org/118188/diff/1008/16#newcode1009
Line 1009: /*
On 2009/06/09 16:06:18, Mike Belshe wrote:
> eliminate the dead code?

Aaron, do we want both evaluateInNewContext and evaluateInIsolatedWorld, or
should we kill the old version?

[Feng Qian, 2009-06-09 18:59:56]

Hi Adam,

My biggest concern is using Context* as HashMap key. This is error-prone. Please
double check.

I didn't look at details in v8_proxy.cpp before fixing hashmap issue.

Feng

http://codereview.chromium.org/118188/diff/1008/18
File webkit/port/bindings/v8/v8_isolated_world.cpp (right):

http://codereview.chromium.org/118188/diff/1008/18#newcode32
Line 32: #include <v8.h>
should #include "v8_isolated_world.h" go first?

http://codereview.chromium.org/118188/diff/1008/18#newcode44
Line 44: typedef HashMap<v8::Context*, V8IsolatedWorld*> IsolatedWorldMap;
In general, you cannot use a V8 object pointer as hash key because it is a
handle in the implementation. Two context may point to the same implementation
in V8 heap. Double check !!! This may give you unexpected behavior.

What I'd do usually is to find a way (may need V8 api changes) to stick
V8IsolatedWord pointer to that Context object. For example, V8Object -> NPObject
is implemented like this.

http://codereview.chromium.org/118188/diff/1008/19
File webkit/port/bindings/v8/v8_isolated_world.h (right):

http://codereview.chromium.org/118188/diff/1008/19#newcode3
Line 3: // found in the LICENSE file.
Why does this file use a different license header than the cpp file?

[abarth-chromium, 2009-06-09 19:07:15]

Thanks for your comments Feng.

http://codereview.chromium.org/118188/diff/1008/18
File webkit/port/bindings/v8/v8_isolated_world.cpp (right):

http://codereview.chromium.org/118188/diff/1008/18#newcode32
Line 32: #include <v8.h>
On 2009/06/09 18:59:56, Feng Qian wrote:
> should #include "v8_isolated_world.h" go first?

Yep.  Thanks.

http://codereview.chromium.org/118188/diff/1008/18#newcode44
Line 44: typedef HashMap<v8::Context*, V8IsolatedWorld*> IsolatedWorldMap;
On 2009/06/09 18:59:56, Feng Qian wrote:
> In general, you cannot use a V8 object pointer as hash key because it is a
> handle in the implementation. Two context may point to the same implementation
> in V8 heap. Double check !!! This may give you unexpected behavior.

Ah.  Thanks.  This would have been a tough bug to track down.

> What I'd do usually is to find a way (may need V8 api changes) to stick
> V8IsolatedWord pointer to that Context object. For example, V8Object ->
NPObject
> is implemented like this.

Can you give me more details about how to do this?  I think your approach here
is better.

http://codereview.chromium.org/118188/diff/1008/19
File webkit/port/bindings/v8/v8_isolated_world.h (right):

http://codereview.chromium.org/118188/diff/1008/19#newcode3
Line 3: // found in the LICENSE file.
On 2009/06/09 18:59:56, Feng Qian wrote:
> Why does this file use a different license header than the cpp file?

It looks like the CPP file is using a WebKit-style license header.  I'll fix
this.  Thanks.

[abarth-chromium, 2009-06-13 06:47:24]

I think I've addressed all the previous review comments.  Ready for another
round.  :)

Almost all the changes in this CL are hidden behind the --isolate-world command
line switch.

http://codereview.chromium.org/118188/diff/1008/19
File webkit/port/bindings/v8/v8_isolated_world.h (right):

http://codereview.chromium.org/118188/diff/1008/19#newcode3
Line 3: // found in the LICENSE file.
On 2009/06/09 19:07:15, abarth wrote:
> On 2009/06/09 18:59:56, Feng Qian wrote:
> > Why does this file use a different license header than the cpp file?
> 
> It looks like the CPP file is using a WebKit-style license header.  I'll fix
> this.  Thanks.

Turns out this matches v8_proxy.{h,cpp}.  I'm going to leave this.  They'll have
to be converted to WebKit licence when they get upstreamed.

[Aaron Boodman, 2009-06-15 18:48:19]

lgtm.

My comments can be generalized to: please add comments.

I didn't look closely at the GC stuff. I'm not the right person for that.

http://codereview.chromium.org/118188/diff/3016/3019
File webkit/port/bindings/v8/v8_isolated_world.cpp (right):

http://codereview.chromium.org/118188/diff/3016/3019#newcode36
Line 36: 
I think you can kill this extra line break.

http://codereview.chromium.org/118188/diff/3016/3019#newcode44
Line 44: static char kIsolatedWorldKey[] = "IsolatedWorld";
Comment. Sorry :)

http://codereview.chromium.org/118188/diff/3016/3019#newcode91
Line 91: V8IsolatedWorld::~V8IsolatedWorld() {
Useless destructor?

http://codereview.chromium.org/118188/diff/3016/3020
File webkit/port/bindings/v8/v8_isolated_world.h (right):

http://codereview.chromium.org/118188/diff/3016/3020#newcode36
Line 36: static void evaluate(const Vector<ScriptSourceCode>& sources, V8Proxy*
proxy);
Explain how the proxy pointer relates to the new world?

http://codereview.chromium.org/118188/diff/3016/3020#newcode38
Line 38: static V8IsolatedWorld* getEntered();
Maybe add a comment here on 'entered'? We really need to change these names
throughout so we don't have to keep explaining them, but that is separate.

http://codereview.chromium.org/118188/diff/3016/3020#newcode43
Line 43: explicit V8IsolatedWorld(v8::Handle<v8::Context> context);
Would be nice to explain how this context is related to the new world.

http://codereview.chromium.org/118188/diff/3016/3020#newcode45
Line 45: v8::Persistent<v8::Context> m_context;
Members need comments.

[abarth-chromium, 2009-06-16 00:08:34]

On 2009/06/15 18:48:19, Aaron Boodman wrote:
> lgtm.

Thanks Aaron.  I've uploaded a new version of the patch with your comments
addressed.  This is now blocked on running performance benchmarks.