Clean up IPC XPCShellEnvironment.cpp

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla25
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed)

Details

(Whiteboard: [qa-])

Attachments

(8 attachments)

Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
Depends on: 889911
Assignee

Comment 8

6 years ago
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)
Assignee

Comment 10

6 years ago
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)
Assignee

Comment 12

6 years ago
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)
Assignee

Comment 14

6 years ago
There's no reason we should be doing this.
Attachment #773629 - Flags: review?(mrbkap)
Assignee

Updated

6 years ago
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+
Assignee

Comment 16

6 years ago
(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)
Assignee

Comment 29

6 years ago
(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.