Clean up IPC XPCShellEnvironment.cpp

RESOLVED FIXED in Firefox 24

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

unspecified
mozilla25
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed)

Details

(Whiteboard: [qa-])

Attachments

(8 attachments)

7.50 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.11 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.84 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.88 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.54 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.85 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.61 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
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
https://tbpl.mozilla.org/?tree=Try&rev=ed9aae2f8f49
https://tbpl.mozilla.org/?tree=Try&rev=dc18ce15eb73
https://tbpl.mozilla.org/?tree=Try&rev=6f4344b93e3c
https://tbpl.mozilla.org/?tree=Try&rev=624100c096d4
https://tbpl.mozilla.org/?tree=Try&rev=149a8204b37f
https://tbpl.mozilla.org/?tree=Try&rev=4f9b6e6e06ab
Created attachment 773616 [details] [diff] [review]
Part 1 - Removed unused ShouldReportWarnings and ShouldCompoleOnly [sic] machinery. v1

lol @ 'ShouldCompoleOnly'.
Attachment #773616 - Flags: review?(mrbkap)
Created attachment 773617 [details] [diff] [review]
Part 2 - Remove unused ExitCode machinery. v1

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)
Created attachment 773620 [details] [diff] [review]
Part 3 - Reduce the number of places where we pull |env| off cx. v1
Attachment #773620 - Flags: review?(mrbkap)
Created attachment 773624 [details] [diff] [review]
Part 4 - Stash the XPCShellEnvironment instance on the global, rather than the cx. v1

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)
Created attachment 773625 [details] [diff] [review]
Part 5 - Stop manually holding onto system JSPrincipals. v1
Attachment #773625 - Flags: review?
Created attachment 773627 [details] [diff] [review]
Part 6 - Use SystemErrorReporter instead of ScriptErrorReporter. v1

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)
Created attachment 773628 [details] [diff] [review]
Part 7 - Use the SafeJSContext in XPCShellEnvironment. v1
Attachment #773628 - Flags: review?(mrbkap)
Created attachment 773629 [details] [diff] [review]
Part 8 - Remove ContextCallback junk. v1

There's no reason we should be doing this.
Attachment #773629 - Flags: review?(mrbkap)
Attachment #773625 - Flags: review? → review?(mrbkap)

Updated

4 years ago
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+

Updated

4 years ago
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+

Updated

4 years ago
Attachment #773625 - Flags: review?(mrbkap) → review+

Updated

4 years ago
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.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3745b7a8bfb9
https://hg.mozilla.org/mozilla-central/rev/d2a8922b7f55
https://hg.mozilla.org/mozilla-central/rev/5383f9b140c0
https://hg.mozilla.org/mozilla-central/rev/fe80ac2b0e25
https://hg.mozilla.org/mozilla-central/rev/35039a0a3316
https://hg.mozilla.org/mozilla-central/rev/2c76f9048c0d
https://hg.mozilla.org/mozilla-central/rev/f248cb25d4db
https://hg.mozilla.org/mozilla-central/rev/068a49936b0c
https://hg.mozilla.org/mozilla-central/rev/3745b7a8bfb9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=84b828b63115
status-firefox24: --- → fixed
status-firefox25: --- → fixed
(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

Updated

4 years ago
status-firefox24: fixed → affected
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=5f9484e134f9
status-firefox24: affected → fixed
Backed out for xpcshell failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3d0c2498b42
status-firefox24: fixed → affected
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=3266c1d73816
status-firefox24: affected → fixed
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.