Closed Bug 889714 Opened 11 years ago Closed 11 years ago

Clean up IPC XPCShellEnvironment.cpp

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Whiteboard: [qa-])

Attachments

(8 files)

This thing is always a thorn in my side, and its weirdness is causing me pain in bug 887334. I'm just going to try taking an axe to it and see how far I get.
Depends on: 889911
This stuff is cribbed from xpcshell, where it makes sense. But it doens't really
make sense in the IPC context, and doesn't appear to be used either.
Attachment #773617 - Flags: review?(mrbkap)
We need to stash it somewhere, because the shell-provided JSNatives need to
access it. This solution only works for script running in the scope of the
global we create. Butthat's fine here, because we only use it for load() and
quit(), which only exist in the scope of that global.
Attachment #773624 - Flags: review?(mrbkap)
SystemErrorReporter is the new unified error reporter for everything non-dom.
In particular, it's used by the SafeJSContext, which we'll be switching to
here shortly.
Attachment #773627 - Flags: review?(mrbkap)
There's no reason we should be doing this.
Attachment #773629 - Flags: review?(mrbkap)
Attachment #773625 - Flags: review? → review?(mrbkap)
Attachment #773616 - Flags: review?(mrbkap) → review+
Comment on attachment 773617 [details] [diff] [review]
Part 2 - Remove unused ExitCode machinery. v1

This patch is fine on its own, but is there anybody who can confirm that we don't actually use this exit code?
Attachment #773617 - Flags: review?(mrbkap) → review+
Attachment #773620 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #15)
> Comment on attachment 773617 [details] [diff] [review]
> Part 2 - Remove unused ExitCode machinery. v1
> 
> This patch is fine on its own, but is there anybody who can confirm that we
> don't actually use this exit code?

Well, I think the patch proves that, because the value never escapes. XPCShellEnvironment isn't actually a standalone binary (like xpcshell). So there's no return code passed to the shell or anything. And since this compiles, we can conclude that nobody was using the accessor here.
Comment on attachment 773624 [details] [diff] [review]
Part 4 - Stash the XPCShellEnvironment instance on the global, rather than the cx. v1

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

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +79,5 @@
> +    JSAutoCompartment ac(cx, global);
> +    Rooted<Value> v(cx);
> +    if (!JS_GetProperty(cx, global, "__XPCShellEnvironment", v.address()))
> +        return nullptr;
> +    return static_cast<XPCShellEnvironment*>(v.get().toPrivate());

Please do a sanity check to make sure v is actually a private jsval.

@@ +791,5 @@
>          JSAutoRequest ar(cx);
>          JSAutoCompartment ac(cx, globalObj);
>  
> +        if (!JS_DefineProperty(cx, globalObj, "__XPCShellEnvironment",
> +                               PRIVATE_TO_JSVAL(this), JS_PropertyStub,

This is safe because this is malloc'd right?

@@ +799,1 @@
>  	    !JS_DefineProfilingFunctions(cx, globalObj)) {

Nit: nuke the tab on the DefineProfilingFunctions line.
Attachment #773624 - Flags: review?(mrbkap) → review+
Attachment #773625 - Flags: review?(mrbkap) → review+
Attachment #773627 - Flags: review?(mrbkap) → review+
Comment on attachment 773628 [details] [diff] [review]
Part 7 - Use the SafeJSContext in XPCShellEnvironment. v1

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

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +708,2 @@
>  
>    JSScript* script =

Uber-nit: extra blank line here.
Attachment #773628 - Flags: review?(mrbkap) → review+
Comment on attachment 773629 [details] [diff] [review]
Part 8 - Remove ContextCallback junk. v1

This was probably useful in the regular JS shell for things like evalcx that actually created new contexts that needed to have their error reporters set on them. For xpcshell and ipcshell it isn't useful.
Attachment #773629 - Flags: review?(mrbkap) → review+
(In reply to Bobby Holley (:bholley) from comment #16)
> Well, I think the patch proves that, because the value never escapes.
> XPCShellEnvironment isn't actually a standalone binary (like xpcshell). So
> there's no return code passed to the shell or anything. And since this
> compiles, we can conclude that nobody was using the accessor here.

Oh, oops. I thought this was a standalone binary. You're clearly right, then.
(In reply to Bobby Holley (:bholley) from comment #23)
> https://hg.mozilla.org/releases/mozilla-aurora/
> pushloghtml?changeset=84b828b63115

Backed out from Aurora for possibly causing xpcshell crashes along with the other changes from bholley's push in https://hg.mozilla.org/releases/mozilla-aurora/rev/659b0d61fbc6
Given the extend of your "axing" are there any regressions we should keep an eye out for?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #28)
> Given the extend of your "axing" are there any regressions we should keep an
> eye out for?

No. This is all test shell stuff. It's used in automation but not shipped to users.
Flags: needinfo?(bobbyholley+bmo)
Thanks Bobby, flagging as [qa-].
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.