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

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 9 obsolete attachments)

2.97 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.47 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.12 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.94 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.99 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.11 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.56 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.68 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
3.71 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
7.08 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
5.13 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
3.91 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
Another (well maybe not just another) batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
Created attachment 8469421 [details] [diff] [review]
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.
Attachment #8469421 - Flags: review?(bobbyholley)
Created attachment 8469431 [details] [diff] [review]
Part 2: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_GetProperty v1

Very similar to Part 1.
Attachment #8469431 - Flags: review?(bobbyholley)
Created attachment 8469433 [details] [diff] [review]
Part 3: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_SetProperty v1

ditto
Attachment #8469433 - Flags: review?(bobbyholley)
Created attachment 8469440 [details] [diff] [review]
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.
Attachment #8469440 - Flags: review?(bobbyholley)
Created attachment 8469447 [details] [diff] [review]
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.
Attachment #8469447 - Flags: review?(bobbyholley)
Created attachment 8469450 [details] [diff] [review]
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?
Attachment #8469450 - Flags: review?(bobbyholley)
Created attachment 8469452 [details] [diff] [review]
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).
Attachment #8469452 - Flags: review?(bobbyholley)
Created attachment 8469457 [details] [diff] [review]
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.
Attachment #8469457 - Flags: review?(bobbyholley)
Created attachment 8469461 [details] [diff] [review]
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.
Attachment #8469461 - Flags: review?(bobbyholley)
Created attachment 8469463 [details] [diff] [review]
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).
Attachment #8469463 - Flags: review?(bobbyholley)
Created attachment 8469464 [details] [diff] [review]
Part 11: Remove nsCxPusher in nsPresContext::MediaFeatureValuesChanged v1

The original callback stuff that you landed should take care of the AutoEntryScript for us here.
Attachment #8469464 - Flags: review?(bobbyholley)
Created attachment 8469468 [details] [diff] [review]
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.
Attachment #8469468 - Flags: review?(bobbyholley)
Created attachment 8469472 [details] [diff] [review]
Part 13: Replace AutoCxPusher in TestShellCommandParent::RunCallback v1

This is a bit of a hack, but hopefully it's OK given where it is.
Attachment #8469472 - Flags: review?(bobbyholley)
Created attachment 8469478 [details] [diff] [review]
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.
Attachment #8469478 - Flags: review?(bobbyholley)
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".
Attachment #8469421 - Flags: review?(bobbyholley) → review+
Attachment #8469431 - Flags: review?(bobbyholley) → review+
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).
Attachment #8469433 - Flags: review?(bobbyholley) → review+
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.
Attachment #8469440 - Flags: review?(bobbyholley) → review-
Attachment #8469447 - Flags: review?(bobbyholley) → review+
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.
Attachment #8469450 - Flags: review?(bobbyholley) → review+
(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.
Attachment #8469452 - Flags: review?(bobbyholley) → review+
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?
Attachment #8469457 - Flags: review?(bobbyholley) → review+
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/
Attachment #8469461 - Flags: review?(bobbyholley) → review+
Attachment #8469463 - Flags: review?(bobbyholley) → review+
Attachment #8469464 - Flags: review?(bobbyholley) → review+
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.
Attachment #8469468 - Flags: review?(bobbyholley) → review+
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.
Attachment #8469472 - Flags: review?(bobbyholley) → review+
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/
Attachment #8469478 - Flags: review?(bobbyholley) → review+
(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.)
Flags: needinfo?(bobbyholley)
Created attachment 8469984 [details] [diff] [review]
Part 1: Replace nsCxPusher in nsJSNPRuntime.cpp doInvoke v2

r=bholley - from comment 16.

Fixed comment and * position.
Attachment #8469421 - Attachment is obsolete: true
Attachment #8469984 - Flags: review+
Created attachment 8469987 [details] [diff] [review]
Part 2: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_GetProperty v2

r=bholley - from #15.

Fixed comment and * position.
Attachment #8469431 - Attachment is obsolete: true
Attachment #8469987 - Flags: review+
Created attachment 8469988 [details] [diff] [review]
Part 3: Replace nsCxPusher in nsJSNPRuntime.cpp nsJSObjWrapper::NP_SetProperty v2

r=bholley - from comment 17.

Fixed comment and * position.
Attachment #8469433 - Attachment is obsolete: true
Attachment #8469988 - Flags: review+
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.
Attachment #8469440 - Attachment is obsolete: true
Flags: needinfo?(bobbyholley)
Attachment #8469478 - Attachment is obsolete: true
Created attachment 8469996 [details] [diff] [review]
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
Attachment #8469450 - Attachment is obsolete: true
Attachment #8469996 - Flags: review+
Attachment #8469452 - Attachment description: Part 8: Change mozJSComponentLoader::LoadModule to use AutoJSAPI and SafeJSContext instead of JSCLContextHelper and its own JSContext v1 → Part 7: Change mozJSComponentLoader::LoadModule to use AutoJSAPI and SafeJSContext instead of JSCLContextHelper and its own JSContext v1
Created attachment 8470005 [details] [diff] [review]
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.
Attachment #8469457 - Attachment is obsolete: true
Attachment #8470005 - Flags: review+
Created attachment 8470006 [details] [diff] [review]
Part 10: Replace nsCxPusher in XRE_XPCShellMain v2

r=bholley - from #h23, fixed comment.
Attachment #8469463 - Attachment is obsolete: true
Attachment #8470006 - Flags: review+
Created attachment 8470009 [details] [diff] [review]
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.
Attachment #8469472 - Attachment is obsolete: true
Attachment #8470009 - Flags: review+
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.
Keywords: checkin-needed
Blocks: 1050795
You need to log in before you can comment on or make changes to this bug.