Ensure that collection iterators' maps have constructors

Ensure that collection iterators' maps have constructors

This fixes JSObject::GetCreationContext() for such iterators.

R=rossberg@chromium.org

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

[Jakob Kummerow, 2014-05-15 13:43:26]

Andreas: PTAL.

Adam, Rafael: FYI. It's unclear whether there are any other objects violating
the assumption in GetCreationContext().

[Jakob Kummerow, 2014-05-15 13:46:40]

+arv FYI, who implemented JS{Map,Set}Iterator

[rossberg, 2014-05-15 13:53:36]

lgtm

LGTM.

@Rafael: to expand on Jakob mentioned is that GetCreationContext

[rossberg, 2014-05-15 14:02:44]

On 2014/05/15 13:53:36, rossberg wrote:
> lgtm
> 
> LGTM.
> 
> @Rafael: to expand on Jakob mentioned is that GetCreationContext

Wrong button. :)

@Rafael: to expand on what Jakob said, the GetCreationContext function you added
assumes that all (JS-side) objects have map()->constructor() set to a
JSFunction. However, it was null for iterators, and we are not quite sure if
that should be considered a bug with iterators or a bug with GetCreationContext
(this CL assumes the former as an easy fix). There might be other objects with a
null constructor. With proxies, you might even encounter some with a
JSFunctionProxy constructor.

It might be good if you could investigate the uses of map->constructor
throughout the system to figure out what's actually the expected invariant.
Michael and I skimmed through them briefly, but it wasn't quite obvious.

[Jakob Kummerow, 2014-05-15 14:14:05]

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

[arv (Not doing code reviews), 2014-05-15 21:36:13]

As long as .constructor does not exist on the __proto__ object this is fine. I
have a CL that tests all of this but it got blocked by the refactoring of
iterators 

See the tests here:

https://codereview.chromium.org/259883002/diff/20001/test/mjsunit/harmony/col...

Once https://codereview.chromium.org/289503002/ lands I'll go back and make sure
that https://codereview.chromium.org/259883002/ still works as expected.

[adamk, 2014-05-16 09:00:05]

On 2014/05/15 14:02:44, rossberg wrote:
> On 2014/05/15 13:53:36, rossberg wrote:
> > lgtm
> > 
> > LGTM.
> > 
> > @Rafael: to expand on Jakob mentioned is that GetCreationContext
> 
> Wrong button. :)
> 
> @Rafael: to expand on what Jakob said, the GetCreationContext function you
added
> assumes that all (JS-side) objects have map()->constructor() set to a
> JSFunction. However, it was null for iterators, and we are not quite sure if
> that should be considered a bug with iterators or a bug with
GetCreationContext
> (this CL assumes the former as an easy fix). There might be other objects with
a
> null constructor. With proxies, you might even encounter some with a
> JSFunctionProxy constructor.
> 
> It might be good if you could investigate the uses of map->constructor
> throughout the system to figure out what's actually the expected invariant.
> Michael and I skimmed through them briefly, but it wasn't quite obvious.

Note that GetCreationContext() wasn't added by Rafael. It was previously part of
the V8 API (added in r7476 by antonm as v8::Object::CreationContext()); Rafael
just moved into into v8::internal::JSObject's interface (to avoid round-tripping
through the API in runtime.cc). So if there are cases where it can fail, those
failures were already exposed to the embedder.

[rossberg, 2014-05-16 09:09:26]

@arv, yes, .constructor is independent of map()->constructor(). The latter
should not be observable externally.

@adamk, ah, thanks for pointing out. We still probably need to investigate this.
I'll discuss with Toon when he is back.