Closed Bug 860438 Opened 7 years ago Closed 7 years ago

Remove custom cx-stack munging scattered around Gecko

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(7 files)

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.
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
Huh, linux64 try builders appear to be busted with autoconf stuff. Repushed macosx64:

https://tbpl.mozilla.org/?tree=Try&rev=f4ad719c25a1
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
And...linux64 try builders still busted.

https://tbpl.mozilla.org/?tree=Try&rev=36a94e54a3c7
Ah, so it's not even the patch that I thought it was.

https://tbpl.mozilla.org/?tree=Try&rev=6bf6e78d71f1
Sheesh, _finally_. Uploading patches and flagging gabor for review.
Attachment #737643 - Flags: review?(gkrizsanits)
This patch should not change any behavior.
Attachment #737644 - Flags: review?(gkrizsanits)
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)
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)
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)
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)
Attachment #737650 - Flags: review?(gkrizsanits)
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)
Attachment #737647 - Flags: review?(gkrizsanits) → review?(bent.mozilla)
Attachment #737648 - Flags: review?(benjamin) → review+
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+
Blocks: 862404
(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.
(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 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 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 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+
Apparently bent is super busy right now. Switching the workers review to gabor.
Attachment #737645 - Flags: review?(bent.mozilla) → review?(gkrizsanits)
Attachment #737647 - Flags: review?(bent.mozilla) → review?(gkrizsanits)
(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?
(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 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+
Attachment #737647 - Flags: review?(gkrizsanits) → review+
Depends on: 863646
bug  863646 has rendered me unable to run today's nightly on Fedora 18 x86_64.
Use -P <name> or -profile <path> as a workaround. bholley is on it.
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 → ---
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/776a69c7f3eb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 865729
You need to log in before you can comment on or make changes to this bug.