Ecma262: Engines don't agree with each other on how to await promise instances with overridden `.then` properties

[isiahmeadows]

Description: Nobody agrees on how to handle overridden .constructor/.then properties on promise instances. The spec seems to side with V8 based on my interpretation of Await, PromiseResolve, and promise resolve functions, but that breaks our ability to hook into promise chaining for scheduling implicit redraws. Our implementation was written initially to work with userland promises, so it'd need updated to work in Chrome for async functions.

We could work around that with the current spec by also setting a .constructor property that delegates to the real constructor, basically p.constructor = function (executor) { return new Promise(executor) } any time we change .then. It was originally written prior to the async function proposal, and it could be easily modified.

I initially filed a bug against V8, but after I read the spec, V8 appears to be in the right here, but it's breaking us in ways that make it look like a browser bug that has nothing to do with the underlying JS engine.

Test script:

// test.js
const p = Promise.resolve()
const then = p.then

Object.defineProperty(p, "constructor", {
    get: () => {
        print("get constructor")
        return Promise
    }
})

p.then = (...args) => {
    print("received then")
    return Reflect.apply(then, p, args)
}

async function testAsync() {
    print("call async")
    await p
    print("resume await")
}

function testThen() {
    print("call then")
    return p.then(() => {
        print("resume then")
    })
}

testAsync()
.then(() => print("---------------"))
.then(testThen)

eshost Output:

$ eshost test.js
#### ch, xs
call async
get constructor
received then
get constructor
resume await
---------------
call then
received then
get constructor
resume then

#### jsc, sm
call async
received then
get constructor
resume await
---------------
call then
received then
get constructor
resume then

#### v8
call async
get constructor
resume await
---------------
call then
received then
get constructor
resume then

(Lightly edited the output for clarity on who aligns with who.)

[isiahmeadows]

I did find one genuine spec bug in the mix: p.constructor === Promise is not enough to ensure that p has all the fields of a promise (as PerformPromiseThen in step 9 of the Await abstract operation requires). That's all PromiseResolve seems to check, and that's not sufficient when internal slots are in play.

Edit: To clarify, that could arise in the scenario of {constructor: Promise, then(resolve, reject) { ... }}. I have not yet tested this, but I do plan to file a second issue specifically to track this.

Edit 2: Filed #1578.

[devsnek]

simplification of the above script which also shows the big concern:

'use strict';

const p = Promise.resolve('x1');

Object.defineProperty(p, 'constructor', {
  get: () => {
    print('get constructor');
    return Promise;
  },
});

p.then = (onFulfilled) => {
  onFulfilled('x2');
  return Promise.resolve();
};

(async function () {
  const x = await p;
  return x;
}()).then(print);

┌────────────────────────┬─────────────────┐
│ Chakra                 │ get constructor │
│ XS                     │ x2              │
├────────────────────────┼─────────────────┤
│ engine262              │ get constructor │
│ V8                     │ x1              │
├────────────────────────┼─────────────────┤
│ JavaScriptCore         │ x2              │
│ SpiderMonkey           │                 │
└────────────────────────┴─────────────────┘

we definitely don't want this resolving to x2...

This does appear to have been fixed in chakra at some point but is not in the latest release (cc @zenparsing) https://github.com/microsoft/ChakraCore/commit/753fc6f88a736e20f2d93d3dec775f5b497b4cdc

[mathiasbynens]

@isiahmeadows

(Lightly edited the output for clarity on who aligns with who.)

Pro tip: eshost -s test.js does this for you.

[mathiasbynens]

TL;DR

• PromiseResolve(constructor, value) returns value if constructor is the original Promise, i.e. %Promise%.
• V8 and engine262 implement this per spec, but other engines have varying behavior.

I don’t think we’d want to change the spec here. Adding Test262 tests for this scenario (assuming they’re missing) would be a good next step and would help implementations align. @isiahmeadows would you be interested in turning your examples into a Test262 PR?

[isiahmeadows]

@mathiasbynens Sure.

[anba]

This difference is expected, see #1250.

The SpiderMonkey shell currently defaults to the previous state, but that'll change soon(ish): https://bugzilla.mozilla.org/show_bug.cgi?id=1558971