Closed
Bug 860438
Opened 11 years ago
Closed 11 years ago
Remove custom cx-stack munging scattered around Gecko
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(7 files)
56.95 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
10.01 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
4.78 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
7.44 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
7.50 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
5.09 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
4.04 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
Probably due to old linkage constraints, there's an insane amount of old, clunky, unsafe, and mostly-redundant-but-ever-so-slightly-different JSContext manipulation scattered throughout gecko. I want to change the call signature of nsIJSContextStack::Pop in bug 860085, but doing that currently requires changing a lot of callsites. So I'm taking the opportunity to fix most of those call sites to use our new, simple, and well-understood RAII classes. This will generally facilitate the process of deprecating JSContexts.
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5fa65410adf7
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ecbe11350cc8
Assignee | ||
Comment 3•11 years ago
|
||
Looks like I messed up the T-push syntax. But at least we know everything builds. Unit test run: https://tbpl.mozilla.org/?tree=Try&rev=8a4dc6ea55bc
Assignee | ||
Comment 4•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e1a58d04c200
Assignee | ||
Comment 5•11 years ago
|
||
Huh, linux64 try builders appear to be busted with autoconf stuff. Repushed macosx64: https://tbpl.mozilla.org/?tree=Try&rev=f4ad719c25a1
Assignee | ||
Comment 6•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a15f839a6c55
Assignee | ||
Comment 7•11 years ago
|
||
Hrm, I'm really stumped on these JSD failures, which are 100% reproducible on try but 0% reproducible locally. Pushing without the JSD changes to make sure that's the issue. https://tbpl.mozilla.org/?tree=Try&rev=a27ec03167c7
Assignee | ||
Comment 8•11 years ago
|
||
And...linux64 try builders still busted. https://tbpl.mozilla.org/?tree=Try&rev=36a94e54a3c7
Assignee | ||
Comment 9•11 years ago
|
||
Ah, so it's not even the patch that I thought it was. https://tbpl.mozilla.org/?tree=Try&rev=6bf6e78d71f1
Assignee | ||
Comment 10•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=624c6baf6b9c
Assignee | ||
Comment 11•11 years ago
|
||
Sheesh, _finally_. Uploading patches and flagging gabor for review.
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #737643 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 13•11 years ago
|
||
This patch should not change any behavior.
Attachment #737644 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 14•11 years ago
|
||
The only functional difference here is the removal of a bug in which we were constructing a dom::workers::AutoSafeJSContext around aCx, but then continuing to pass aCx to the callee. If aCx were to be null, we'd end up with a mismatch between the stack and the active cx. But it looks likes stuff depends on cx being non-null anyway, so that probably never happened.
Attachment #737645 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 15•11 years ago
|
||
The only functional difference here is that AutoSafeJSContext entered a request. Really, we should always enter a request and a compartment at the same time. But for now we just preserve the old behavior with a JSAutoRequest.
Attachment #737647 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 16•11 years ago
|
||
The old code does two little bits of special sauce that are worth mentioning: 1 - It calls OnWrapper{Created,Destroyed} to maintain the lifetime of the addref'd context stack pointer. But that whole thing is gone now. 2 - It calls ScriptEvaluated to potentially invoke termination functions in certain cases. nsCxPusher does this too, but with slightly different logic. In particular, nsCxPusher checks whether the given JSContext was already on the stack, whereas AutoCXPusher checked whether there was another cx on the stack above this one. As far as I can tell from my investigations of this stuff, this is all total voodoo, and I think it should probably be fine to just use an nsCxPusher here. Also, termination functions are going away soon in bug 841312.
Attachment #737648 -
Flags: review?(benjamin)
Assignee | ||
Comment 17•11 years ago
|
||
Some of this existing code is little wacky in that it calls Environment(mCx) in a non-static method, which I would think would be equivalent to |this|. But I don't know this code well enough to be sure of that, so I'm just going to do the careful thing.
Attachment #737649 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #737650 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 737645 [details] [diff] [review] Part 3 - Simplify JSContext handling {Cancel,Suspend,Resume}WorkersForWindow. v1 On second thought, bent should probably review the workers changes.
Attachment #737645 -
Flags: review?(gkrizsanits) → review?(bent.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #737647 -
Flags: review?(gkrizsanits) → review?(bent.mozilla)
Updated•11 years ago
|
Attachment #737648 -
Flags: review?(benjamin) → review+
Comment 20•11 years ago
|
||
Comment on attachment 737643 [details] [diff] [review] Part 1 - Straightforward cases. v1 Review of attachment 737643 [details] [diff] [review]: ----------------------------------------------------------------- In the nsCxPusher if the cx stack is null we should crash.
Attachment #737643 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20) > In the nsCxPusher if the cx stack is null we should crash. Followup is ok, right? I filed bug 862404.
Comment 22•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #21) > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20) > > In the nsCxPusher if the cx stack is null we should crash. > > Followup is ok, right? I filed bug 862404. Sure.
Comment 23•11 years ago
|
||
Comment on attachment 737644 [details] [diff] [review] Part 2 - Remove context stack craziness from nsWindowWatcher. v1 Review of attachment 737644 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup.
Attachment #737644 -
Flags: review?(gkrizsanits) → review+
Comment 24•11 years ago
|
||
Comment on attachment 737649 [details] [diff] [review] Part 6 - Remove custom AutoPusher in ipc XPCShellEnvironment. v1 Review of attachment 737649 [details] [diff] [review]: ----------------------------------------------------------------- + JSContext* GetJSContext() { return mCx; } The style here is a bit off compared to the other one-liners in this file even though I prefer this form in general. The real problem I have here is there is already a function named GetContext that does the same thing. https://mxr.mozilla.org/mozilla-central/source/ipc/testshell/XPCShellEnvironment.h#44 Please fix this.
Attachment #737649 -
Flags: review?(gkrizsanits) → review+
Comment 25•11 years ago
|
||
Comment on attachment 737650 [details] [diff] [review] Part 7 - Use nsCxPusher in JSD. v2 Review of attachment 737650 [details] [diff] [review]: ----------------------------------------------------------------- + while (NS_SUCCEEDED(rv) && mNestedLoopLevel >= nestLevel) { + if (!NS_ProcessNextEvent(thread)) + rv = NS_ERROR_UNEXPECTED; + } ^ indentation error
Attachment #737650 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 26•11 years ago
|
||
Apparently bent is super busy right now. Switching the workers review to gabor.
Assignee | ||
Updated•11 years ago
|
Attachment #737645 -
Flags: review?(bent.mozilla) → review?(gkrizsanits)
Assignee | ||
Updated•11 years ago
|
Attachment #737647 -
Flags: review?(bent.mozilla) → review?(gkrizsanits)
Comment 27•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #14) > Created attachment 737645 [details] [diff] [review] > Part 3 - Simplify JSContext handling > {Cancel,Suspend,Resume}WorkersForWindow. v1 > > The only functional difference here is the removal of a bug in which we were > constructing a dom::workers::AutoSafeJSContext around aCx, but then > continuing > to pass aCx to the callee. If aCx were to be null, we'd end up with a > mismatch > between the stack and the active cx. But it looks likes stuff depends on cx > being non-null anyway, so that probably never happened. But in the non-null case the line you removed did a push and a start request for cx, now we don't do that... In the next patch in somewhat similar cases (in the macro) you do the push and the start request, I don't see why you want to remove it from these places. By the way, why do we need this exactly (push/start request)? I can see an object being rooted with that Cx, is there anything else?
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #27) > (In reply to Bobby Holley (:bholley) from comment #14) > > Created attachment 737645 [details] [diff] [review] > > Part 3 - Simplify JSContext handling > > {Cancel,Suspend,Resume}WorkersForWindow. v1 > > > > The only functional difference here is the removal of a bug in which we were > > constructing a dom::workers::AutoSafeJSContext around aCx, but then > > continuing > > to pass aCx to the callee. If aCx were to be null, we'd end up with a > > mismatch > > between the stack and the active cx. But it looks likes stuff depends on cx > > being non-null anyway, so that probably never happened. > > But in the non-null case the line you removed did a push and a start request > for cx, now we don't do that... In the next patch in somewhat similar cases > (in the macro) you do the push and the start request, I don't see why you > want to remove it from these places. I just hoisted it up the callstack a little bit. See the JSAutoRequests added in nsGlobalWindow. > By the way, why do we need this exactly (push/start request)? I can see an > object being rooted with that Cx, is there anything else? The push is necessary so that exceptions encountered in this stuff get reported on the appropriate DOM window. The request is more or less always supposed to happen before calling into the JS engine. The request API is going away at some point.
Comment 29•11 years ago
|
||
Comment on attachment 737645 [details] [diff] [review] Part 3 - Simplify JSContext handling {Cancel,Suspend,Resume}WorkersForWindow. v1 Review of attachment 737645 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Bobby Holley (:bholley) from comment #28) > I just hoisted it up the callstack a little bit. See the JSAutoRequests > added in nsGlobalWindow. Sorry about that, I failed to realize that that's the only consumer of these functions. Looks good.
Attachment #737645 -
Flags: review?(gkrizsanits) → review+
Updated•11 years ago
|
Attachment #737647 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 30•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0124f27361 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef58074cedfd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c080eb83983 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fbf3dcab515 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c14d7d33f3e7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dd382d615402 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4be115c79b50 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba928cbd5191
Comment 31•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9f0124f27361 https://hg.mozilla.org/mozilla-central/rev/ef58074cedfd https://hg.mozilla.org/mozilla-central/rev/7c080eb83983 https://hg.mozilla.org/mozilla-central/rev/9fbf3dcab515 https://hg.mozilla.org/mozilla-central/rev/c14d7d33f3e7 https://hg.mozilla.org/mozilla-central/rev/dd382d615402 https://hg.mozilla.org/mozilla-central/rev/4be115c79b50 https://hg.mozilla.org/mozilla-central/rev/ba928cbd5191
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment 32•11 years ago
|
||
bug 863646 has rendered me unable to run today's nightly on Fedora 18 x86_64.
Comment 33•11 years ago
|
||
Use -P <name> or -profile <path> as a workaround. bholley is on it.
Assignee | ||
Comment 34•11 years ago
|
||
Backed out the nsWindowWatcher patch for now so that the startup crashes go away: https://hg.mozilla.org/mozilla-central/rev/dd03d42b01b1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•11 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 35•11 years ago
|
||
Relanded the backed-out patch to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/776a69c7f3eb
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/776a69c7f3eb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•