If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Move Promise.cpp to a model where settlement immediately queues the invocation of "then" callbacks

RESOLVED FIXED in mozilla36

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
A model where settlement immediately queues the invocation of "then" callbacks is required for bug 1013625.
(Assignee)

Comment 1

3 years ago
Created attachment 8507899 [details] [diff] [review]
The patch

Here is the patch, please take a look in particular at lifetime handling and JSContext usage, the tests pass but I'm not sure I understand all of this well enough.

I've copied some code that makes the value itself rooted, but the callbacks are only referenced by an nsRefPtr. Is the call to "HoldJSObjects(this);" in their constructor that keeps them alive despite cycle collection? Or is it related to the "JS::Heap<JSObject*> mGlobal;" member variable?

I added some aCx parameters, is it correct that passing those parameters around is an optimization and they could be replaced by ThreadsafeAutoJSContext or ThreadsafeAutoSafeJSContext inside the functions?

In the one place where I did not have an aCx parameter, I used ThreadsafeAutoSafeJSContext, as I've seen it used in a runnable, but I'm not sure if I should have used ThreadsafeAutoJSContext, which is what was used inside the function I removed.
Attachment #8507899 - Flags: review?(bzbarsky)
Comment on attachment 8507899 [details] [diff] [review]
The patch

>+++ b/dom/promise/Promise.cpp
>+  PromiseCallbackTask(nsRefPtr<Promise> aPromise,

Promise* aPromise.

>+                      nsRefPtr<PromiseCallback> aCallback,

PromiseCallback* aCallback.

>+                      JS::Handle<JS::Value> aValue)

I think you can safely make this a JS::Value and then you don't need the rooting bits at the callsite.  But do make sure to do a try run for the static analysis build.

>-    MOZ_ASSERT(aPromise);

Why drop that?

>   Run() MOZ_OVERRIDE
>+    NS_ASSERT_OWNINGTHREAD(PromiseResolverTask);

PromiseCallbackTask, right?

>+    if (!wrapper) {

Can't happen.  You just asserted so.

>+    JS::Rooted<JS::Value> rootedValue(cx, mValue);
>+    mCallback->Call(cx, rootedValue);

This lost the MaybeWrapValue we used to have.  That's definitely still needed.

>@@ -906,23 +878,22 @@ Promise::AppendCallbacks(PromiseCallback
>+    ThreadsafeAutoSafeJSContext cx;

Won't be needed if we change EnqueueCallbackTasks as I suggest below.

>@@ -1100,46 +1042,36 @@ Promise::ResolveInternal(JSContext* aCx,
>   // If the synchronous flag is set, process our resolve callbacks with
>   // value. Otherwise, the synchronous flag is unset, queue a task to process
>   // own resolve callbacks with value. Otherwise, the synchronous flag is
>   // unset, queue a task to process our resolve callbacks with value.

This whole comment should go, right?

> Promise::RejectInternal(JSContext* aCx,
>   // If the synchronous flag is set, process our reject callbacks with
>   // value. Otherwise, the synchronous flag is unset, queue a task to process
>   // promise's reject callbacks with value.

And this one.

>+Promise::RunResolveTask(JSContext* aCx,

Shouldn't need the aCx arg; see below.

Also, the name is now a bit weird.  It's not running a task of any sort at all, right?  What's a better name for this function?

Maybe document that mResolvePending is still needed in case we're resolved with a thenable?

>+  JS::Rooted<JS::Value> result(aCx, mResult);

I don't think you should need this if you make PromiseCallbackTask take a JS::Value argument and just pass mResult directly to the constructor...  And then you don't need aCx.

>+++ b/dom/promise/tests/test_promise.html

Why this change?  Did the order change?

> the callbacks are only referenced by an nsRefPtr

That's fine.  That nsRefPtr keeps them alive and all.  ;)

> Is the call to "HoldJSObjects(this);" in their constructor that keeps them alive despite cycle collection

That's what tells CC that they have JS members they want to trace.  The mGlobal, in particular.

What keeps them alive is that your runnable has a reference to them that's not part of a cycle, until the runnable itself is destroyed.

> is it correct that passing those parameters around is an optimization

Yes, but see above for why I don't think we need them at all.

r=me with the above nits.  This looks wonderful.
Attachment #8507899 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

3 years ago
Depends on: 1086528
(Assignee)

Comment 3

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #2)
> >+    if (!wrapper) {
> 
> Can't happen.  You just asserted so.

Ah, then I removed another instance of a similar check. I thought it was there for additional pointer safety in release builds.

> Also, the name is now a bit weird.  It's not running a task of any sort at
> all, right?  What's a better name for this function?

MaybeSettle, for now. I do have a list of other renames as well, filed bug 1086627 for them.

> Maybe document that mResolvePending is still needed in case we're resolved
> with a thenable?

Should be renamed to just mResolved, and documented, yes.

> >+++ b/dom/promise/tests/test_promise.html
> 
> Why this change?  Did the order change?

It did, because the "then" call itself has not yet been made when "setTimeout" is called. The runnable is now enqueued by the time we reach "then", while previously it was enqueued by the "r1" call.
(Assignee)

Comment 4

3 years ago
Created attachment 8508791 [details] [diff] [review]
Updated patch
Attachment #8507899 - Attachment is obsolete: true
>+      NS_WARNING("Failed to wrap value into the right compartment.");

Just realized: we should JS_ClearPendingException(cx) there.  That was a preexisting bug in this code...

> Should be renamed to just mResolved, and documented, yes.

Is that part of bug 1086627, then?

> because the "then" call itself has not yet been made when "setTimeout" is called.

Aha.  Does bug 1013625 change the order back, then?
(Assignee)

Comment 6

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> Is that part of bug 1086627, then?
> Aha.  Does bug 1013625 change the order back, then?

Yes and yes.
(Assignee)

Updated

3 years ago
Depends on: 1088704
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/048a23942308
https://hg.mozilla.org/mozilla-central/rev/048a23942308
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.