Closed Bug 1055472 Opened 6 years ago Closed 4 years ago

Update builtin constructors to support subclassing

Categories

(Core :: JavaScript: Standard Library, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: evilpie, Assigned: efaust)

References

(Blocks 1 open bug)

Details

Attachments

(21 files, 6 obsolete files)

4.14 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.98 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
9.68 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.21 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.18 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.71 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.06 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.39 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.90 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.41 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.50 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.29 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
16.14 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.42 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.03 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.75 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
15.42 KB, patch
jorendorff
: review+
bhackett1024
: review+
terrence
: review+
Details | Diff | Splinter Review
14.20 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.93 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
9.61 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.28 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
No description provided.
Summary: Implement proper constructor and @@create semantics for WeakSet → Implement WeakSet constructor semantics
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.)
Summary: Implement WeakSet constructor semantics → Update builtin constructors to support subclassing
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8672826 - Flags: review?(jwalden+bmo)
NewFunctionWithSpecialFeaturesAndSpecificProtoCreatedFromThisOneExactSpot(79 arguments)
Attachment #8672872 - Flags: review?(jwalden+bmo)
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...
Attachment #8672877 - Flags: review?(jwalden+bmo)
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.
Attachment #8672826 - Flags: review?(jwalden+bmo) → review+
Attachment #8672871 - Flags: review?(jwalden+bmo) → review+
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.
Add a little helper for the cases where the constructor reads "if newTarget is undefined, let newTarget be the active function" or so
Attachment #8673345 - Flags: review?(jwalden+bmo)
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.
Attachment #8672872 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #8672877 - Flags: review?(jwalden+bmo) → review+
This could probably use some more ordering tests. What do you think?
Attachment #8673395 - Flags: review?(jwalden+bmo)
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!
Attachment #8673267 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8673345 - Flags: review?(jwalden+bmo) → review+
Attachment #8673355 - Flags: review?(jwalden+bmo) → review+
Attachment #8673370 - Flags: review?(jwalden+bmo) → review+
Depends on: 1215430
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?
Attachment #8673395 - Flags: review?(jwalden+bmo)
Attachment #8673400 - Flags: review?(jwalden+bmo) → review+
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 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.
Attachment #8673410 - Flags: review?(jwalden+bmo) → review+
Attachment #8673414 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8673419 - Flags: review?(jwalden+bmo) → review+
Also add an "IsDetached" check in the TypedArray(typedArray) constructor, to improve spec compliance and account for possible neuterings in the new get.
Attachment #8679111 - Flags: review?(jwalden+bmo)
Oops, also add the check for the TypedArray(buffer) case.
Attachment #8679111 - Attachment is obsolete: true
Attachment #8679111 - Flags: review?(jwalden+bmo)
Attachment #8679121 - Flags: review?(jwalden+bmo)
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?
Flags: needinfo?(lhansen)
(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.
Flags: needinfo?(lhansen)
Attachment #8679121 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8679769 - Flags: review?(jwalden+bmo)
Attachment #8673395 - Attachment is obsolete: true
Attachment #8682263 - Flags: review?(jwalden+bmo)
This should have gone up before the rebased step 9. My apologies.
Attachment #8682265 - Flags: review?(jwalden+bmo)
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");
Faust tested, Andre approved. Now with a few more tests.
Attachment #8682263 - Attachment is obsolete: true
Attachment #8682263 - Flags: review?(jwalden+bmo)
Attachment #8682777 - Flags: review?(jwalden+bmo)
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.
Attachment #8682265 - Flags: review?(jwalden+bmo) → review+
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.  :-) )
Attachment #8682777 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8682266 - Flags: review?(jwalden+bmo) → review+
There's a bunch of TI churn here. I want Brian to take a once over the changes to ObjectGroup.
Attachment #8685192 - Flags: review?(jwalden+bmo)
Attachment #8685192 - Flags: review?(bhackett1024)
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?
Attachment #8685192 - Flags: review?(bhackett1024)
(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.
Flags: needinfo?(bhackett1024)
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.
Attachment #8685192 - Flags: review+
Flags: needinfo?(bhackett1024)
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?
(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.
Whoops! Make sure that we actually set the rval of the cross compartment machinery, a step initially lost during the refactor.
Attachment #8679769 - Attachment is obsolete: true
Attachment #8679769 - Flags: review?(jwalden+bmo)
Attachment #8686421 - Flags: review?(jwalden+bmo)
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.
Attachment #8685192 - Flags: review?(jwalden+bmo) → review+
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...
Attachment #8686421 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8687445 - Flags: review?(jwalden+bmo) → review+
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 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.
Attachment #8685192 - Flags: review?(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.
Attachment #8685192 - Flags: review?(terrence) → review+
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.
Attachment #8688746 - Flags: review?(bhackett1024)
Fix stupid oom bug.
Attachment #8688746 - Attachment is obsolete: true
Attachment #8688746 - Flags: review?(bhackett1024)
Attachment #8688755 - Flags: review?(bhackett1024)
Remove unrelated bugfix from patch. Sorry for the noise.
Attachment #8688755 - Attachment is obsolete: true
Attachment #8688755 - Flags: review?(bhackett1024)
Attachment #8688756 - Flags: review?(bhackett1024)
Comment on attachment 8688743 [details] [diff] [review]
Followup 1: Upadte the GC interactions of ObjectGroupCompartment::AllocationSiteKey

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

\o/
Attachment #8688743 - Flags: review?(terrence) → review+
Attachment #8688756 - Flags: review?(bhackett1024) → review+
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
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
Blocks: 1225031
Depends on: 1232159
Landed, clearing.
Flags: needinfo?(efaustbmo)
Depends on: 1237177
Depends on: 1299098
Depends on: 1308022
You need to log in before you can comment on or make changes to this bug.