1279313 - Should Promise MaybeSomething be using AutoEntryScript instead of AutoJSAPI?

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Seems to me like we can end up running random script under there: consider resolution with an object which has a scripted getter for "then".

That leaves the fun question of what the entry global should be, of course.  Simplest would be the Promise global itself, but speccing that might be hard.

Also, are there other codepaths that can reach the Get(promise, "then")?  Do they all have AutoEntryScripts on them?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bobby, thoughts?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #0)
> Seems to me like we can end up running random script under there: consider
> resolution with an object which has a scripted getter for "then".

Agreed.

> That leaves the fun question of what the entry global should be, of course. 
> Simplest would be the Promise global itself, but speccing that might be hard.

Two options would be the global of "this" promise or that of the thing we're getting "then" from. I'm fine with whatever is easier to spec. Maybe ask Domenic?

> Also, are there other codepaths that can reach the Get(promise, "then")?  Do
> they all have AutoEntryScripts on them?

I don't know offhand.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> or that of the thing we're getting "then" from

You mean enter an AutoEntryScript right before doing the "then" get?  That's an interesting idea, actually.  It would ensure that we hit all the codepaths, at least.

Domenic, thoughts?

Comment 4

[Domenic Denicola]

(I'm assuming that AutoEntryScript is somewhat equivalent to the spec's "Prepare to run script" in that it sets up the necessary stuff to make the entry settings object definition work.)

This seems like a reasonable location to perform that operation. I can't see any better one, really.

As for what global to use. This seems very similar to the problems discussed in https://github.com/tc39/ecma262/issues/573 where V8 at least would like to move away from associating every object with a global, in which case using the object's global is not great.

I'm having a hard time mapping MaybeSomething to something that exists in the spec (even after browsing the Gecko code a bit). In the spec the only time you grab the .then property is either as part of an Invoke() or in https://tc39.github.io/ecma262/#sec-promise-resolve-functions. The latter is sometimes invoked asynchronously as part of https://tc39.github.io/ecma262/#sec-promisereactionjob (step 8a), in which case I would have thought the correct thing to do was set up the entry global stuff before doing step 6, since step 6 is where user code starts being called. See https://github.com/whatwg/html/pull/1189#issuecomment-224998842 for more thoughts in this area.

In any case everything here will be a giant mess to spec because it involves fairly invasive changes to the ES spec in order to support concepts of entry (and incumbent) which ES doesn't care about at all. But I think first I'd appreciate figuring out what we would ideally want to spec, and then worrying about how to do so. https://github.com/whatwg/html/issues/1426 is meant to be that discussion.

Comment 5

[Domenic Denicola]

> This seems like a reasonable location to perform that operation. I can't see any better one, really.

I realize that I kind of contradicted this a couple paragraphs down. Sorry. Please ignore this sentence. In the end I am confused as to where we want to do this in spec-land, so I don't have any thoughts on where to do this in Gecko.

Also, not knowing where we want to do this makes it harder for me to imagine what our options are for globals to use.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm assuming that AutoEntryScript is somewhat equivalent to the spec's "Prepare to run script"

Yes, exactly.  It's the thing that sets up the entry settings object.

> I'm having a hard time mapping MaybeSomething to something that exists in the spec

MaybeSomething is basically what we use on the C++ side to resolve or reject a Promise with some value.  In spec terms it basically corresponds to the shorthand phrases defined in <https://www.w3.org/2001/tag/doc/promises-guide#shorthand-manipulating>.  Note that this need not happen off an ES Job per our discussion recently in one of the spec threads... It can happen off an arbitrary task.

The important thing for my purposes is that in Gecko we do not want to be running any script without an entry global being set up.  I guess what I'll do for now in Gecko is go ahead and just "prepare to run script" using the global of the promise when one of the things at https://www.w3.org/2001/tag/doc/promises-guide#shorthand-manipulating is performed (i.e. in MaybeSomething in Gecko) and not worry about other ways the Get(resolution, "then") of https://tc39.github.io/ecma262/#sec-promise-resolve-functions can be reached: the other cases would all de facto have an entry settings once we switch to the Promise-in-SpiderMonkey implementation...

I'm sorry this is all such a mess.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Simplify AutoEntryScript to not make callers pass in a JSContext

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use an AutoEntryScript when resolving or rejecting a Promise from C++, in case we are resolving with an object and plan to call the "then" getter

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77acaa22eaa
part 1.  Simplify AutoEntryScript to not make callers pass in a JSContext.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26509e4c8a1
part 2.  Use an AutoEntryScript when resolving or rejecting a Promise from C++, in case we are resolving with an object and plan to call the "then" getter.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b77acaa22eaa
https://hg.mozilla.org/mozilla-central/rev/e26509e4c8a1