1055472 - Update builtin constructors to support subclassing

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

All the builtin constructors now require this. They need to be updated to set the prototype chain correctly based on new.target.

(This is nothing to do with @@species, which affects methods.)

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 1: Factor out GetPrototypeFromConstructor, and use it in existing this creation paths.

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 2: Make the Function constructor properly subclassable.

Comment 4

[Eric Faust [:efaust]]

Attachment: Part 3: Make Function.prototype.bind work properly with subclassed Functions

NewFunctionWithSpecialFeaturesAndSpecificProtoCreatedFromThisOneExactSpot(79 arguments)

Comment 5

[Eric Faust [:efaust]]

Attachment: Part 4: Make Object constructor properly subclassable.

The spec is weird here. I don't think this has the behavior they intended with new Proxy(Object, {}), but it's the behavior they specified...

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8672826 [details] [diff] [review]
Part 1: Factor out GetPrototypeFromConstructor, and use it in existing this creation paths.

Review of attachment 8672826 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +1010,5 @@
> +    RootedValue protov(cx);
> +    if (!GetProperty(cx, newTarget, newTarget, cx->names().prototype, &protov))
> +        return false;
> +    if (protov.isObject())
> +        proto.set(&protov.toObject());

Could you null this out if not an object?  Relying on callers to provide a null MH seems un-nice.

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 5: Make the Boolean constructor properly subclassable.

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8672872 [details] [diff] [review]
Part 3: Make Function.prototype.bind work properly with subclassed Functions

Review of attachment 8672872 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +1662,5 @@
>      if (!nameAtom)
>          return false;
>  
> +    RootedObject proto(cx);
> +    if (!GetPrototype(cx, target, &proto))

This is the wrong point to call [[GetPrototypeOf]], this should happen in BoundFunctionCreate in step 4. Our implementation here is somewhat complicated, because we don't want to create the function object before we have its name and length. So please move this under Step 3. and before the observable code that gets .length (Steps 6-7). Sadly we can't test this at the moment, because we don't support getPrototypeOf traps in proxies. This should basically fix bug 1171053.

Comment 9

[Eric Faust [:efaust]]

Attachment: Part 6: Make the Error constructors properly subclassable.

Add a little helper for the cases where the constructor reads "if newTarget is undefined, let newTarget be the active function" or so

Comment 10

[Eric Faust [:efaust]]

Attachment: Part 7: Make the Number constructor properly subclassable.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8672872 [details] [diff] [review]
Part 3: Make Function.prototype.bind work properly with subclassed Functions

Review of attachment 8672872 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +1664,5 @@
>  
> +    RootedObject proto(cx);
> +    if (!GetPrototype(cx, target, &proto))
> +        return false;
> +

Yeah, evilpie's right, this wants to be earlier.

@@ +1996,5 @@
>                                  nullptr, allocKind, newKind);
>  }
>  
>  JSFunction*
> +js::NewNativeConstructorWithGivenProto(JSContext* cx, Native native, unsigned nargs,

Make this file-static until someone else needs this, and get rid of the actually-just-constant arguments in the signature.  Same for the other.

::: js/src/tests/ecma_6/Class/boundFunctionSubclassing.js
@@ +13,5 @@
> +Object.setPrototypeOf(inst, null);
> +bound = Function.prototype.bind.call(inst, {bar:1}, 3);
> +assertEq(Object.getPrototypeOf(bound), null);
> +assertEq(bound(), 4);
> +

Add a test that would fail with the current approach, if we allowed getPrototypeOf to be proxied.

Comment 12

[Eric Faust [:efaust]]

Attachment: Part 8: Make the Date constructor properly subclassable.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8672877 [details] [diff] [review]
Part 4: Make Object constructor properly subclassable.

Review of attachment 8672877 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/Class/superCallBaseInvoked.js
@@ -52,5 @@
>  
>  handler.construct = (target, args, nt) => Reflect.construct(target, args, nt);
>  testBase(p);
>  
> -// Object will have to wait for fixed builtins.

No adding the corresponding test?

Comment 14

[Eric Faust [:efaust]]

Attachment: Part 9: Make RegExp constructor properly subclassable.

This could probably use some more ordering tests. What do you think?

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8673267 [details] [diff] [review]
Part 5: Make the Boolean constructor properly subclassable.

Review of attachment 8673267 [details] [diff] [review]:
-----------------------------------------------------------------

O brave new world, that has such things in't!

Comment 16

[Eric Faust [:efaust]]

Attachment: Part 10: Make the Map constructor properly subclassable.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8673345 [details] [diff] [review]
Part 6: Make the Error constructors properly subclassable.

Review of attachment 8673345 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsexn.cpp
@@ +340,5 @@
>  
> +    // ES6 19.5.1.1 mandates the .prototype lookup happens before the toString
> +    RootedObject proto(cx);
> +    if (!GetPrototypeFromCallableConstructor(cx, args, &proto))
> +        return false;

A test for this would be good, using new Proxy(*Error, { get() { ... } }), and seems pretty easy.  Add one to check ordering.

Comment 18

[Eric Faust [:efaust]]

Attachment: Part 11: Make the Set constructor properly subclassable.

Comment 19

[Eric Faust [:efaust]]

Attachment: Part 12: Make the WeakMap constructor properly subclassable.

Comment 20

[Eric Faust [:efaust]]

Attachment: Part 13: Make the WeakSet constructor properly subclassable.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8673395 [details] [diff] [review]
Part 9: Make RegExp constructor properly subclassable.

Review of attachment 8673395 [details] [diff] [review]:
-----------------------------------------------------------------

RegExpObjectBuilder is a hot mess that greatly obscures the algorithm, making quite unclear whether spec compliance is achieved.  So I wrote some patches to get rid of it (and incidentally, and somewhat surprisingly, remove 50 lines of code).  Those are in bug 1215430, and I'd like to see this work done atop them.

Incidentally, those patches point out a bug you'll introduce, in the case of |new RegExpSubclass(regex, { toString() { ... } })| performing toString before accessing .constructor.  I'd like to see that resolved (with testcase) in the ultimate patch here, so the XXX comment dies a quick death.

::: js/src/tests/ecma_6/Class/extendBuiltinConstructors.js
@@ +30,5 @@
>  testBuiltin(Date, 5);
>  testBuiltin(Date, 5, 10);
> +testBuiltin(RegExp);
> +testBuiltin(RegExp, /cow/);
> +testBuiltin(RegExp, "This is a regexp");

Uh, maybe "string argument used to create regexp" or something?

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 8673400 [details] [diff] [review]
Part 10: Make the Map constructor properly subclassable.

Review of attachment 8673400 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/MapObject.cpp
@@ +421,3 @@
>  
>      ValueMap* map = cx->new_<ValueMap>(cx->runtime());
>      if (!map || !map->init()) {

Actually, while you're here.  Could you flip the ordering here, so we don't have to contemplate whether it's okay to have a MapObject with unset private?

  auto map = cx->make_unique<ValueMap>(cx->runtime());
  if (!map || !map->init()) {
    ReportOutOfMemory(cx);
    return nullptr;
  }

  JSObject* mapObj = NewObjectWithClassProto(cx, &class_, proto);
  if (!mapObj)
    return nullptr;

  mapObj->setPrivate(map.release());
  return &mapObj->as<MapObject>();

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 8673410 [details] [diff] [review]
Part 11: Make the Set constructor properly subclassable.

Review of attachment 8673410 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/MapObject.cpp
@@ +1077,2 @@
>          return nullptr;
> +    SetObject* obj = &setObj->as<SetObject>();

Same sort of ordering comment as for MapObject.

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 8673419 [details] [diff] [review]
Part 13: Make the WeakSet constructor properly subclassable.

Review of attachment 8673419 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/WeakSetObject.cpp
@@ +68,1 @@
>          return nullptr;

As before, reorder?

@@ +72,1 @@
>      RootedObject map(cx, JS::NewWeakMapObject(cx));

I'd prefer if this were the method, inlined, for greater clarity.

Comment 25

[Eric Faust [:efaust]]

Attachment: Make the various TypedArrayConstructors properly subclassable.

Also add an "IsDetached" check in the TypedArray(typedArray) constructor, to improve spec compliance and account for possible neuterings in the new get.

Comment 26

[Eric Faust [:efaust]]

Attachment: Part 14: Make the various TypedArray constructors properly subclassable.

Oops, also add the check for the TypedArray(buffer) case.

Comment 27

[Eric Faust [:efaust]]

Looking at Shared*Buffer with respect to this, I have a few questions. Lars, hopefully you can help me out.

1) It looks like you can't neuter (or detach, in the modern parlance) a SharedArrayBuffer. Is this true?

2) Waldo pointed me at draft spec language for SharedArrayBuffer and the shared TypedArray variants, but there didn't appear to be language there about prototypes or subclassing. I assume that the intention is for them to be subclassable. Is there more modern language that I should look at, or should I just make up a spot to do the lookup, as seems appropriate?

Comment 28

[Lars T Hansen [:lth]]

(In reply to Eric Faust [:efaust] from comment #27)
> Looking at Shared*Buffer with respect to this, I have a few questions. Lars,
> hopefully you can help me out.
> 
> 1) It looks like you can't neuter (or detach, in the modern parlance) a
> SharedArrayBuffer. Is this true?

At the moment that is true, and I expect it to remain that way - nobody has expressed a desire for anything else and it'd be hard to implement.

> 2) Waldo pointed me at draft spec language for SharedArrayBuffer and the
> shared TypedArray variants, but there didn't appear to be language there
> about prototypes or subclassing. I assume that the intention is for them to
> be subclassable. Is there more modern language that I should look at, or
> should I just make up a spot to do the lookup, as seems appropriate?

The SharedTypedArray types are all in the process of being removed.  Instead, standard (ie very lightly modified from ES6) TypedArray views will be used on shared memory.  This is bug 1172614, which I'm actively working on (big job, hard to land piecemeal unfortunately).

In any case, the current ES6-compatible/style spec is here: http://lars-t-hansen.github.io/ecmascript_sharedmem/shmem.html

There's a bug tracker attached to the parent repo for that page: https://github.com/lars-t-hansen/ecmascript_sharedmem

Feel free to file spec bugs/issues there, should you come across any.

Comment 29

[Eric Faust [:efaust]]

Attachment: Part 15: Make the DataView constructor properly subclassable.

Man, this is ugly. Again, add some neutered checks to meet spec.

Most of the pain is in refactoring around the wrapper case, because we have to do the check /after/ we throw on lengths.

This also rewrites the length checks to match the current spec.

Comment 30

[Eric Faust [:efaust]]

Attachment: Part 9: Make RegExp constructor properly subclassable. (rebased)

Comment 31

[Eric Faust [:efaust]]

Attachment: Part 8.5: Rename InitializeRegExp to RegExpObject::initFromAtom for readability.

This should have gone up before the rebased step 9. My apologies.

Comment 32

[Eric Faust [:efaust]]

Attachment: Part 16: Make the String constructor properly subclassable.

Comment 33

[André Bargull [:anba]]

Comment on attachment 8682263 [details] [diff] [review]
Part 9: Make RegExp constructor properly subclassable. (rebased)

Review of attachment 8682263 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.cpp
@@ +353,5 @@
> +        // Step 5. In this case, we can reorder this parsing /after/ the object
> +        // creation. Step 5a and 5b are spec idempotent, and the ToString behavior
> +        // we want to invoke as part of step 5c is actually in step 10. As getting
> +        // this out of order with the |.constructor| access from step 8 is visible
> +        // to script, our hand is forced.

Drive-by comment: Steps 8-9 are not side-effect free, therefore it's not valid to move 8-9 before 5.a-b.

var re = /a/;
var newRe = Reflect.construct(RegExp, [re], Object.defineProperty(function(){}.bind(null), "prototype", {
  get() {
    re.compile("b");
    return RegExp.prototype;
  }
}));
assertEq(newRe.source, "a");

Comment 34

[Eric Faust [:efaust]]

Attachment: Part 9: Make RegExp constructor properly subclassable. (rebased, and corrected)

Faust tested, Andre approved. Now with a few more tests.

Comment 35

[Jeff Walden [:Waldo]]

Comment on attachment 8682265 [details] [diff] [review]
Part 8.5: Rename InitializeRegExp to RegExpObject::initFromAtom for readability.

Review of attachment 8682265 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.cpp
@@ +700,5 @@
>          return nullptr;
>      proto->NativeObject::setPrivate(nullptr);
>  
>      RootedAtom source(cx, cx->names().empty);
> +    if (!RegExpObject::initFromAtom(cx, proto, source, RegExpFlag(0)))

Use cx->names().empty directly here, if you can.  I think you can.

Comment 36

[Jeff Walden [:Waldo]]

Comment on attachment 8682777 [details] [diff] [review]
Part 9: Make RegExp constructor properly subclassable. (rebased, and corrected)

Review of attachment 8682777 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.cpp
@@ +310,4 @@
>  
> +    // We can delay step 3 and step 4a until later, during
> +    // GetPrototypeFromCallableConstructor calls. Accessing the new.target
> +    // and the callee from the stack is not side-effectful.

Could do "unobservable" rather than "not side-effectful".

@@ +367,5 @@
>              return false;
>  
>          // Step 10.
> +        if (args.hasDefined(1)) {
> +            // Step 5c / 21.2.3.2.2 step 5 (sub-step of the RegExpInitialize call).

Maybe

  // Step 5c, 21.2.3.2.2 RegExpInitialize step 5.

::: js/src/tests/ecma_6/RegExp/constructor-ordering-2.js
@@ +10,5 @@
> +  get() {
> +    didLookup = true;
> +    return RegExp.prototype;
> +  }
> +}));

Might as well assertEq(Object.getPrototypeOf(newRe), RegExp.prototype), because why not.  And definitely assertEq(didLookup, true).

::: js/src/tests/ecma_6/RegExp/constructor-ordering.js
@@ +1,4 @@
> +// Make sure that we don't misorder subclassing accesses with respect to
> +// accessing regex arg internal slots
> +//
> +// Test credit Andre Bargul.

"that sick devil André Bargull" ;-)

(Okay, the accent is serious.  :-) )

Comment 37

[Jeff Walden [:Waldo]]

Comment on attachment 8682266 [details] [diff] [review]
Part 16: Make the String constructor properly subclassable.

Review of attachment 8682266 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineIC.cpp
@@ +8557,1 @@
>          return !!res;

I'd prefer

  StringObject* strobj = ...;
  if (!strobj)
    return false;

  res.set(strobj);
  return true;

myself.

Comment 38

[Eric Faust [:efaust]]

Attachment: Part 17: Make the Array constructor properly subclassable. \o/

There's a bunch of TI churn here. I want Brian to take a once over the changes to ObjectGroup.

Comment 39

[Brian Hackett [Laid off!]]

Comment on attachment 8685192 [details] [diff] [review]
Part 17: Make the Array constructor properly subclassable. \o/

Review of attachment 8685192 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ObjectGroup.cpp
@@ +1362,5 @@
>  
>      uint32_t offset : 24;
>      JSProtoKey kind : 8;
>  
> +    JSObject* proto;

The TI changes in this patch look good, but adding the proto pointer here means that a post barrier or GC phase is needed which keeps the proto alive during GGC and rekeys the hash table.  ObjectGroupCompartment::fixupNewTableAfterMovingGC should be a good example for how to do this.

@@ +1388,5 @@
>      uint32_t offset = script->pcToOffset(pc);
>  
> +    if (offset >= ObjectGroupCompartment::AllocationSiteKey::OFFSET_LIMIT) {
> +        if (protoArg)
> +            return defaultNewGroup(cx, GetClassForProtoKey(kind), TaggedProto(protoArg));

Is it possible for a subclassed array to have a NULL proto?

Comment 40

[Eric Faust [:efaust]]

(In reply to Brian Hackett (:bhackett) from comment #39)
> Comment on attachment 8685192 [details] [diff] [review]
> Part 17: Make the Array constructor properly subclassable. \o/
> 
> Review of attachment 8685192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/ObjectGroup.cpp
> @@ +1362,5 @@
> >  
> >      uint32_t offset : 24;
> >      JSProtoKey kind : 8;
> >  
> > +    JSObject* proto;
> 
> The TI changes in this patch look good, but adding the proto pointer here
> means that a post barrier or GC phase is needed which keeps the proto alive
> during GGC and rekeys the hash table. 
> ObjectGroupCompartment::fixupNewTableAfterMovingGC should be a good example
> for how to do this.
> 

Even though we ensure that the stored proto is tenured? Shouldn't that keep GGC at bay? I was under the impression that the if (!gc::IsInsideNursery(protoArg)) check would do it for us.

> @@ +1388,5 @@
> >      uint32_t offset = script->pcToOffset(pc);
> >  
> > +    if (offset >= ObjectGroupCompartment::AllocationSiteKey::OFFSET_LIMIT) {
> > +        if (protoArg)
> > +            return defaultNewGroup(cx, GetClassForProtoKey(kind), TaggedProto(protoArg));
> 
> Is it possible for a subclassed array to have a NULL proto?

Nope, if the .prototype of the new.target is not an object, then %ArrayPrototype% is used.

Comment 41

[Brian Hackett [Laid off!]]

Comment on attachment 8685192 [details] [diff] [review]
Part 17: Make the Array constructor properly subclassable. \o/

Review of attachment 8685192 [details] [diff] [review]:
-----------------------------------------------------------------

If the proto is guaranteed to be tenured then the post barrier / table fixup stuff isn't needed, though I don't see any guarantees to that effect in this patch.  There should be isTenured() assertions on the prototype in ObjectGroup::allocationSiteGroup anyhow.

Comment 42

[Eric Faust [:efaust]]

Comment on attachment 8685192 [details] [diff] [review]
Part 17: Make the Array constructor properly subclassable. \o/

Review of attachment 8685192 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ObjectGroup.cpp
@@ +1395,2 @@
>  
> +    if (protoArg && gc::IsInsideNursery(protoArg))

Is this check not sufficient? Is there something else I should be checking?

Comment 43

[Brian Hackett [Laid off!]]

(In reply to Eric Faust [:efaust] from comment #42)
> Comment on attachment 8685192 [details] [diff] [review]
> Part 17: Make the Array constructor properly subclassable. \o/
> 
> Review of attachment 8685192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/ObjectGroup.cpp
> @@ +1395,2 @@
> >  
> > +    if (protoArg && gc::IsInsideNursery(protoArg))
> 
> Is this check not sufficient? Is there something else I should be checking?

Oops, yes, this check is sufficient, I don't know why I didn't notice it.  Sorry for the noise.

Comment 44

[Eric Faust [:efaust]]

Attachment: Part 15: Make the DataView constructor properly subclassable.

Whoops! Make sure that we actually set the rval of the cross compartment machinery, a step initially lost during the refactor.

Comment 45

[Jason Orendorff [:jorendorff]]

Comment on attachment 8685192 [details] [diff] [review]
Part 17: Make the Array constructor properly subclassable. \o/

Review of attachment 8685192 [details] [diff] [review]:
-----------------------------------------------------------------

Pretty straightforward, but this needs more tests.

Test that (where not already adequately tested in other tests):
* objects created this way are arrays (Array.isArray; also test some non-generic method or the .length behavior)
* given `class Foo extends Array {}`, the implicit constructor passes arguments through to Array.[[Construct]] as desired.
* given `class Bar extends Foo {}`, Foo's implicit constructor passes new.target=Bar through to Array.[[Construct]] as desired when `new Bar` is called.
* same thing, but using `Reflect.construct(Foo, [], c)` rather than Bar's constructor to populate new.target.

Comment 46

[Jason Orendorff [:jorendorff]]

Comment on attachment 8686421 [details] [diff] [review]
Part 15: Make the DataView constructor properly subclassable.

Review of attachment 8686421 [details] [diff] [review]:
-----------------------------------------------------------------

(Stealing review from Jeff again --- I bet he won't mind.)

::: js/src/tests/ecma_6/Class/extendBuiltinConstructors.js
@@ +55,5 @@
>  testBuiltin(WeakMap);
>  testBuiltin(WeakSet);
>  testBuiltin(ArrayBuffer);
>  testBuiltinTypedArrays();
> +testBuiltin(DataView, new ArrayBuffer());

Might as well test the cross-global case, since it is such a pain:

testBuiltin(DataView, new otherGlobal.ArrayBuffer());

::: js/src/vm/ArrayBufferObject.cpp
@@ +893,5 @@
>  
>      /*
>       * This method is only called for |DataView(alienBuf, ...)| which calls
>       * this as |createDataViewForThis.call(alienBuf, ..., DataView.prototype)|,
>       * ergo there must be at least two arguments.

Fix the comment to match the code.

::: js/src/vm/TypedArrayObject.cpp
@@ +1104,5 @@
>          }
>  
> +        if (args.get(2).isUndefined()) {
> +            uint32_t bufferLength = buffer->byteLength();
> +            byteLength = bufferLength - byteOffset;

Nit: kill the temporary. If you want, you can just write

    byteLength -= byteOffset;

@@ +1117,2 @@
>  
> +            if (byteOffset + byteLength > buffer->byteLength()) {

Before this last line, please assert that the addition is valid.

    MOZ_ASSERT(byteOffset + byteLength > byteOffset, "can't overflow: both numbers are less than INT32_MAX");

This code has to change more eventually, since it's not ES6-compliant at least in the details of coercion. So we might as well have the "watch out!" sign in place.

@@ +1159,5 @@
>  
>  bool
> +DataViewObject::constructWrapped(JSContext* cx, JSObject* bufobj, const CallArgs& args)
> +{
> +    MOZ_ASSERT(args.isConstructing());

Comment for this method:

// Create a DataView object in another compartment.
//
// ES6 supports creating a DataView in global A (using global A's DataView
// constructor) backed by an ArrayBuffer created in global B.
//
// Our DataViewObject implementation doesn't support a DataView in
// compartment A backed by an ArrayBuffer in compartment B. So in this case,
// we create the DataView in B (!) and return a cross-compartment wrapper.
//
// Extra twist: the spec says the new DataView's [[Prototype]] must be
// A's DataView.prototype. So even though we're creating the DataView in B,
// its [[Prototype]] must be (a cross-compartment wrapper for) the
// DataView.prototype in A.
//
// Not confusing enough? The way we actually do this is also tricky.
// We call compartment A's createDataViewForThis method, passing it bufobj
// as `this`. That calls ArrayBufferObject::createDataViewForThis(),
// which uses CallNonGenericMethod to switch to compartment B so that
// the new DataView is created there.

@@ +1164,5 @@
> +    MOZ_ASSERT(bufobj->is<WrapperObject>());
> +
> +    JSObject* unwrapped = CheckedUnwrap(bufobj);
> +    if (!unwrapped) {
> +        JS_ReportError(cx, "Permission denied to access object");

Use JSMSG_UNWRAP_DENIED?

@@ +1177,5 @@
> +    // And all for this! We do a similar dance that ArrayBuffer does,
> +    // crating the view in the same compartment as the buffer. We need to
> +    // ensure, though, that the prototype comes from this compartment,
> +    // so set that up before leveraging invoke to get all the cross-
> +    // compartment machinery.

This comment can be trimmed if you take my essay...

Comment 47

[Eric Faust [:efaust]]

Attachment: Part 18: Make the ArrayBuffer constructor properly subclassable.

Comment 48

[Jeff Walden [:Waldo]]

Comment on attachment 8687445 [details] [diff] [review]
Part 18: Make the ArrayBuffer constructor properly subclassable.

Review of attachment 8687445 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ArrayBufferObject.cpp
@@ +833,5 @@
>      gc::AllocKind allocKind = GetGCObjectKind(nslots);
>  
>      AutoSetNewObjectMetadata metadata(cx);
> +    JSObject* buffObj = NewObjectWithClassProto(cx, &class_, proto, allocKind, newKind);
> +    if (!buffObj) {

I'm going to assume we don't now/yet have something templaty that returns an ABO* here.

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69d9fb855787
https://hg.mozilla.org/integration/mozilla-inbound/rev/67f0802a5c13
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e58e0e479b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd09d5077094
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2b91587001
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec1640cdfd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/03d708347ebb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3975d948208
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e968e8a279a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f77faf418ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd35ea9ef38
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb9403a345c
https://hg.mozilla.org/integration/mozilla-inbound/rev/42d1ecbce781
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b8fa139568b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce74f9a7b479
https://hg.mozilla.org/integration/mozilla-inbound/rev/f42360dbd545
https://hg.mozilla.org/integration/mozilla-inbound/rev/40919fcffecd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c454e1ac50c
https://hg.mozilla.org/integration/mozilla-inbound/rev/738e23a218c8

Comment 50

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9b54b8ed53 for hazards, http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-inbound/sha512/bf8319a67e00ba872b7908b5265daf56391d5d85e203f81e6436df85360d6b8a793a5947813960db1b8dc7ba324d8c4f394c6d69e0cd6e3188dca2dfcc36c377, and jsreftest failures, https://treeherder.mozilla.org/logviewer.html#?job_id=17274350&repo=mozilla-inbound, and shell failures, https://treeherder.mozilla.org/logviewer.html#?job_id=17273268&repo=mozilla-inbound

Comment 51

[Tooru Fujisawa [:arai]]

In Array subclass, prototype property access is optimized somehow wrongly in baseline, following testcase fails with rev 738e23a218c8, there |v.prop| returns "A" for SubArrayB instance that expects "B".  TypedArrays and DataView also have same issue.  similar case is hit by @@species patch, while accessing |this.constructor|.  (might be better to file a separated bug for this?)

function f(v, expected) {
  assertEq(v.prop, expected);
};

class SubArrayA extends Array {
}
class SubArrayB extends Array {
}
SubArrayA.prototype.prop = "A";
SubArrayB.prototype.prop = "B";

var a = new SubArrayA();
var b = new SubArrayB();
for (let i = 0; i < 10; i++) {
  f(a, "A");
  f(b, "B");
}

Comment 52

[Eric Faust [:efaust]]

Comment on attachment 8685192 [details] [diff] [review]
Part 17: Make the Array constructor properly subclassable. \o/

Review of attachment 8685192 [details] [diff] [review]:
-----------------------------------------------------------------

We've gotten ourselves confused on IRC. Best to make sure.

Comment 53

[Terrence Cole [:terrence]]

Comment on attachment 8685192 [details] [diff] [review]
Part 17: Make the Array constructor properly subclassable. \o/

Review of attachment 8685192 [details] [diff] [review]:
-----------------------------------------------------------------

Ask on irc if you have any questions about this.

::: js/src/vm/ObjectGroup.cpp
@@ +1358,4 @@
>  /////////////////////////////////////////////////////////////////////
>  
>  struct ObjectGroupCompartment::AllocationSiteKey : public DefaultHasher<AllocationSiteKey> {
>      JSScript* script;

I guess that this script is a bare pointer because we know that reads cannot escape. I'd suggest making this a read barriered so that future changes do not accidentally violate this property: the cost should be fairly minimal.

@@ +1362,5 @@
>  
>      uint32_t offset : 24;
>      JSProtoKey kind : 8;
>  
> +    JSObject* proto;

As of last Friday we can actually do this fairly automatically. First, make this member into a ReadBarrieredObject. This will add automatic post barriers so that the pointer gets updated during minor GC.

@@ +1369,5 @@
>  
>      AllocationSiteKey() { mozilla::PodZero(this); }
>  
>      static inline uint32_t hash(AllocationSiteKey key) {
> +        return uint32_t(size_t(key.script->offsetToPC(key.offset)) ^ key.kind ^ size_t(key.proto));

Now that ReadBarrieredObject is changing the proto's value without the HashTable being notified, it's important that both the hash code and identity do not depend on the proto address. Use:

MovableCellHasher<JSObject*>::hash(key.proto)

to get a safe, stable hash code here. You can just xor it in like you are currently doing with the raw value of the proto address.

@@ +1374,4 @@
>      }
>  
>      static inline bool match(const AllocationSiteKey& a, const AllocationSiteKey& b) {
> +        return a.script == b.script && a.offset == b.offset && a.kind == b.kind && a.proto == b.proto;

Thirdly, the object identity must also be stable. Use MovableCellHasher<JSObject*>::match(a.proto, b.proto) instead of ==.

@@ +1395,2 @@
>  
> +    if (protoArg && gc::IsInsideNursery(protoArg))

This should not be necessary.

@@ +1783,5 @@
>      if (allocationSiteTable) {
>          for (AllocationSiteTable::Enum e(*allocationSiteTable); !e.empty(); e.popFront()) {
>              AllocationSiteKey key = e.front().key();
> +            bool keyDying = IsAboutToBeFinalizedUnbarriered(&key.script) ||
> +                            (key.proto && IsAboutToBeFinalizedUnbarriered(&key.proto));

Now that these are barriered, you should get a compile error if you do not remove the "Unbarriered".

@@ +1794,2 @@
>                  e.rekeyFront(key);
> +            }

You can now safely remove the rekey branch.

Comment 54

[Eric Faust [:efaust]]

Attachment: Followup 1: Upadte the GC interactions of ObjectGroupCompartment::AllocationSiteKey

Comment 55

[Eric Faust [:efaust]]

Attachment: Followup 2: Thwart DataView and TypedArray object creation paths seemingly obsessive incorrectness vis a vis the NewObjectCache.

I don't understand why we did later group smashing instead of just allocating the object outright. This passes all tests. The old way failed arai's test above (refitted for TypedArray or DataView instead of Array) for similar reasons: The NewBuiltinClassInstance calls were returning objects with identical shapes.

Since I couldn't understand how the new groups were better than the old, I removed them. If this is incorrect, let me know.

Comment 56

[Eric Faust [:efaust]]

Attachment: Followup 2: Thwart DataView and TypedArray object creation paths seemingly obsessive incorrectness vis a vis the NewObjectCache.

Fix stupid oom bug.

Comment 57

[Eric Faust [:efaust]]

Attachment: Followup 2: Thwart DataView and TypedArray object creation paths seemingly obsessive incorrectness vis a vis the NewObjectCache.

Remove unrelated bugfix from patch. Sorry for the noise.

Comment 58

[Terrence Cole [:terrence]]

Comment on attachment 8688743 [details] [diff] [review]
Followup 1: Upadte the GC interactions of ObjectGroupCompartment::AllocationSiteKey

Review of attachment 8688743 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 59

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38cf9e26c832
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad5599d97bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3efaea09b45f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f83ba89eb48
https://hg.mozilla.org/integration/mozilla-inbound/rev/021a2bf010be
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e04101582c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3eee2d8c90e
https://hg.mozilla.org/integration/mozilla-inbound/rev/df80bd4dbb44
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d4a1ae22fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/16d85567a5ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef23d40ed0d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2f4666560f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f81eb5d97ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c24c5a4b85e
https://hg.mozilla.org/integration/mozilla-inbound/rev/150c6494dc11
https://hg.mozilla.org/integration/mozilla-inbound/rev/bab58078551b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2903bab1d3d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/740977ceab24
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4006cfea7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0389acea3fc7

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out parts 17 and 18 in https://hg.mozilla.org/integration/mozilla-inbound/rev/70f41cd98857 to hopefully fix the build bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=17479459&repo=mozilla-inbound

Comment 61

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/191079ef76d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/74aaf5ce4f7b

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c940e393031

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Everything's been backed out in the following commits to get the tree back to a known less-bad state:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd71e6e0f976
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0ddffeb2d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/259854424b8e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4618d76a09f0

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ec3005245c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f593ea362c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4f7a838a66
https://hg.mozilla.org/integration/mozilla-inbound/rev/a47a3ce6f35e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7180ea9dfa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1509efcfa629
https://hg.mozilla.org/integration/mozilla-inbound/rev/94135702e1b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/65d962413c98
https://hg.mozilla.org/integration/mozilla-inbound/rev/284934443cd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/69bf1faa9d85
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8f9604c34f
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c41d6a16ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd0b1fff545
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a4144b96fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/74dca890ec34
https://hg.mozilla.org/integration/mozilla-inbound/rev/33d5c8ef947c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f0b0b246e25
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed32cadfc31
https://hg.mozilla.org/integration/mozilla-inbound/rev/41be086be0e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/edd1c18b5a5b

Comment 65

[Phil Ringnalda (:philor)]

This go-round we have the fathomable Linux64 cgc https://treeherder.mozilla.org/logviewer.html#?job_id=17566036&repo=mozilla-inbound, and the unfathomable but so far apparent jemalloc crash in the cpp test test_audio only on 10.6 debug, https://treeherder.mozilla.org/logviewer.html#?job_id=17579218&repo=mozilla-inbound

Comment 66

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/652bd59cdb51

Comment 68

[Hannes Verschore [:h4writer]]

octane e-b regression: Bug 1055472 - Part 17: Make the Array constructor properly subclassable....
https://arewefastyet.com/regressions/#/regression/1799698

octane mandreel regression: Bug 1055472 - Part 14: Make the various TypedArray constructors properly subclass....
https://arewefastyet.com/regressions/#/regression/1799699

Comment 69

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e53bad3e7559
https://hg.mozilla.org/integration/mozilla-inbound/rev/695d86baf80e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9325b0e173
https://hg.mozilla.org/integration/mozilla-inbound/rev/58937c535d90
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c0779e0ecc
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d87e6e1064
https://hg.mozilla.org/integration/mozilla-inbound/rev/628c1d07a0e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1086bc9744
https://hg.mozilla.org/integration/mozilla-inbound/rev/e22cd35d3c39
https://hg.mozilla.org/integration/mozilla-inbound/rev/e38d42f7ba2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1209c6d7e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/57cf6353c9fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/40b67434bff6
https://hg.mozilla.org/integration/mozilla-inbound/rev/579411961394
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa71e4ddef2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0c29a6d058
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2573c84ff61
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b47f5f715a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d302571cd5e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6084eaafbf

Comment 70

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e53bad3e7559
https://hg.mozilla.org/mozilla-central/rev/695d86baf80e
https://hg.mozilla.org/mozilla-central/rev/1c9325b0e173
https://hg.mozilla.org/mozilla-central/rev/58937c535d90
https://hg.mozilla.org/mozilla-central/rev/e0c0779e0ecc
https://hg.mozilla.org/mozilla-central/rev/b6d87e6e1064
https://hg.mozilla.org/mozilla-central/rev/628c1d07a0e9
https://hg.mozilla.org/mozilla-central/rev/ad1086bc9744
https://hg.mozilla.org/mozilla-central/rev/e22cd35d3c39
https://hg.mozilla.org/mozilla-central/rev/e38d42f7ba2b
https://hg.mozilla.org/mozilla-central/rev/ed1209c6d7e3
https://hg.mozilla.org/mozilla-central/rev/57cf6353c9fb
https://hg.mozilla.org/mozilla-central/rev/40b67434bff6
https://hg.mozilla.org/mozilla-central/rev/579411961394
https://hg.mozilla.org/mozilla-central/rev/dfa71e4ddef2
https://hg.mozilla.org/mozilla-central/rev/1e0c29a6d058
https://hg.mozilla.org/mozilla-central/rev/c2573c84ff61
https://hg.mozilla.org/mozilla-central/rev/79b47f5f715a
https://hg.mozilla.org/mozilla-central/rev/d302571cd5e5
https://hg.mozilla.org/mozilla-central/rev/ca6084eaafbf

Comment 71

[Eric Faust [:efaust]]

Landed, clearing.