Ensure that JSProxy::Fix gives the generated JSObject map a constructor

Ensure that JSProxy::Fix gives the generated JSObject map a constructor

All JSObjects in V8 either have a map()->constructor() field or are
JSFunctions. JSProxy::Fix, however, was not enforcing this, and
Object.observe's use of JSObject::GetCreationContext() exposed this.

Note that this is not Object.observe-specific: the API call
v8::Object::CreationContext() also would have revealed this bug.

This patch chooses Object as a reasonable constructor to put on the
newly-fixed object's map. Note that this has no effect on the "constructor"
property in JS. In doing so, I've also tightened up the code underlying
JSProxy::Fix to only support JSObject and JSFunction as possible output
types.

BUG=405844
LOG=N
R=rossberg@chromium.org, verwaest@chromium.org

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

[adamk, 2014-08-26 22:07:23]

adamk@chromium.org changed reviewers:
+ rossberg@chromium.org, verwaest@chromium.org

[adamk, 2014-08-26 22:10:54]

https://codereview.chromium.org/505303004/diff/1/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/505303004/diff/1/src/factory.h#newcode708
src/factory.h:708: void ReinitializeJSProxy(Handle<JSProxy> proxy, InstanceType
type, int size);
I made this private since the only callers are BecomeJSObject() and
BecomeJSFunction(), and making it private lets me assert that those are the only
two types that might have this applied to them.

[Toon Verwaest, 2014-08-27 09:54:35]

lgtm, nice cleanup along the way.

We should get rid of uses of GetCreationContext though. It seems like the API
function CreationContext is currently used all over the place though; even as a
way to get to the isolate(!?). We should work towards removing this constraint.

The problem is that exposing the constructor forces our objects to keep track of
more state than the language itself needs. Currently that's in the map, but that
means that our maps automatically are per-constructor (actual closure). It would
be nice if in the future we could e.g., make maps per-shared-function-info.
Getting there requires more plumbing than just this, but having additional
requirements that aren't even imposed by the language make that even harder.

https://codereview.chromium.org/505303004/diff/1/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/505303004/diff/1/src/factory.cc#newcode1827
src/factory.cc:1827: // context the JSProxy originated in. But that context
isn't stored anywhere.
And I think shouldn't be stored anywhere...

[rossberg, 2014-08-27 11:15:29]

LGTM too

[adamk, 2014-08-27 15:50:38]

On 2014/08/27 at 09:54:35, verwaest wrote:
> lgtm, nice cleanup along the way.
> 
> We should get rid of uses of GetCreationContext though. It seems like the API
function CreationContext is currently used all over the place though; even as a
way to get to the isolate(!?). We should work towards removing this constraint.

It is used in quite a few places in Blink, though I agree that some of them
should be trivially easy to remove. For example, the cases where it's used to
get ahold of the isolate only do so because there's no GetIsolate() on
v8::Object. 

> The problem is that exposing the constructor forces our objects to keep track
of more state than the language itself needs. Currently that's in the map, but
that means that our maps automatically are per-constructor (actual closure). It
would be nice if in the future we could e.g., make maps
per-shared-function-info. Getting there requires more plumbing than just this,
but having additional requirements that aren't even imposed by the language make
that even harder.

I agree that Map::constructor is weird. The very fact that this patch works
exposes that flaw, since there's really no connection between this newly-fixed
object and the "Object" function.

Would you mind filing a bug about removing CreationContext as a concept from the
API?

> https://codereview.chromium.org/505303004/diff/1/src/factory.cc
> File src/factory.cc (right):
> 
> https://codereview.chromium.org/505303004/diff/1/src/factory.cc#newcode1827
> src/factory.cc:1827: // context the JSProxy originated in. But that context
isn't stored anywhere.
> And I think shouldn't be stored anywhere...

[adamk, 2014-08-27 15:54:38]

Committed patchset #1 manually as 23466.