Fix the entry settings object for promise callbacks

[domenic]

Closes #1426.

/cc @syg @littledan @bzbarsky @tschneidereit.

I was kind of waiting for tc39/ecma262#735 to land since that will change things, but I guess we should just fix this now, especially since #4571 needs to add something similar.

• At least two implementers are interested (and none opposed):
  • Firefox implements this
  • Chrome implements this with one bug regarding different-realm functions with PromiseResolveThenableJob
  • Safari implements the basics of this but fails all the different-realm function tests
• Tests are written and can be reviewed and commented upon at:
  • Test entry and incumbent settings object for promise jobs web-platform-tests/wpt#21206
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/v8/issues/detail?id=10200
  • Firefox: Not necessary
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=207641

(See WHATWG Working Mode: Changes for more details.)

---

/infrastructure.html ( diff )
/webappapis.html ( diff )

[syg]

We came up with a test where Chrome doesn't correctly thread the backup incumbent settings object for promise callbacks, though not in a way I yet understand...

[littledan]

These semantics (and the ones proposed at #4571 (comment)) seem reasonable to me.

Looks like this PR is about the entry Realm, not the backup incumbent settings object, so I don't see why mismatches there should block it from landing. (I'm not surprised if V8 doesn't handle this in the way Firefox does, and I'm worried it could be a bit complex to correct it.)

Minor: For entry, I'd editorially prefer it if we passed the realm up into HTML from the JS spec, but I'm fine with this landing as is; we can do whatever refactor later.

[domenic]

Thanks @littledan! I agree there are definitely better editorial/layering ways to do this if we can make changes on the 262 side. (That holds for incumbent too, I think: #5213.)

[domenic]

@bzbarsky ping on this, the tests at web-platform-tests/wpt#21206, and perhaps the discussion in #5213.

[bzbarsky]

So in terms of what Firefox implements here...

For a PromiseResolveThenableJob we seem to use the global of the thenAction argument (in spec terms) as the entry global, if I read the code right. That matches the proposed spec.

For a PromiseReactionJob we seem to use the global of the reaction's [[Handler]], if [[Handler]] is defined. That too matches the proposed spec.

For a PromiseReactionJob when [[Handler]] is not defined, I think we set the entry global to whatever the current global is at the point when we are creating the PromiseReactionJob, as far as I can tell. Which is not great. This is in fact somewhat observable, because we later do perform a "is it OK to run script in this global?" check, and if that returns false then the reaction's steps are not performed, I would think, even though they technically do not involve running script. I haven't looked at this stuff recently, but from code inspection it looks like this can happen for the then(non_callable) case (and similar for the rejection handler), something to do with "Async-from-Sync Iterator Objects", something to do with async functions (presumably the await special-case), and something to do with async generators.

There are certainly some interesting questions there. For example, if an async function is awaiting a promise and its window gets unloaded, and then the promise gets resolved, should the async function resume? In Gecko, I think it would, as long as the resolution happened from a window that was not yet unloaded. Per proposed spec it would in all cases, because the "check if we can run script" check gets skipped. What behavior do we want here?

[domenic]

What behavior do we want here?

I think the proposed spec is fairly reasonable, in that it tries to maintain a symmetry between promise.then().then(f) and promise.then(f). That is, it seems reasonable to me that introducing a no-handler then() should not introduce additional opportunities for scripting-is-disabled checks.

[bzbarsky]

Sure, but the thing that await does is not really "no-handler" in an important sense. That is, when the promise being awaited resolves, some script will run, and the question is whether at that point a check for whether scripting is disabled should happen. This is a significant difference from the promise.then().then(f) case, no?

[domenic]

Hmm, I might be confused then. The spec only skips the check if there's no handler, whereas await has a handler which performs a continuation. Right?

[bzbarsky]

Ah, interesting. So in terms of specification await has a non-observable handler function. In terms of implementation, it's implemented at least in SpiderMonkey as a PromiseReactionJob with no handler and some extra state of some sort, as far as I can tell (and I could very well be wrong!). So one just has to be very careful mapping from spec to implementation...

[annevk]

I'm assuming here that arguments, arguments[2], and arguments[0] can never be undefined or equivalent.

Is there an issue somewhere that tracks the further refactoring @littledan mentioned?

[domenic]

Thanks!

I double-checked, and yeah, those are never undefined.

I think tc39/ecma262#735 is the refactoring.

[littledan]

I agree that these are never undefined. I think we'd need the further refactoring I alluded to in #5212 (review) to make this cleaner, but let's wait on that until tc39/ecma262#1597 goes through (which is the new tc39/ecma262#735).