Bug in unhandledrejection context of promise rejected in next tick

[jridgewell]

There's currently a bug with our spec text and the handling of unhandledrejection, where the global context will be incorrect (undefined?) in the cases we care about:

ctx.run(1, () => {
  Promise.resolve().then(() => {
    throw new Error('test');
  });
});

addEventLitener('unhandledrejection', () => {
  assert.equal(ctx.get(), undefined);
});

Following the stack:

• Promise.prototype.then
• Step 5:
  • PerformPromiseThen
  • Step 10.b
    • NewPromiseReactionJob
    • Step 1.e - 1.h

The problem is that we've specified the restore step only within a HostCallJobCallback call, which is called during 1.e. Which means that by the time we get to 1.h.i to call the promiseCapability.[[Reject]], we've already restored the prior context. That prior context will be present when we finally call HostCallJobCallback, which calls HostPromiseRejectionTracker.

Re: #16, but I'm making a new issue because that one is specific to call/create time context and not the bug in our propagating.

[littledan]

It took me a bunch of staring but I'm convinced that this is a bug too :) . One simple (but arguably ugly) fix would be to use HostMakeJobCallback on Step 4 of NewPromiseReactionJob, and an associated HostCallJobCallback on the other side--I guess it's only needed for this particular purpose of unhandled rejections.

[Ultimately, if AsyncContext works, I think we might end up removing the host hook and using AsyncContext variables for the exact purpose that HTML needs those hooks in the first place. Maybe this would cause some observable changes and not actually be possible, but I'm not sure how interoperable browsers are in these edge cases anyway.]

[andreubotella]

It took me a bunch of staring but I'm convinced that this is a bug too :) . One simple (but arguably ugly) fix would be to use HostMakeJobCallback on Step 4 of NewPromiseReactionJob, and an associated HostCallJobCallback on the other side--I guess it's only needed for this particular purpose of unhandled rejections.

Currently we're not using the [[AsyncContextSnapshot]] field in JobCallback records in TC39 at all, we're only dictating that the host must set it and use it. What if we swap to that snapshot before calling reject and resolve in NewPromiseReactionJob, and then swap back? Or is there some reason we're leaving everything to the host?

[littledan]

We should probably maintain an HTML PR so that we can have a coherent understanding of what the hooks are for. For unhandled rejections, this HTML logic would need to save and restore the AsyncContextSnapshot between when the rejection initially occurs and when the event is triggered (after the microtask queue is drained).