1047509 - Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 14

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

Another (well maybe not just another) batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.

Comment 1

[Bob Owen (:bobowen)]

https://tbpl.mozilla.org/?tree=Try&rev=2374813a9223

* bobowen crosses his fingers

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace nsCxPusher in nsJSNPRuntime.cpp doInvoke v1

Try push is looking reasonably good, so I'm going to start uploading.
I suspect that a few of these might take some review iterations, but hopefully starting with some less controversial ones.

Just switching the nsCxPusher for AutoEntryScript here.
The AutoEntryScript effectively does the same as GetJSContext to get to its context or will end up with SafeJSContext if that fails.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_GetProperty v1

Very similar to Part 1.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_SetProperty v1

ditto

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 4: Replace nsCxPusher in WorkerRunnable::Run v1

This one's a bit of a dog's dinner, but it's mainly because this function is wearing too many hats.
* bobowen waits for r-

I can't help thinking that this would be a lot simpler if the TargetAndBusyBehavior was broken out into separate classes or something similar.
Also, if CompileScriptRunnable was special-cased, as that creates the global.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 5: Change mozJSComponentLoader::PrepareObjectForLocation to take JSContext* instead of JSCLContextHelper& v1

I've broken the one for the nsCxPusher in mozJSComponentLoader into separate patches, as it seemed to make sense to get rid of the new JSContext it creates as well, so it meant quite a few changes.

This one is fairly straight forward PrepareObjectForLocation treats the JSCLContextHelper like a JSContext anyway.

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 7: Change mozJSComponentLoader::ObjectForLocation to use AutoJSAPI and SafeJSContext instead of JSCLContextHelper and its own JSContext v1

This is switching to the SystemErrorReporter anyway, so I think we can just use the SafeJSContext.
Then I'm upping this to an AutoEntryScript, just before we run the script and when we have a global.

Not sure if the JS_FireOnNewGlobalObject in PrepareObjectForLocation also needs an AutoEntryScript around it?

Comment 8

[Bob Owen (:bobowen)]

Attachment: Part 7: Change mozJSComponentLoader::LoadModule to use AutoJSAPI and SafeJSContext instead of JSCLContextHelper and its own JSContext v1

Again I think we can use the SafeJSContext here, I don't think we'll be running script here (outside of the call to ObjectForLocation, which is handled in the last patch).

Comment 9

[Bob Owen (:bobowen)]

Attachment: Part 8: Add an AutoEntryScript into mozJSComponentLoader::ImportInto v1

I've re-purposed the JSCLContextHelper to do a similar job, but in a slightly different way.
This was mainly to reduce churn in the code.

It now takes the caller context and reports any errors as it dies.
This finally kills the nsCxPusher.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 9: Remove final uses of mozJSComponentLoader's own JSContext and tidy up v1

The last use of the old JSContext is in UnloadModules and again I think that we can use the SafeJSContext.

I'm assuming that we're holding onto the runtime and service, so that we can destroy our context.
From what I can tell, we don't have that problem with the SafeJSContext.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 10: Replace nsCxPusher in XRE_XPCShellMain v1

My thoughts here were that it is the creation of the new global that is now important and we can just use the SafeJSContext and then our new global for the AutoEntryScript (which will also use the SafeJSContext).

Comment 12

[Bob Owen (:bobowen)]

Attachment: Part 11: Remove nsCxPusher in nsPresContext::MediaFeatureValuesChanged v1

The original callback stuff that you landed should take care of the AutoEntryScript for us here.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 12: Remove nsCxPusher in nsTextBoxFrame::UpdateAccesskey v1

I think that we shouldn't be relying on the context alone for security checking anymore, so again I think that this can go.
I'm not at all sure though, as I found it difficult to work out where this would eventually make a difference anyway.

Comment 14

[Bob Owen (:bobowen)]

Attachment: Part 13: Replace AutoCxPusher in TestShellCommandParent::RunCallback v1

This is a bit of a hack, but hopefully it's OK given where it is.

Comment 15

[Bob Owen (:bobowen)]

Attachment: Part 14: Remove nsCxPusher v1

In my excitement I missed this patch from the Try push. :-)

But, I suspect I'll need another one after review changes.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8469421 [details] [diff] [review]
Part 1: Replace nsCxPusher in nsJSNPRuntime.cpp doInvoke v1

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +615,5 @@
>      return false;
>    }
>  
> +  // We're about to run script via JS_CallFunctionValue, so we need an
> +  // AutoEntryScript. NPAPI plugins are Gecko specific and not in any spec.

Nit: I'd hyphenate "Gecko-specific".

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8469433 [details] [diff] [review]
Part 3: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_SetProperty v1

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +777,5 @@
>      return false;
>    }
>  
> +  // We're about to run script via JS_CallFunctionValue, so we need an
> +  // AutoEntryScript. NPAPI plugins are Gecko specific and not in any spec.

(Here as well. I'll stop pointing them out).

@@ +779,5 @@
>  
> +  // We're about to run script via JS_CallFunctionValue, so we need an
> +  // AutoEntryScript. NPAPI plugins are Gecko specific and not in any spec.
> +  dom::AutoEntryScript aes(globalObject);
> +  JSContext* cx = aes.cx();

* on the right in this file (may apply to others as well).

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8469440 [details] [diff] [review]
Part 4: Replace nsCxPusher in WorkerRunnable::Run v1

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

::: dom/workers/WorkerRunnable.cpp
@@ +262,5 @@
>      return NS_OK;
>    }
>  
> +  nsCOMPtr<nsIGlobalObject> globalObject;
> +  bool isMainThread = false;

I'd prefer:

bool isMainThread = !targetIsWorkerThread && !mWorkerPrivate->GetGlobalScope();
MOZ_ASSERT(isMainThread == NS_IsMainThread());

@@ +267,1 @@
>    JSContext* cx;

Let's move the definition of cx to immediately above the jsapi/aes construction.

@@ +295,5 @@
> +    if (isMainThread) {
> +      globalObject = static_cast<nsGlobalWindow*>(mWorkerPrivate->GetWindow());
> +    } else {
> +      globalObject = mWorkerPrivate->GetParent()->GlobalScope();
> +      cx = mWorkerPrivate->GetParent()->GetJSContext();

Let's remove the cx assignment - see below.

@@ +311,5 @@
> +    if (isMainThread) {
> +      aes.construct(globalObject);
> +    } else {
> +      aes.construct(globalObject, false, cx);
> +    }

I'd prefer to remove the isMainThread conditional and make this all:

aes.construct(globalObject, isMainThread, isMainThread ? nullptr : GetCurrentThreadJSContext());

@@ +326,5 @@
>  
>    Maybe<JSAutoCompartment> ac;
>    if (targetCompartmentObject) {
>      ac.construct(cx, targetCompartmentObject);
>    }

All this targetCompartmentObject logic is unnecessary now, right? In any case where the object is non-null, we've already constructed an aes, and should be in the right compartment right?

I think we should nuke it. Then, we can replace the Maybe<> stuff below with a conditional construction of aes2 that happens if aes wasn't constructed and mWorkerPrivate->GlobalScope() is now non-null.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8469450 [details] [diff] [review]
Part 7: Change mozJSComponentLoader::ObjectForLocation to use AutoJSAPI and SafeJSContext instead of JSCLContextHelper and its own JSContext v1

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

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1043,5 @@
> +        // We're going to run script via JS_ExecuteScriptVersion or
> +        // JS_CallFunction, so we need an AutoEntryScript.
> +        // This is Gecko specific and not in any spec.
> +        dom::AutoEntryScript aes(GetNativeForGlobal(CurrentGlobalOrNull(cx)),
> +                                 NS_IsMainThread(), cx);

This should always be called on the main thread. Please just assert that at the top of the function, and drop the last two params here.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #7)
> Not sure if the JS_FireOnNewGlobalObject in PrepareObjectForLocation also
> needs an AutoEntryScript around it?

Oh! and yes it does.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8469457 [details] [diff] [review]
Part 8: Add an AutoEntryScript into mozJSComponentLoader::ImportInto v1

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

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1288,4 @@
>  
> +        // We might run script via JS_SetPropertyById on targetObj, so we need
> +        // an AutoEntryScript. This is Gecko specific and not in any spec.
> +        dom::AutoEntryScript aes(GetNativeForGlobal(js::GetGlobalForObjectCrossCompartment(mod->obj)));

This is one of those places where I'm pretty sure we don't actually want to be running script, ever. Can you switch to an AutoJSAPI?

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8469461 [details] [diff] [review]
Part 9: Remove final uses of mozJSComponentLoader's own JSContext and tidy up v1

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

\o/

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8469468 [details] [diff] [review]
Part 12: Remove nsCxPusher in nsTextBoxFrame::UpdateAccesskey v1

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

The context doesn't matter, but pushing the context _also_ enters the compartment of the context's default global (i.e. the DOMWindow), which in turn affects the subject principal.

In this case, it doesn't matter - This comment is talking about the case where the interface is implemented in XBL (which AFAICT is the only places it's implemented). In that case, the XPCWrappedJS aggregation machinery is going to set up all the JSAPI stuff anyway.

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8469472 [details] [diff] [review]
Part 13: Replace AutoCxPusher in TestShellCommandParent::RunCallback v1

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

r=me with that change.

::: ipc/testshell/TestShellParent.cpp
@@ +69,4 @@
>  
> +  // We're about to run script via JS_CallFunctionValue, so we need an
> +  // AutoEntryScript. This is just for testing and not in any spec.
> +  dom::AutoEntryScript aes(xpc::GetNativeForGlobal(xpc::GetSafeJSContextGlobal()));

Shouldn't we just initialize the AutoEntryScript with GetNativeGlobal(mCallback.ToJSObject())? That'd be more meaningful and eliminate the JSAutoCompartment below.

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 8469478 [details] [diff] [review]
Part 14: Remove nsCxPusher v1

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

\o/ \o/ \o/ \o/ \o/ \o/ \o/

Comment 26

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #18)

> >    Maybe<JSAutoCompartment> ac;
> >    if (targetCompartmentObject) {
> >      ac.construct(cx, targetCompartmentObject);
> >    }
> 
> All this targetCompartmentObject logic is unnecessary now, right? In any
> case where the object is non-null, we've already constructed an aes, and
> should be in the right compartment right?

(Might catch you)

But aren't we entering the compartment of mWorkerPrivate's global scope here?
After the aes, we'd be in our parent's.


(Thanks for the other tidying up, took me a while to get this working and I then couldn't see the wood for the trees.)

Comment 27

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace nsCxPusher in nsJSNPRuntime.cpp doInvoke v2

r=bholley - from comment 16.

Fixed comment and * position.

Comment 28

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_GetProperty v2

r=bholley - from #15.

Fixed comment and * position.

Comment 29

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_SetProperty v2

r=bholley - from comment 17.

Fixed comment and * position.

Comment 30

[Bob Owen (:bobowen)]

Comment on attachment 8469440 [details] [diff] [review]
Part 4: Replace nsCxPusher in WorkerRunnable::Run v1

I'm going to move this and Part 14 (that removes the nsCxPusher) to a separate bug.

I'm on PTO next week and I'd like to get the rest of these landed.

Comment 31

[Bob Owen (:bobowen)]

Attachment: Part 6: Change mozJSComponentLoader::ObjectForLocation to use AutoJSAPI and SafeJSContext instead of JSCLContextHelper and its own JSContext v2

r=bholley - from comment 19.

Also, fixed the Part number, which either I or bzexport messed up.

(In reply to Bobby Holley (:bholley) from comment #19)

> > +        dom::AutoEntryScript aes(GetNativeForGlobal(CurrentGlobalOrNull(cx)),
> > +                                 NS_IsMainThread(), cx);
> 
> This should always be called on the main thread. Please just assert that at
> the top of the function, and drop the last two params here.

Done.

(In reply to Bobby Holley (:bholley) from comment #20)
> (In reply to Bob Owen (:bobowen) from comment #7)
> > Not sure if the JS_FireOnNewGlobalObject in PrepareObjectForLocation also
> > needs an AutoEntryScript around it?
> 
> Oh! and yes it does.

Added this.
I couldn't remember whether you'd said these debugger hooks would be handled by something else, although I knew that you'd added one in nsGlobalWindow::SetNewDocument

Comment 32

[Bob Owen (:bobowen)]

Attachment: Part 8: Add an AutoJSAPI into mozJSComponentLoader::ImportInto v2

r=bholley - from comment 21.

(In reply to Bobby Holley (:bholley) from comment #21)
> Comment on attachment 8469457 [details] [diff] [review]
> Part 8: Add an AutoEntryScript into mozJSComponentLoader::ImportInto v1
> 
> Review of attachment 8469457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> @@ +1288,4 @@
> >  
> > +        // We might run script via JS_SetPropertyById on targetObj, so we need
> > +        // an AutoEntryScript. This is Gecko specific and not in any spec.
> > +        dom::AutoEntryScript aes(GetNativeForGlobal(js::GetGlobalForObjectCrossCompartment(mod->obj)));
> 
> This is one of those places where I'm pretty sure we don't actually want to
> be running script, ever. Can you switch to an AutoJSAPI?

Done.
I also improved the comment above |cxhelper| a bit.

Comment 33

[Bob Owen (:bobowen)]

Attachment: Part 10: Replace nsCxPusher in XRE_XPCShellMain v2

r=bholley - from #h23, fixed comment.

Comment 34

[Bob Owen (:bobowen)]

Attachment: Part 13: Replace AutoCxPusher in TestShellCommandParent::RunCallback v2

r=bholley - from comment 24

(In reply to Bobby Holley (:bholley) from comment #24)
> Comment on attachment 8469472 [details] [diff] [review]
> Part 13: Replace AutoCxPusher in TestShellCommandParent::RunCallback v1
> 
> Review of attachment 8469472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that change.
> 
> ::: ipc/testshell/TestShellParent.cpp
> @@ +69,4 @@
> >  
> > +  // We're about to run script via JS_CallFunctionValue, so we need an
> > +  // AutoEntryScript. This is just for testing and not in any spec.
> > +  dom::AutoEntryScript aes(xpc::GetNativeForGlobal(xpc::GetSafeJSContextGlobal()));
> 
> Shouldn't we just initialize the AutoEntryScript with
> GetNativeGlobal(mCallback.ToJSObject())? That'd be more meaningful and
> eliminate the JSAutoCompartment below.

Thanks, that makes much more sense.
I had to use GetGlobalForObjectCrossCompartment as well.
I also moved the NS_ENSURE_TRUE(mCallback.ToJSObject(), false); to the top to replace the other one, as it's a stronger requirement anyway and it protects the construction of aes.

Comment 35

[Bob Owen (:bobowen)]

Try push after all the review changes and with Parts 4 and 14 removed:
https://tbpl.mozilla.org/?tree=Try&rev=7b24a4883663

Please land in Part number order (Part 4 is missing), thanks.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49ac5b0ca16a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e19d389f151f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7220f73ca25e
https://hg.mozilla.org/integration/mozilla-inbound/rev/86c1bd4fac4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1892effbdae
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9c1f5585ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/e587c78c649b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ceaa597c2a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a4ff555b9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b96313c0e42
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba74ce69c786
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e04c828071d

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49ac5b0ca16a
https://hg.mozilla.org/mozilla-central/rev/e19d389f151f
https://hg.mozilla.org/mozilla-central/rev/7220f73ca25e
https://hg.mozilla.org/mozilla-central/rev/86c1bd4fac4a
https://hg.mozilla.org/mozilla-central/rev/b1892effbdae
https://hg.mozilla.org/mozilla-central/rev/6a9c1f5585ab
https://hg.mozilla.org/mozilla-central/rev/e587c78c649b
https://hg.mozilla.org/mozilla-central/rev/0ceaa597c2a9
https://hg.mozilla.org/mozilla-central/rev/62a4ff555b9e
https://hg.mozilla.org/mozilla-central/rev/6b96313c0e42
https://hg.mozilla.org/mozilla-central/rev/ba74ce69c786
https://hg.mozilla.org/mozilla-central/rev/0e04c828071d