Make Object.observe on the global object functional

Make Object.observe on the global object functional

The approach in this change is to handle the unwrapping/wrapping of the global object transparently with respect to the JS implementation of Object.observe. An alternate approach would be to add a runtime method like %IsJSGlobalProxy and %UnwrapJSGlobalProxy, but it seems ugly to give JS (even implementation JS) access to the unwrapped global.

BUG=v8:2409
Committed: https://code.google.com/p/v8/source/detail?r=13142

[rossberg, 2012-11-21 12:43:17]

I think this is the right approach. However, it'd be good to have some API-side
test cases that check things like switching the global receiver. This is
important, I believe, because it might actually be a security leak if it goes
wrong.

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13163
src/runtime.cc:13163: if (obj->IsNull()) return
isolate->heap()->undefined_value();
Perhaps you should return false here.

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13175
src/runtime.cc:13175: JSGlobalProxy* global_proxy = NULL;
What do you need this for?

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13234
src/runtime.cc:13234: if (key->IsNull()) return
isolate->heap()->undefined_value();
Return this.

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13248
src/runtime.cc:13248: if (key->IsNull()) return
isolate->heap()->undefined_value();
False here, too.

https://codereview.chromium.org/11414094/diff/1/test/mjsunit/harmony/object-o...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11414094/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:61: } catch (e) { }
Can you factor this out into a helper function that you use here and below?

[adamk, 2012-11-21 17:02:57]

Will work on API tests for this today. And will try to simulate the way the
global actually works in the browser, too.

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13163
src/runtime.cc:13163: if (obj->IsNull()) return
isolate->heap()->undefined_value();
On 2012/11/21 12:43:17, rossberg wrote:
> Perhaps you should return false here.

Ah, that's probably a better plan, yes.

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13175
src/runtime.cc:13175: JSGlobalProxy* global_proxy = NULL;
On 2012/11/21 12:43:17, rossberg wrote:
> What do you need this for?

Cruft from a previous version before I discovered global_receiver(). Removed.

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13234
src/runtime.cc:13234: if (key->IsNull()) return
isolate->heap()->undefined_value();
On 2012/11/21 12:43:17, rossberg wrote:
> Return this.

Woops, fixed. This is one that will be covered by the API test.

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13248
src/runtime.cc:13248: if (key->IsNull()) return
isolate->heap()->undefined_value();
On 2012/11/21 12:43:17, rossberg wrote:
> False here, too.

This function wasn't carrying its weight, just removed and implemented in JS.

https://codereview.chromium.org/11414094/diff/1/test/mjsunit/harmony/object-o...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11414094/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:61: } catch (e) { }
On 2012/11/21 12:43:17, rossberg wrote:
> Can you factor this out into a helper function that you use here and below?

Done.

[adamk, 2012-11-21 23:27:28]

Added an API test with a call to DetachGlobal. Demonstrates the behavior, but
I'm not at all sure this is how it ought to work...

[rossberg, 2012-11-22 12:17:05]

I think the most important case to test for would be one that reuses a detached
global with a different context. In that state, should one continue to observe
any changes? What if the original context gets destroyed?

I'm not sure how the current implementation would react in that case, because it
seems to be associating the info with the global object proper, not the global
proxy?

I also recall that, in addition to global proxies, the WebKit binding makes
extensive use of hidden prototypes to implement the global object. That's a
similar but different mechanism. We probably need tests for that stuff as well.

To be honest, I'm not entirely sure either what the desired semantics should be
for all these things. Adding Adam Barth to CC, maybe he can shed some light on
the global object madness in WebKit/V8, and how it should interact with
observation. :)

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13248
src/runtime.cc:13248: if (key->IsNull()) return
isolate->heap()->undefined_value();
On 2012/11/21 17:02:57, adamk wrote:
> On 2012/11/21 12:43:17, rossberg wrote:
> > False here, too.
> 
> This function wasn't carrying its weight, just removed and implemented in JS.

I'm fine with that. But note that there no longer is a way to differentiate
between an absent key and a key mapping to 'undefined'. Not that anybody should
ever need that...

[adamk, 2012-11-26 18:43:53]

On 2012/11/22 12:17:05, rossberg wrote:
> I think the most important case to test for would be one that reuses a
detached
> global with a different context. In that state, should one continue to observe
> any changes? What if the original context gets destroyed?
> 
> I'm not sure how the current implementation would react in that case, because
it
> seems to be associating the info with the global object proper, not the global
> proxy?

I've added a test where we take a global proxy and attach it to a different
context, so you can see the behavior. The behavior is that any previous
observations are still attached to the "old" global, which seems right to me
since that's the global which is accessible via "this" after detachment. After
re-attachment, observing the proxy is equivalent to observing the new global.

This all seems pretty logical to me, but I agree that abarth's input on the
security implications would be valuable.

> I also recall that, in addition to global proxies, the WebKit binding makes
> extensive use of hidden prototypes to implement the global object. That's a
> similar but different mechanism. We probably need tests for that stuff as
well.
> 
> To be honest, I'm not entirely sure either what the desired semantics should
be
> for all these things. Adding Adam Barth to CC, maybe he can shed some light on
> the global object madness in WebKit/V8, and how it should interact with
> observation. :)
> 
> https://codereview.chromium.org/11414094/diff/1/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/11414094/diff/1/src/runtime.cc#newcode13248
> src/runtime.cc:13248: if (key->IsNull()) return
> isolate->heap()->undefined_value();
> On 2012/11/21 17:02:57, adamk wrote:
> > On 2012/11/21 12:43:17, rossberg wrote:
> > > False here, too.
> > 
> > This function wasn't carrying its weight, just removed and implemented in
JS.
> 
> I'm fine with that. But note that there no longer is a way to differentiate
> between an absent key and a key mapping to 'undefined'. Not that anybody
should
> ever need that...

[abarth-chromium, 2012-11-26 22:53:07]

https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
test/cctest/test-object-observe.cc:244:
CHECK(inner_global->StrictEquals(CompileRun("records[1].object")));
So, records[1].object no longer points to the global_proxy?  I guess that makes
sense.

https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
test/cctest/test-object-observe.cc:250:
CHECK(global_proxy->StrictEquals(CompileRun("records[2].object")));
What would happen if you grabbed a reference to records[1].object on line 245? 
Would it be strictly equal to global_proxy or inner_global at this point?

[abarth-chromium, 2012-11-26 22:54:02]

adamk and I talked through this topic and his understanding of how things work
on the WebCore side match my understanding.  Thanks for looping me in.  :)

[rossberg, 2012-12-03 13:26:18]

Great, LGTM.

https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
test/cctest/test-object-observe.cc:258: context2->ReattachGlobal(global_proxy);
Is this equivalent to calling Context::New with a given global object? (That's
what the WebKit bindings do, I believe.)

[adamk, 2012-12-03 19:23:20]

https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
test/cctest/test-object-observe.cc:258: context2->ReattachGlobal(global_proxy);
On 2012/12/03 13:26:18, rossberg wrote:
> Is this equivalent to calling Context::New with a given global object? (That's
> what the WebKit bindings do, I believe.)

Added an explicit test for that codepath as it's a bit different.

[rossberg, 2012-12-05 11:52:26]

On 2012/12/03 19:23:20, adamk wrote:
>
https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
> File test/cctest/test-object-observe.cc (right):
> 
>
https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
> test/cctest/test-object-observe.cc:258:
context2->ReattachGlobal(global_proxy);
> On 2012/12/03 13:26:18, rossberg wrote:
> > Is this equivalent to calling Context::New with a given global object?
(That's
> > what the WebKit bindings do, I believe.)
> 
> Added an explicit test for that codepath as it's a bit different.

Excellent, thanks!