1107687 - Promise should push AutoEntryScript before resolving thenable properties.

Status: RESOLVED FIXED

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

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

"then" may be implemented as getters, so we want the correct script entry on the stack

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

(We really need to go through all the cases where AutoJSAPI is used and check if AutoEntryScript should be used instead.)

Comment 2

[Bobby Holley (:bholley)]

(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.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: 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!

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=820cf3b87a5e

Comment 5

[Bobby Holley (:bholley)]

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. :-(

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: 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.

Comment 7

[Bobby Holley (:bholley)]

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.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Hey Bobby, do you think you could take this at some point?

Comment 9

[Bobby Holley (:bholley)]

(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.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Thank you.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I expect once bug 911216 lands this will become irrelevant...

Comment 12

[Till Schneidereit [:till]]

This can be closed now, right?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, very much so.  It even got fixed for DOM promises in bug 1279313!