Normative: Align detached buffer semantics with web reality

[rkirsling]

Resolves #678.

Specifically, web reality is such that IsDetachedBuffer checks should not cause the following to throw:

• ArrayBuffer.prototype.byteLength
• [[HasProperty]] for typed arrays
• IntegerIndexedElementGet (i.e. [[GetOwnProperty]] and [[Get]] for typed arrays)
• IntegerIndexedElementSet (i.e. [[DefineOwnProperty]] and [[Set]] for typed arrays)

To ensure that we don't violate EIM invariants for typed arrays, we must also:

• Make integer-indexed elements [[Configurable]]
• Override [[Delete]] such that false is returned upon attempted deletion of an integer-indexed element

(CC @syg, @rwaldron)

[anba]

This change will also require updates to the TypedArray built-ins:

1. Some operations are no longer fallible and should be updated to use the ! prefix instead of ?.
2. Previous implicit detached ArrayBuffer checks now need to be explicit, for example in %TypedArray%.prototype.slice() to ensure we get the same behaviour for both code-paths (steps 14-15).
   • slice() might be a special-case, but it probably doesn't hurt to verify this for all built-ins and compare against actual implementations.

[rkirsling]

@anba %TypedArray%.prototype.slice does seem unique in this respect, but I'm also not sure a change is required? Step 15.b checks that O.[[ViewedArrayBuffer]] is detached before making a direct call to GetValueFromBuffer (and right after giving it a local name), but it seems that this is effectively an assertion, since we already verified this on step 2 (ValidateTypedArray step 4)?

That said, we do double-down on this check (of IsDetachedBuffer after ValidateTypedArray and before Set) in %TypedArray%.prototype.fill step 10—is there some reason that this isn't redundant?

[anba]

TypedArraySpeciesCreate() in step 9 can detach O. Then if count > 0, ? Get(O, Pk) in step 14.b.ii throws for detached buffers. And step 15.b has an explicit detached buffer check. With the current PR, step 14 will no longer throw, whereas step 15 will still throw, because there's the explicit check. For consistency both code paths should either both throw or not throw.

[rkirsling]

TypedArraySpeciesCreate() in step 9 can detach O.

Ah, thanks. I think I've (hopefully) covered your concerns now.

[anba]

Is there agreement to keep the explicit detached ArrayBuffer checks in %TypedArray%.prototype.copyWithin, %TypedArray%.prototype.fill, and %TypedArray%.prototype.slice? Not all engines include these checks.

detach function:

function detach(ab) {
  if (ArrayBuffer.transfer) {
    ArrayBuffer.transfer(ab);
  } else if (ArrayBuffer.detach) {
    ArrayBuffer.detach(ab);
  } else if (typeof detachArrayBuffer === "function") {
    detachArrayBuffer(ab);
  } else if (typeof transferArrayBuffer === "function") {
    transferArrayBuffer(ab)
  } else if (typeof Worker === "function") {
    try { eval("%ArrayBufferDetach(ab)") } catch (e) {
      var w = new Worker("", {type: "string"});
      w.postMessage(ab, [ab]);
      w.terminate();
    }
  } else {
    throw new TypeError("cannot detach array buffer");
  }
}

Correctly throws a TypeError in JSC and SM, but no error thrown in V8:

var ta = new Int32Array(10);
ta.fill({
  valueOf() {
    detach(ta.buffer);
    return 0;
  }
});

Same for copyWithin():

var ta = new Int32Array(10);
ta.copyWithin({
  valueOf() {
    detach(ta.buffer);
    return 0;
  }
});

And for the explicit check in %ArrayIteratorPrototype%.next ( ):

Correctly prints "0" and then throws TypeError in V8 and SM, but doesn't throw an error in JSC (, still stops after the first element):

var ta = new Int32Array(10);
for (var k of ta) {
  detach(ta.buffer);
  print(k);
}

Shouldn't throw a TypeError, but incorrectly throws one in V8:

var ta = new Int32Array(1);
var vals = ta.values();
print(JSON.stringify(vals.next())); // {"value":0,"done":false}
print(JSON.stringify(vals.next())); // {"done":true}
detach(ta.buffer);
print(JSON.stringify(vals.next())); // {"done":true}

There are probably still more differences in detached ArrayBuffer across engines. Do we attempt to find and fix them as part of this bug? For example in %TypedArray%.prototype.set:

Expected: Prints "get length", "get one", and then throws a TypeError.
Actual:

• JSC: Prints "get length" and then throws a RangeError
• V8: Prints "get length" and then crashes with Check failed: !destination_ta->WasDetached().. (Fortunately that's a release crash.)

var ta = new Int32Array(10);
ta.set({
  get length() {
    print("get length");
    detach(ta.buffer);
    return 1;
  },
  get 0() {
    print("get one");
    return 100;
  },
});

---

[anba]

And more importantly [[GetOwnProperty]] needs to be changed to use configurable = true for indexed properties. Engines currently don't do this, but it's required before this can land in the spec.

[anba]

for-in loops also show differences across engines. Are no longer existent indexed properties considered as deleted for EnumerateObjectProperties?

var ta = new Int8Array(4);
for (var k in ta) {
  print(k);
  detach(ta.buffer);
}

[syg]

My thinking is that we align with reality on what's here in this PR, and punt on the method incompats that @anba listed.

#2164 (comment) is an interesting point. As it stands, TAs break the non-configurable invariant in that detached buffers cause TAs to return undefined when getting own property descriptors on the indexed properties, as well as [[HasProperty]] returning false. But changing configurable to true doesn't seem possible either: what do we expect delete to do? TAs can't have holes -- making them have holes is a nonstarter.

What are our options here? I don't see a way to unbreak the non-configurable invariant as well as keeping with reality around detached buffers.

Edit: See below. I had misunderstood the invariants to be bidirectional on configurability. There's no issue making them configurable. We should make that change -- there is a compat risk but I'm willing to take it. Please point it out during plenary and ask if JSC and SM would be willing to make the change as well.

[bakkot]

Strictly speaking it is not an essential invariant that a configurable: true property can be delete'd (only the converse). You can make a Proxy which reports configurable: true for some property but refuses to let it be deleted, for example: new Proxy({}, { deleteProperty: (t, k) => k === 'x' ? false : Reflect.deleteProperty(t, k) }). I wouldn't be too surprised to find that there are existing web-platform objects which display similar behavior.

[syg]

Ah, good call on delete. What about [[HasProperty]]? It has:

• If P was previously observed as a non-configurable own data or accessor property of the target, [[HasProperty]] must return true.

[bakkot]

If P was previously observed as a non-configurable own data or accessor property of the target, [[HasProperty]] must return true.

As with delete, this is only a constraint on properties which are not configurable, and places no constraints on those which are.

[syg]

Ah, of course, then there's no issue here.

[rkirsling]

My thinking is that we align with reality on what's here in this PR, and punt on the method incompats that @anba listed.

Agreed. The crucial thing here is fixing the spec where reality is consistent (and has been for a long time). Ending up with as much consistency as possible would be ideal, but it's less urgent since (1) behavior that engines don't agree upon is behavior that users can't rely upon and (2) removing errors is web-compatible.

[anba]

Integer indexed object will need to have their own [[Delete]] method. Something along the lines of:

1. Assert: IsPropertyKey(P) is true.
2. Assert: O is an Integer-Indexed exotic object.
3. If Type(P) is String, then
   1. Let numericIndex be ! CanonicalNumericIndexString(P).
   2. If numericIndex is not undefined, then
      1. If IsDetachedBuffer(buffer) is true, return true.
      2. If ! IsValidIntegerIndex(O, numericIndex) is false, return true.
      3. Return false.
4. Return ? OrdinaryDelete(O, P).

And the configurable check in [[DefineOwnProperty]] needs to be flipped.

[syg]

@rkirsling My understanding of this PR currently is the following. Is this correct?

• Make byteLength return 0 for detached buffers.
• Make length return 0 for detached buffers.
• Make [[Get]] return undefined for detached buffers.
• Make [[Set]] do nothing in sloppy mode for detached buffers.
• Make slice() only throw when it would return a non-empty array.

Unfortunately not all of these are web reality. Only byteLength and length seem to be.

I'd still prefer that we align on non-throwing behavior for [[Get]] and [[Set]], but would need input from Apple.

Here's a test program and the output:

test.js

function detach(ab) {
  if (ArrayBuffer.transfer) {
    ArrayBuffer.transfer(ab);
  } else if (ArrayBuffer.detach) {
    ArrayBuffer.detach(ab);
  } else if (typeof detachArrayBuffer === "function") {
    detachArrayBuffer(ab);
  } else if (typeof transferArrayBuffer === "function") {
    transferArrayBuffer(ab)
  } else if (typeof Worker === "function") {
    try { eval("%ArrayBufferDetach(ab)") } catch (e) {
      var w = new Worker("", {type: "string"});
      w.postMessage(ab, [ab]);
      w.terminate();
    }
  } else {
    throw new TypeError("cannot detach array buffer");
  }
}

class MyArray extends Uint32Array {
  static get [Symbol.species]() { detach(gToDetach.buffer); return Uint8Array; }
}

function test(msg, cb) {
  print(msg);
  try { print("  >> " + cb()); } catch (e) { print("  >> caught " + e); }
  print("");
}

var ta = new Uint8Array(512);
detach(ta.buffer);

test(".byteLength", () => ta.byteLength);
test(".length", () => ta.length);
test("[[Get]]", () => ta[42]);
test("[[Set]]", () => ta[42] = 42);
test("[[Has]]", () => ta.hasOwnProperty("42"));
test(".slice() @@species count>0 path", () => {
  gToDetach = new MyArray(512);
  gToDetach.slice(0, 128);
});
test(".slice() @@species count=0 path", () => {
  gToDetach = new MyArray(512);
  gToDetach.slice(42, 42);
});
test(".slice() count>0 path", () => {
  ta.slice(0, 128);
});
test(".slice() count=0 path", () => {
  ta.slice(42, 42);
});

Output

jsc

.byteLength
  >> 0

.length
  >> 0

[[Get]]
  >> caught TypeError: Underlying ArrayBuffer has been detached from the view

[[Set]]
  >> caught TypeError: Underlying ArrayBuffer has been detached from the view

[[Has]]
  >> true

.slice() @@species count>0 path
  >> caught TypeError: Underlying ArrayBuffer has been detached from the view

.slice() @@species count=0 path
  >> caught TypeError: Underlying ArrayBuffer has been detached from the view

.slice() count>0 path
  >> caught TypeError: Underlying ArrayBuffer has been detached from the view

.slice() count=0 path
  >> caught TypeError: Underlying ArrayBuffer has been detached from the view

sm

.byteLength
  >> 0

.length
  >> 0

[[Get]]
  >> undefined

[[Set]]
  >> 42

[[Has]]
  >> false

.slice() @@species count>0 path
  >> caught TypeError: attempting to access detached ArrayBuffer

.slice() @@species count=0 path
  >> undefined

.slice() count>0 path
  >> caught TypeError: attempting to access detached ArrayBuffer

.slice() count=0 path
  >> caught TypeError: attempting to access detached ArrayBuffer

v8

.byteLength
  >> 0

.length
  >> 0

[[Get]]
  >> undefined

[[Set]]
  >> 42

[[Has]]
  >> false

.slice() @@species count>0 path
  >> caught TypeError: Cannot perform %TypedArray%.prototype.slice on a detached ArrayBuffer

.slice() @@species count=0 path
  >> undefined

.slice() count>0 path
  >> caught TypeError: Cannot perform %TypedArray%.prototype.slice on a detached ArrayBuffer

.slice() count=0 path
  >> caught TypeError: Cannot perform %TypedArray%.prototype.slice on a detached ArrayBuffer

[rkirsling]

Note that this came up because I was going through JSC's test262 failures and looking for things to fix, so it should be understood that we have a slew of conformance bugs yet to fix in ArrayBuffer#slice and the EIMs for integer-indexed objects. (We can get explicit confirmation from an Appler if you're worried though. 😅)

[syg]

Note that this came up because I was going through JSC's test262 failures and looking for things to fix, so it should be understood that we have a slew of conformance bugs yet to fix in ArrayBuffer#slice and the EIMs for integer-indexed objects. (We can get explicit confirmation from an Appler if you're worried though. 😅)

Good to hear! I'm not worried per se, but would nevertheless like explicit confirmation.

Edit: I mean explicit confirmation during plenary itself, not necessary ahead of time.

[syg]

This PR lgtm editorially.

[bakkot]

For .slice, it looks like the behavior specified in this PR is for

.slice() @@species count>0 path
  >> TypeError

.slice() @@species count=0 path
  >> undefined (but the `.slice` does return a new empty Uint8Array)

.slice() count>0 path
  >> TypeError

.slice() count=0 path
  >> TypeError

That doesn't match what @syg said above:

Make slice() only throw when it would return a non-empty array.

Though it does match V8 and SpiderMonkey's behavior reported above.

Specifically, it's still throwing in the fourth case as a consequence of step 2, "Perform ? ValidateTypedArray(O).".

Is that intentional?

[bakkot]

LGTM assuming the behavior described in my above comment is intentional.

[michaelficarra]

This LGTM, though I would also be okay with delaying the ValidateTypedArray call until after _count_ was determined so that we can conform to @shu's preference.

[rkirsling]

The change to slice in this PR was just meant to ensure its behavior wouldn't be inadvertently changed due to implicit checks, though we could certainly improve consistency here if desired.

[ljharb]

editorial LGTM

[michaelficarra]

@rkirsling If you'd like to make that change (even though all implementations agree on throwing), please open a separate PR so we can get consensus on that.

[syg]

@bakkot Thanks for catching my misunderstanding re: #2164 (comment). I'm pretty sure I misread the PR at the time. The diff showed a bunch of steps moving under "If count > 0", which I must've misunderstood to mean it was moving the only detached throwing case to under that condition.

For #2164 (comment), I didn't intend nor currently think we should be delaying ValidateTypedArray to be after checking the count. I'm happy with the merged PR reflecting implementation reality.

[bakkot]

Looking again, I think this ought to also have added a IsDetachedBuffer check to the [[OwnPropertyKeys]] internal method of integer-indexed exotic objects, so that

let x = new Uint8Array(20);
detach(x.buffer);
Object.getOwnPropertyNames(x)

would produce [], as it does in V8, SM, and JSC.

(Aside: a simple detach implementation for browsers is let detach = b => postMessage(null, '*', [b]), if you want to try it yourself.)