Remove unnecessary %UnwrapGlobalProxy calls from object-observe.js

Remove unnecessary %UnwrapGlobalProxy calls from object-observe.js

The intent of these calls was to properly key the WeakMap get/set calls
on the underlying global object, not the proxy, since that is the object
actually being observed. But unwrapping at this layer is unnecessary
since GetIdentityHash will already do the unwrapping (via its call to
GetHiddenProperty).

Also remove the runtime function itself, as these were the only callers,
and remove the now-redundant IS_SPEC_OBJECT() checks from object-observe.js's
MapWrapper type.

R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20740

[Toon Verwaest, 2014-04-11 09:50:43]

This patch LGTM (arg, we manually unwrapped the proxy!?).

What you are saying regarding the identity hash sounds wrong to me. That
basically means that if you have a map somewhere, and use a global proxy as a
key (some iframe), you may or may not find it anymore after the iframe
navigates, depending on the new identity hash generated for the new global
object for the new context.

I discussed this shortly with dcarney, and he agrees that we should store the
identity hash on the proxy to ensure that we keep on finding the proxy after
navigation. I thought we could perhaps just copy it from global object to the
new global object, but dcarney mentioned that we do it in 2 steps: first detach,
then reattach. So we don't have the old global object anymore by the time we
attach the new one. Hence the global proxy should have it.

Does that sound reasonable? Could you do this in a follow-up CL?

[adamk, 2014-04-14 19:52:23]

On 2014/04/11 09:50:43, Toon Verwaest wrote:
> This patch LGTM (arg, we manually unwrapped the proxy!?).

Thanks for the quick review (and yeah, I don't have any idea why we did that;
youthful indiscretion, I suppose).

> What you are saying regarding the identity hash sounds wrong to me. That
> basically means that if you have a map somewhere, and use a global proxy as a
> key (some iframe), you may or may not find it anymore after the iframe
> navigates, depending on the new identity hash generated for the new global
> object for the new context.
>
> I discussed this shortly with dcarney, and he agrees that we should store the
> identity hash on the proxy to ensure that we keep on finding the proxy after
> navigation. I thought we could perhaps just copy it from global object to the
> new global object, but dcarney mentioned that we do it in 2 steps: first
detach,
> then reattach. So we don't have the old global object anymore by the time we
> attach the new one. Hence the global proxy should have it.
> 
> Does that sound reasonable? Could you do this in a follow-up CL?

That doesn't sound right to me. After
https://code.google.com/p/v8/source/detail?r=18299, reattaching a global proxy
should never happen, so after navigation the global will never be found in a
map. This seems fine to me: it matches the behavior of properties on the global,
which are gone after navigation.

What am I missing? Is reattachment still possible despite
v8::Context::ReattachGlobal() not being part of the API?

[adamk, 2014-04-14 20:52:35]

Committed patchset #1 manually as r20740 (presubmit successful).

[Toon Verwaest, 2014-04-15 05:57:55]

TL;DR: No, the global proxy is rebound to a new global object after navigation,
and we need to ensure that existing references outside of that context persist
properly. Hence the identity has to stay the same.

The global proxy is the unique object representing a window. When a window
navigates, the proxy stays, but the underlaying global object and context is
swapped for a new global object and new context.

To do so, the existing proxy is passed in when creating this new context. In the
end the proxy is attached to the new context by CreateNewGlobals. The old proxy
is passed in as the (admittedly very confusingly named) "global_object"
parameter (which is actually a global proxy, not a global object).

The global proxy will still be available to other contexts though. The frames
surrounding the navigated frame could have a reference, but the old context that
navigated away still has its own "this" in the global scope, which will be
rebound after navigation to the new page.

Mozilla has some good extra documentation on this:
https://developer.mozilla.org/en-US/docs/SpiderMonkey/Split_object

Of course, since the object persists in various contexts, the identity hash has
to stay the same or stuff will break after navigation. I'll see if I can write a
test for this.

[adamk, 2014-04-15 17:31:02]

On 2014/04/15 05:57:55, Toon Verwaest wrote:
> TL;DR: No, the global proxy is rebound to a new global object after
navigation,
> and we need to ensure that existing references outside of that context persist
> properly. Hence the identity has to stay the same.
> 
> The global proxy is the unique object representing a window. When a window
> navigates, the proxy stays, but the underlaying global object and context is
> swapped for a new global object and new context.
> 
> To do so, the existing proxy is passed in when creating this new context. In
the
> end the proxy is attached to the new context by CreateNewGlobals. The old
proxy
> is passed in as the (admittedly very confusingly named) "global_object"
> parameter (which is actually a global proxy, not a global object).
> 
> The global proxy will still be available to other contexts though. The frames
> surrounding the navigated frame could have a reference, but the old context
that
> navigated away still has its own "this" in the global scope, which will be
> rebound after navigation to the new page.
> 
> Mozilla has some good extra documentation on this:
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/Split_object
> 
> Of course, since the object persists in various contexts, the identity hash
has
> to stay the same or stuff will break after navigation. I'll see if I can write
a
> test for this.

Arg, I get this wrong every single time I try to reason about this. I'm glad
someone besides Adam Barth now has this paged into their head.

[adamk, 2014-04-15 19:05:42]

On 2014/04/15 17:31:02, adamk wrote:
> On 2014/04/15 05:57:55, Toon Verwaest wrote:
> > TL;DR: No, the global proxy is rebound to a new global object after
> navigation,
> > and we need to ensure that existing references outside of that context
persist
> > properly. Hence the identity has to stay the same.
> > 
> > The global proxy is the unique object representing a window. When a window
> > navigates, the proxy stays, but the underlaying global object and context is
> > swapped for a new global object and new context.
> > 
> > To do so, the existing proxy is passed in when creating this new context. In
> the
> > end the proxy is attached to the new context by CreateNewGlobals. The old
> proxy
> > is passed in as the (admittedly very confusingly named) "global_object"
> > parameter (which is actually a global proxy, not a global object).
> > 
> > The global proxy will still be available to other contexts though. The
frames
> > surrounding the navigated frame could have a reference, but the old context
> that
> > navigated away still has its own "this" in the global scope, which will be
> > rebound after navigation to the new page.
> > 
> > Mozilla has some good extra documentation on this:
> > https://developer.mozilla.org/en-US/docs/SpiderMonkey/Split_object
> > 
> > Of course, since the object persists in various contexts, the identity hash
> has
> > to stay the same or stuff will break after navigation. I'll see if I can
write
> a
> > test for this.
> 
> Arg, I get this wrong every single time I try to reason about this. I'm glad
> someone besides Adam Barth now has this paged into their head.

After putting together the minimal test case of this just to remember how it
worked, of course there's reattachment; I was thinking of the lack of
reattachment in the other direction (from a global object -> detached global
object -> reattached global object).

I'm still not convinced of what the right behavior is. In some ways, maps are
analogous to properties as ways of keeping information about in object, and so
losing those associations upon navigation doesn't seem that surprising, since
expando properties are also dropped. I'll have to think more about what problems
this might cause.

[adamk, 2014-04-15 19:20:29]

On 2014/04/15 19:05:42, adamk wrote:
> On 2014/04/15 17:31:02, adamk wrote:
> > On 2014/04/15 05:57:55, Toon Verwaest wrote:
> > > TL;DR: No, the global proxy is rebound to a new global object after
> > navigation,
> > > and we need to ensure that existing references outside of that context
> persist
> > > properly. Hence the identity has to stay the same.
> > > 
> > > The global proxy is the unique object representing a window. When a window
> > > navigates, the proxy stays, but the underlaying global object and context
is
> > > swapped for a new global object and new context.
> > > 
> > > To do so, the existing proxy is passed in when creating this new context.
In
> > the
> > > end the proxy is attached to the new context by CreateNewGlobals. The old
> > proxy
> > > is passed in as the (admittedly very confusingly named) "global_object"
> > > parameter (which is actually a global proxy, not a global object).
> > > 
> > > The global proxy will still be available to other contexts though. The
> frames
> > > surrounding the navigated frame could have a reference, but the old
context
> > that
> > > navigated away still has its own "this" in the global scope, which will be
> > > rebound after navigation to the new page.
> > > 
> > > Mozilla has some good extra documentation on this:
> > > https://developer.mozilla.org/en-US/docs/SpiderMonkey/Split_object
> > > 
> > > Of course, since the object persists in various contexts, the identity
hash
> > has
> > > to stay the same or stuff will break after navigation. I'll see if I can
> write
> > a
> > > test for this.
> > 
> > Arg, I get this wrong every single time I try to reason about this. I'm glad
> > someone besides Adam Barth now has this paged into their head.
> 
> After putting together the minimal test case of this just to remember how it
> worked, of course there's reattachment; I was thinking of the lack of
> reattachment in the other direction (from a global object -> detached global
> object -> reattached global object).
> 
> I'm still not convinced of what the right behavior is. In some ways, maps are
> analogous to properties as ways of keeping information about in object, and so
> losing those associations upon navigation doesn't seem that surprising, since
> expando properties are also dropped. I'll have to think more about what
problems
> this might cause.

A datapoint in favor of storing the hash on the proxy is that Firefox seems to
hold the association across navigations.