1336705 - Support creating and resolving Promises without resolve/reject functions

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Till Schneidereit [:till]]

This should be a nice optimization for when promises are created internally and thus can't leak their resolution functions to content.

My main motivation for doing this is that Readable- and WritableStream create lots of promises, but it should also work nicely for async functions.

Comment 1

[Till Schneidereit [:till]]

Attachment: Part 1: Support creating and resolving Promises without resolve/reject functions

Comment 2

[Till Schneidereit [:till]]

Attachment: Part 2: Add self-hosting intrinsics for resolving/rejecting Promises and adding reactions

These aren't used right now, but they will be once bug 1272697 lands. In the meantime they could well also be useful for other code.

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 8833607 [details] [diff] [review]
Part 1: Support creating and resolving Promises without resolve/reject functions

Review of attachment 8833607 [details] [diff] [review]:
-----------------------------------------------------------------

almost looks good :)
I have one question in ResolvePromiseFunctionImpl.

::: js/src/builtin/Promise.cpp
@@ +385,5 @@
>      return true;
>  }
>  
> +static bool
> +ResolvePromiseFunctionImpl(JSContext* cx, HandleObject promise, HandleValue resolutionVal)

can you add "ES2016, 25.4.1.3.2, steps 6-13" or something before this function definition?
so that "// Step 6." comment below makes sense.

@@ +397,5 @@
> +        return true;
> +    }
> +
> +    // Step 6.
> +    if (resolutionVal.isObject() && &resolutionVal.toObject() == promise) {

I don't see any reason why this step should be done before calling ResolvePromiseInternal.
ResolvePromiseInternal contains almost same code for same step,
and reordered Step 7 isn't executed if this condition matches.

will ResolvePromiseInternal be modified in later patch or different bug?

@@ +426,1 @@
>      RootedValue promiseVal(cx, resolve->getExtendedSlot(ResolveFunctionSlot_Promise));

we don't need to get or root promiseVal here,
that can be done at once with the following code later:

> RootedObject promise(cx, &promiseVal.toObject());

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8833608 [details] [diff] [review]
Part 2: Add self-hosting intrinsics for resolving/rejecting Promises and adding reactions

Review of attachment 8833608 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SelfHosting.cpp
@@ +2178,5 @@
>      return true;
>  }
>  
> +static bool
> +intrinsic_GetPendingPromise(JSContext* cx, unsigned argc, Value* vp)

looks like this and other "Get*" functions are actually creating new object.
can they be named "Create*" or "New*" ?

@@ +2244,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() >= 2);
> +
> +    RootedObject promise(cx, &args[0].toObject());
> +    Value val = args.get(1);

if |args.length() >= 2|, we can use args[1] instead.

@@ +2264,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() >= 2);
> +
> +    RootedObject promise(cx, &args[0].toObject());
> +    Value val = args.get(1);

same here

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53155a41c47a
Part 1: Support creating and resolving Promises without resolve/reject functions. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdba3f199f3d
Part 2: Add self-hosting intrinsics for resolving/rejecting Promises and adding reactions. r=arai

Comment 6

[Till Schneidereit [:till]]

FTR: I hadn't seen that arai gave an f+ instead of an r+ on part 1. Luckily they were observant enough to see the landing and inform me about my mistake. Also luckily, I got a post hoc r+ on the revised patch on IRC.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/53155a41c47a
https://hg.mozilla.org/mozilla-central/rev/cdba3f199f3d