Closed
Bug 889714
Opened 11 years ago
Closed 11 years ago
Clean up IPC XPCShellEnvironment.cpp
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Whiteboard: [qa-])
Attachments
(8 files)
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.
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ed9aae2f8f49
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=dc18ce15eb73
Assignee | ||
Comment 3•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6f4344b93e3c
Assignee | ||
Comment 4•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=624100c096d4
Assignee | ||
Comment 5•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=149a8204b37f
Assignee | ||
Comment 6•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4f9b6e6e06ab
Assignee | ||
Comment 7•11 years ago
|
||
lol @ 'ShouldCompoleOnly'.
Attachment #773616 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•11 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 9•11 years ago
|
||
Attachment #773620 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•11 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 11•11 years ago
|
||
Attachment #773625 -
Flags: review?
Assignee | ||
Comment 12•11 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 13•11 years ago
|
||
Attachment #773628 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•11 years ago
|
||
There's no reason we should be doing this.
Attachment #773629 -
Flags: review?(mrbkap)
Assignee | ||
Updated•11 years ago
|
Attachment #773625 -
Flags: review? → review?(mrbkap)
Updated•11 years ago
|
Attachment #773616 -
Flags: review?(mrbkap) → review+
Comment 15•11 years ago
|
||
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•11 years ago
|
Attachment #773620 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 16•11 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 17•11 years ago
|
||
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•11 years ago
|
Attachment #773625 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #773627 -
Flags: review?(mrbkap) → review+
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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+
Comment 20•11 years ago
|
||
(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.
Assignee | ||
Comment 21•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3745b7a8bfb9
Comment 22•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=84b828b63115
status-firefox24:
--- → fixed
Updated•11 years ago
|
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
Assignee | ||
Comment 25•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=5f9484e134f9
Comment 26•11 years ago
|
||
Backed out for xpcshell failures. https://hg.mozilla.org/releases/mozilla-aurora/rev/b3d0c2498b42
Assignee | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=3266c1d73816
Assignee | ||
Updated•11 years ago
|
Comment 28•11 years ago
|
||
Given the extend of your "axing" are there any regressions we should keep an eye out for?
Flags: needinfo?(bobbyholley+bmo)
Assignee | ||
Comment 29•11 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)
You need to log in
before you can comment on or make changes to this bug.
Description
•