Closed Bug 1336705 Opened 3 years ago Closed 3 years ago

Support creating and resolving Promises without resolve/reject functions

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: till, Assigned: till)

References

Details

Attachments

(2 files)

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.
Component: JavaScript Engine → JavaScript: Standard Library
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.
Attachment #8833608 - Flags: review?(arai.unmht)
Blocks: 1272697
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());
Attachment #8833607 - Flags: review?(arai.unmht) → feedback+
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
Attachment #8833608 - Flags: review?(arai.unmht) → review+
Pushed by tschneidereit@gmail.com:
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
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.
https://hg.mozilla.org/mozilla-central/rev/53155a41c47a
https://hg.mozilla.org/mozilla-central/rev/cdba3f199f3d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.