Promise should push AutoEntryScript before resolving thenable properties.

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: nsm, Unassigned)

Tracking

(Blocks: 2 bugs)

36 Branch
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

"then" may be implemented as getters, so we want the correct script entry on the stack
(We really need to go through all the cases where AutoJSAPI is used and check if AutoEntryScript should be used instead.)
(In reply to Olli Pettay [:smaug] from comment #1)
> (We really need to go through all the cases where AutoJSAPI is used and
> check if AutoEntryScript should be used instead.)

Yes. Unfortunately given the MSE firefighting, I'm unlikely to have time to do any of that stuff myself over the next 3 months. If you have any cycles to spare for this smaug, it would great to get this stuff nailed down.
Blocks: 1108262
Created attachment 8537345 [details] [diff] [review]
Promise pushes AutoEntryScript before resolving thenables

bholley, I don't understand why something like this fails without the JS_WrapObject():

  |return window.Promise.resolve(new window.Array())| called from a JS implemented webidl binding

In this case, the cx received via WebIDL is the cx of the JS script right? But the cx AutoEntryScript obtains is that of the window.
But trying to call JS_GetProperty(aes.cx(), Array, "then"...) has a compartment mismatch! I thought objects created as new window.Array() would inherit the compartment of the global (window) which should match the global's context's cx?

Thanks!
Attachment #8537345 - Flags: review?(bobbyholley)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=820cf3b87a5e
Comment on attachment 8537345 [details] [diff] [review]
Promise pushes AutoEntryScript before resolving thenables

Per IRC discussion, should just be asserting GetEntryGlobal() in ResolveInternal, and fix up the ThreadsafeAutoSafeJSContext to use AutoEntryScript.

Sorry for the confusion here. :-(
Attachment #8537345 - Flags: review?(bobbyholley) → review-
Created attachment 8552109 [details] [diff] [review]
Promise pushes AutoEntryScript before resolving thenables

Asserting GetEntryGlobal() in ResolveInternal() and ensuring all callers use AES required a fair amount of changes, and repeated calls to NS_IsMainThread. Is this the best that can be done or did I not understand your IRC comments properly? You'd told me to use AES where we have a possibility of tripping the assert.
Attachment #8552109 - Flags: feedback?(bobbyholley)
Attachment #8537345 - Attachment is obsolete: true
Comment on attachment 8552109 [details] [diff] [review]
Promise pushes AutoEntryScript before resolving thenables

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

Sorry for the terrible lag. :-(

At a high level, my concern still stands - it seems like all of the callers who pass a cx down into this code should set up an AutoEntryScript, rather than having the Promise code re-instantiate an AutoEntryScript under the hood. Is that infeasible? If so, can you point me to the problematic callers?

::: dom/promise/Promise.cpp
@@ +66,5 @@
>    {
>      NS_ASSERT_OWNINGTHREAD(PromiseCallbackTask);
> +
> +    AutoJSAPI jsapi;
> +    jsapi.Init(mPromise->GetParentObject());

Hm, doesn't this call into PromiseCallback::Call, which can call Promise::ResolveInternal, which needs an AES?

@@ -1001,5 @@
>      EnqueueCallbackTasks();
>    }
>  }
>  
> -class WrappedWorkerRunnable MOZ_FINAL : public WorkerSameThreadRunnable

This was just unused I guess?

@@ +1113,4 @@
>  
> +    bool isMainThread = NS_IsMainThread();
> +    AutoEntryScript aes(GetParentObject(), isMainThread,
> +                        isMainThread ? nullptr : GetCurrentThreadJSContext());

The idea of comment 5 is that we should not be creating a new cx here, and instead be making sure that all of the cxes that trickle into here have been set up with an AutoEntryScript, right? Or are you saying in comment 6 that there are too many to change? If so, can you point me to the ones that would need to be changed?

::: dom/promise/Promise.h
@@ +261,5 @@
>                         JS::Handle<JS::Value> aValue);
>    void RejectInternal(JSContext* aCx,
>                        JS::Handle<JS::Value> aValue);
>  
> +  static JSContext* GetWorkerJSContext();

This can't fail IIUC, so let's just call this WorkerJSContext().

::: dom/promise/PromiseCallback.cpp
@@ +80,5 @@
>    // Run resolver's algorithm with value and the synchronous flag set.
>  
> +  bool isMainThread = NS_IsMainThread();
> +  AutoEntryScript aes(mPromise->GetParentObject(), isMainThread,
> +                      isMainThread ? nullptr : GetCurrentThreadJSContext());

This should be pushed up into the caller.

@@ +200,5 @@
>                               JS::Handle<JS::Value> aValue)
>  {
> +  bool isMainThread = NS_IsMainThread();
> +  AutoEntryScript aes(mNextPromise->GetParentObject(), isMainThread,
> +                      isMainThread ? nullptr : GetCurrentThreadJSContext());

And this.
Attachment #8552109 - Flags: feedback?(bobbyholley) → feedback-
Hey Bobby, do you think you could take this at some point?
Flags: needinfo?(bobbyholley)
Blocks: 1207693
(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #8)
> Hey Bobby, do you think you could take this at some point?

Not in the near future, but I filed a tracking bug.
Flags: needinfo?(bobbyholley)
Thank you.
Assignee: nsm.nikhil → nobody
Status: ASSIGNED → NEW
I expect once bug 911216 lands this will become irrelevant...
Depends on: 911216
This can be closed now, right?
Flags: needinfo?(bzbarsky)
Oh, very much so.  It even got fixed for DOM promises in bug 1279313!
Status: NEW → RESOLVED
Last Resolved: a year ago
Depends on: 1279313
Flags: needinfo?(bzbarsky)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.