Closed
Bug 1047509
Opened 10 years ago
Closed 10 years ago
Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 14
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(12 files, 9 obsolete files)
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2374813a9223
* bobowen crosses his fingers
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
Very similar to Part 1.
Attachment #8469431 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
The original callback stuff that you landed should take care of the AutoEntryScript for us here.
Attachment #8469464 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
This is a bit of a hack, but hopefully it's OK given where it is.
Attachment #8469472 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8469431 -
Flags: review?(bobbyholley) → review+
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8469447 -
Flags: review?(bobbyholley) → review+
Comment 19•10 years ago
|
||
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+
Comment 20•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8469452 -
Flags: review?(bobbyholley) → review+
Comment 21•10 years ago
|
||
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 22•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8469463 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8469464 -
Flags: review?(bobbyholley) → review+
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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 25•10 years ago
|
||
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+
Assignee | ||
Comment 26•10 years ago
|
||
(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)
Assignee | ||
Comment 27•10 years ago
|
||
r=bholley - from comment 16.
Fixed comment and * position.
Attachment #8469421 -
Attachment is obsolete: true
Attachment #8469984 -
Flags: review+
Assignee | ||
Comment 28•10 years ago
|
||
r=bholley - from #15.
Fixed comment and * position.
Attachment #8469431 -
Attachment is obsolete: true
Attachment #8469987 -
Flags: review+
Assignee | ||
Comment 29•10 years ago
|
||
r=bholley - from comment 17.
Fixed comment and * position.
Attachment #8469433 -
Attachment is obsolete: true
Attachment #8469988 -
Flags: review+
Assignee | ||
Comment 30•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8469478 -
Attachment is obsolete: true
Assignee | ||
Comment 31•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 32•10 years ago
|
||
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+
Assignee | ||
Comment 33•10 years ago
|
||
r=bholley - from #h23, fixed comment.
Attachment #8469463 -
Attachment is obsolete: true
Attachment #8470006 -
Flags: review+
Assignee | ||
Comment 34•10 years ago
|
||
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+
Assignee | ||
Comment 35•10 years ago
|
||
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
Comment 36•10 years ago
|
||
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
Keywords: checkin-needed
Comment 37•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•