Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementations do not agree on ArrayBuffer construction step order #3388

Open
gibson042 opened this issue Aug 12, 2024 · 2 comments
Open

Implementations do not agree on ArrayBuffer construction step order #3388

gibson042 opened this issue Aug 12, 2024 · 2 comments

Comments

@gibson042
Copy link
Contributor

gibson042 commented Aug 12, 2024

new ArrayBuffer(length, options = { maxByteLength }) is specified to execute steps in the following order (with each potential source of an abrupt completion prefixed with a circled digit):

  1. ➊ Let byteLength be ? ToIndex(length)
  2. Let requestedMaxByteLength be ? GetArrayBufferMaxByteLengthOption(options)
    1. ➋ Let maxByteLength be ? Get(options, "maxByteLength")
    2. ➌ Return ? ToIndex(maxByteLength)
  3. ? AllocateArrayBuffer(NewTarget as constructor, byteLength, requestedMaxByteLength as maxByteLength)
    1. ➍ If byteLength > maxByteLength, throw a RangeError exception.
    2. ? OrdinaryCreateFromConstructor(constructor, "%ArrayBuffer.prototype%" as intrinsicDefaultProto, slots as internalSlotsList)
      1. ? GetPrototypeFromConstructor(constructor, intrinsicDefaultProto)
        1. ➎ Let proto be ? Get(constructor, "prototype")
        2. If proto is not an Object, then… ? GetFunctionRealm(constructor as obj)
          1. ➏ If obj is a Proxy exotic object, then… ? ValidateNonRevokedProxy(obj)
    3. ? CreateByteDataBlock(byteLength as size)
      1. ➐ If it is impossible to create a new Data Block value consisting of size bytes, throw a RangeError exception.
    4. ➑ If it is not possible to create a Data Block block consisting of maxByteLength bytes, throw a RangeError exception.

However, all current implementations except SpiderMonkey observably deviate from this order. This part of the specification (including other call sites of AllocateArrayBuffer) should be revisited with implementers to settle on universally acceptable behavior and cover it in test262 (i.e., beyond the simple a vs. b before/after tests at https://github.com/tc39/test262/tree/main/test/built-ins/ArrayBuffer ).

@anba
Copy link
Contributor

anba commented Aug 13, 2024

Can provide some hints where SpiderMonkey works incorrectly? Some basic tests show spec compliance issues in JSC and V8, but I couldn't find a test which fails in SpiderMonkey.

function runTest(expected, {
  index: indexValue,
  maxByteLength: maxByteLengthValue,
  proto = ArrayBuffer.prototype,
}) {
  let log = [];

  let index = {
    valueOf() {
      log.push("index valueOf");
      return indexValue;
    }
  };

  let maxByteLength = {
    valueOf() {
      log.push("maxByteLength valueOf");
      return maxByteLengthValue;
    }
  };

  let options = {
    get maxByteLength() {
      log.push("get maxByteLength");
      if (maxByteLengthValue === undefined) {
        return undefined;
      }
      return maxByteLength;
    }
  };

  let {proxy: newTarget, revoke} = Proxy.revocable(function(){}, {
    get(t, pk, r) {
      log.push("proxy-get " + String(pk));
      if (proto === null) {
        revoke();
        return undefined;
      }
      return proto;
    }
  });

  try {
    let ab = Reflect.construct(ArrayBuffer, [index, options], newTarget);

    if (Object.getPrototypeOf(ab) !== ArrayBuffer.prototype) {
      log.push("wrong prototype");
    }
  } catch (e) {
    log.push("caught " + e.name);
  }

  if (log.toString() !== expected.toString()) {
    (console?.log ?? print)(`${log} != ${expected}`);
  }
}

// conversions and property accesses
runTest([
  "index valueOf",
  "get maxByteLength",
  "maxByteLength valueOf",
  "proxy-get prototype",
], {
  index: 0,
  maxByteLength: 0,
});

// index > maxByteLength
runTest([
  "index valueOf",
  "get maxByteLength",
  "maxByteLength valueOf",
  "caught RangeError",
], {
  index: 10,
  maxByteLength: 0,
});

// index < 0
runTest([
  "index valueOf",
  "caught RangeError",
], {
  index: -1,
  maxByteLength: 0,
});

// maxByteLength < 0
runTest([
  "index valueOf",
  "get maxByteLength",
  "maxByteLength valueOf",
  "caught RangeError",
], {
  index: 0,
  maxByteLength: -1,
});

// index too large for ToIndex
runTest([
  "index valueOf",
  "caught RangeError",
], {
  index: 2**53,
  maxByteLength: 0,
});

// maxByteLength too large for ToIndex
runTest([
  "index valueOf",
  "get maxByteLength",
  "maxByteLength valueOf",
  "caught RangeError",
], {
  index: 0,
  maxByteLength: 2**53,
});

// index too large to allocate
runTest([
  "index valueOf",
  "get maxByteLength",
  "proxy-get prototype",
  "caught RangeError",
], {
  index: 2**52,
  maxByteLength: undefined,
});

// maxByteLength too large to allocate
runTest([
  "index valueOf",
  "get maxByteLength",
  "maxByteLength valueOf",
  "proxy-get prototype",
  "caught RangeError",
], {
  index: 0,
  maxByteLength: 2**52,
});

// constructor.prototype is non-object and constructor is revoked
runTest([
  "index valueOf",
  "get maxByteLength",
  "maxByteLength valueOf",
  "proxy-get prototype",
  "caught TypeError",
], {
  index: 0,
  maxByteLength: 0,
  proto: null,
});

// constructor.prototype is non-object and constructor is revoked and
// index too large to allocate
runTest([
  "index valueOf",
  "get maxByteLength",
  "proxy-get prototype",
  "caught TypeError",
], {
  index: 2**52,
  maxByteLength: undefined,
  proto: null,
});

// constructor.prototype is non-object and constructor is revoked and
// maxByteLength too large to allocate
runTest([
  "index valueOf",
  "get maxByteLength",
  "maxByteLength valueOf",
  "proxy-get prototype",
  "caught TypeError",
], {
  index: 0,
  maxByteLength: 2**52,
  proto: null,
});

@gibson042
Copy link
Contributor Author

@anba I just updated my esvu SpiderMonkey, and confirmed that it implements the spec as written. Thanks for keeping me honest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants