If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fold JSAutoRequest into nsCxPusher

RESOLVED FIXED in mozilla24

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments, 2 obsolete attachments)

2.33 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
1.14 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
1.32 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
1.15 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
1.07 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
10.06 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
1.13 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
22.58 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
65.35 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
3.12 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
14.68 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
2.41 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
2.29 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.27 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
122.43 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
Now that we force everyone to push a cx before using it, and all pushes go through nsCxPusher, we should be able to replace all the random JSAutoRequests littered throughout gecko with a single JSAutoRequest inside nsCxPusher.

Can anyone think of a reason why this would be a problem?

Comment 1

5 years ago
Maybe performance? nsCxPusher isn't a cheap thing to use, but perhaps JSAutoRequest isn't either.
(In reply to Olli Pettay [:smaug] from comment #1)
> Maybe performance? nsCxPusher isn't a cheap thing to use, but perhaps
> JSAutoRequest isn't either.

JSAutoRequest is quite cheap compared to nsCxPusher.
When do we want to do a request on a cx that is not on the stack? Do we have a use case for that? If so, shouldn't we use there a different API/autoclass that features the fact that we do something tricky?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> When do we want to do a request on a cx that is not on the stack? Do we have
> a use case for that? If so, shouldn't we use there a different API/autoclass
> that features the fact that we do something tricky?

I think we more or less should never have a request for a cx that isn't on the stack.

I was talking with Luke about this on IRC. He said that we should actually just use a JSAutoCompartment to explicitly enter the compartment of the default object (rather than just using the default compartment implicitly), at which point we should no longer need requests at all.

Comment 5

5 years ago
(In reply to Bobby Holley (:bholley) from comment #4)
From IRC: I tried to remove requests altogether last year (see old WIP patch in bug 722345) but got bogged down in the ton of JSAutoRequests scattered about and the problem of how to find the right compartment to enter for each one.  It sounds like the situation is a lot better now so ride with the wind bholley!
So, a good path on this bug might be:

1 - Embed both a JSAutoRequest and a JSAutoCompartment in nsCxPusher.
2 - Remove all the other instances of JSAutoRequest in gecko
3 - Make sure things run without asserting.
4 - Remove the request API and the JSAutoRequest from nsCxPusher.

Luke, does that sound like a plan?

Comment 7

5 years ago
A very good plan.
Duplicate of this bug: 584673
https://tbpl.mozilla.org/?tree=Try&rev=9a0df45a4121
https://tbpl.mozilla.org/?tree=Try&rev=d58b8f18a51b
https://tbpl.mozilla.org/?tree=Try&rev=23e594577e20
https://tbpl.mozilla.org/?tree=Try&rev=ee6d6f4fe8ae
https://tbpl.mozilla.org/?tree=Try&rev=09f0d277452e
https://tbpl.mozilla.org/?tree=Try&rev=4c65bc3ef699
wohoo, green! Uploading patches and flagging for review.

Gabor, feel free to cancel/bounce review on any of these that you don't feel comfortable with.
Created attachment 750889 [details] [diff] [review]
Part 1 - Enter a request in OnJSContextNew. v1
Attachment #750889 - Flags: review?(gkrizsanits)
Created attachment 750892 [details] [diff] [review]
Part 2 - Make sure mContext is stack-top in nsJSContext::CompileScript. v1
Attachment #750892 - Flags: review?(gkrizsanits)
Created attachment 750893 [details] [diff] [review]
Part 3 - Make sure the new cx is stack-top in nsFrameMessageManager. v1
Attachment #750893 - Flags: review?(gkrizsanits)
Created attachment 750894 [details] [diff] [review]
Part 4 - Use an AutoSafeJSContext in hueyfix. v1
Attachment #750894 - Flags: review?(gkrizsanits)
Created attachment 750895 [details] [diff] [review]
Part 5 - Make sure to push a JSContext in TabParent::ReceiveMessage. v1
Attachment #750895 - Flags: review?(gkrizsanits)
Created attachment 750896 [details] [diff] [review]
Part 6 - Use an AutoSafeJSContext in nsObjectLoadingContent::TeardownProtoChain. v1
Attachment #750896 - Flags: review?(gkrizsanits)
Created attachment 750897 [details] [diff] [review]
Part 7 - Make sure mContext is stack-top in the various places where nsJSEnvironment uses it. v1

I also replaced some uses of mJSContext with AutoJSContext upon auditing that
they're only ever called by other nsJSContext functions that just pushed. This
makes the code much easier to audit because we know that we're just inheriting
the caller's cx (which is stack-top) rather than injecting a potentially-
unrelated cx.
Attachment #750897 - Flags: review?(gkrizsanits)
Created attachment 750898 [details] [diff] [review]
Part 8 - Make sure to push the context in the ObjectWrapperChild constructor. v2
Attachment #750898 - Flags: review?(gkrizsanits)
Created attachment 750900 [details] [diff] [review]
Part 9 - Separate nsCxPusher an friends into their own file. v1

We want to put a JSAutoRequest into nsCxPusher, but that would involve
including jsapi.h in nsContentUtils.h, which we'd probably rather avoid doing.
Let's just bite the bullet and do this.
Attachment #750900 - Flags: review?(gkrizsanits)
Created attachment 750901 [details] [diff] [review]
Part 10 - Include nsCxPusher.h everywhere we need it, and stop including it from nsContentUtils.h. v1
Attachment #750901 - Flags: review?(gkrizsanits)
Created attachment 750902 [details] [diff] [review]
Part 11 - Move nsCxPusher into XPConnect. v1

The JSContext stack is an XPConnect construction. In particular, there are
situations where we want to manipulate it outside the lifetime of nsContentUtils
but within the lifetime of the stack itself. In order to do this cleanly, it's
helpful to use private XPConnect APIs. So the first step here is to move this into
js/src/xpconnect, so that we can take advantage of the stuff in xpcprivate.h.
Attachment #750902 - Flags: review?(gkrizsanits)
Created attachment 750903 [details] [diff] [review]
Part 12 - Remove the dependencies of the nsCxPusher machinery on nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1

No more xpc::danger, for now. ;-)
Attachment #750903 - Flags: review?(gkrizsanits)
Created attachment 750905 [details] [diff] [review]
Part 13 - Add an AutoRequest inside nsCxPusher. v1
Attachment #750905 - Flags: review?(gkrizsanits)
Created attachment 750906 [details] [diff] [review]
Part 14 - Remove the lion's share of JSAutoRequests in gecko. v2

There are still a handful that either are used with other runtimes, or that
happen very early/late in cx the lifetime of various things where it doesn't
necessarily make sense to have a cx on the stack. This should definitely ensure
that we're not doing double-duty with the nsCxPusher change, though.
Attachment #750906 - Flags: review?(gkrizsanits)
(In reply to Bobby Holley (:bholley) from comment #16)
> Created attachment 750889 [details] [diff] [review]
> Part 1 - Enter a request in OnJSContextNew. v1

There is already a JSAutoRequest a few lines below yours in the if branch. What does it do now? And why is it important that it goes out of scope before DefineStaticJSVals anyway (see comment there)? I guess it should just go with the comment right?
Comment on attachment 750892 [details] [diff] [review]
Part 2 - Make sure mContext is stack-top in nsJSContext::CompileScript. v1

Review of attachment 750892 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: Any reason you didn't replace mContect with cx here?
XPCAutoRequest ar(mContext);

Not that it should matter much...
Attachment #750892 - Flags: review?(gkrizsanits) → review+
Attachment #750893 - Flags: review?(gkrizsanits) → review+
Attachment #750894 - Flags: review?(gkrizsanits) → review+
Attachment #750895 - Flags: review?(gkrizsanits) → review+
Attachment #750896 - Flags: review?(gkrizsanits) → review+
Comment on attachment 750897 [details] [diff] [review]
Part 7 - Make sure mContext is stack-top in the various places where nsJSEnvironment uses it. v1

Review of attachment 750897 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the helpful description, it took me a while to figure it out what's going on until I noticed it :)
Attachment #750897 - Flags: review?(gkrizsanits) → review+
Attachment #750898 - Flags: review?(gkrizsanits) → review+
Attachment #750900 - Flags: review?(gkrizsanits) → review+
Attachment #750901 - Flags: review?(gkrizsanits) → review+
Attachment #750902 - Flags: review?(gkrizsanits) → review+
Comment on attachment 750903 [details] [diff] [review]
Part 12 - Remove the dependencies of the nsCxPusher machinery on nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1

Review of attachment 750903 [details] [diff] [review]:
-----------------------------------------------------------------

I'm so glad to see that namespace go ;)
Attachment #750903 - Flags: review?(gkrizsanits) → review+
Attachment #750905 - Flags: review?(gkrizsanits) → review+
Created attachment 751710 [details] [diff] [review]
Part 1 - Enter a request for all of OnJSContextNew. v2

Hm, good catch gabor, this is indeed somewhat suspicious. But I'm also not
convinced it's still relevant, especially because the call before it to
JS_SetGCParameterForThread is now a no-op. Peter, do you remember what this
was about? Is this change ok?
Attachment #750889 - Attachment is obsolete: true
Attachment #750889 - Flags: review?(gkrizsanits)
Attachment #751710 - Flags: review?(peterv)
Blocks: 872599
Comment on attachment 750900 [details] [diff] [review]
Part 9 - Separate nsCxPusher an friends into their own file. v1

::: content/base/public/nsCxPusher.h
>+#ifndef nsCxPusher_h___
>+#define nsCxPusher_h___

Please do not add a new symbol containing consecutive underscores.
(In reply to Masatoshi Kimura [:emk] from comment #35)
> Comment on attachment 750900 [details] [diff] [review]
> Part 9 - Separate nsCxPusher an friends into their own file. v1
> 
> ::: content/base/public/nsCxPusher.h
> >+#ifndef nsCxPusher_h___
> >+#define nsCxPusher_h___
> 
> Please do not add a new symbol containing consecutive underscores.

What is the preferred style? If there's some consensus on it, please add it to the style guide.
(In reply to Bobby Holley (:bholley) from comment #36)
> (In reply to Masatoshi Kimura [:emk] from comment #35)
> > Comment on attachment 750900 [details] [diff] [review]
> > Part 9 - Separate nsCxPusher an friends into their own file. v1
> > 
> > ::: content/base/public/nsCxPusher.h
> > >+#ifndef nsCxPusher_h___
> > >+#define nsCxPusher_h___
> > 
> > Please do not add a new symbol containing consecutive underscores.
> 
> What is the preferred style? If there's some consensus on it, please add it
> to the style guide.

The way you include it with punctuation replaced by underscores:

  nsCxPusher_h
  mozilla_dom_Foo_h
(In reply to Bobby Holley (:bholley) from comment #29)
> Created attachment 750906 [details] [diff] [review]
> Part 14 - Remove the lion's share of JSAutoRequests in gecko. v2
> 

These are the ones where I think we need a cx push:

nsXPConnect::CheckForDebugMode
BluetoothService::Notify
BroadcastSystemMessage

And these are the ones where I'm not sure... I might be missing something obvious, but I don't see the reason why we shouldn't

TabChild::DispatchMessageManagerMessage
NPStringIdentifierIsPermanent
PluginScriptableObjectParent::AnswerEnumerate

The rest looks good, so if you can fix/explain me these I'll r+ it.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #38)
> (In reply to Bobby Holley (:bholley) from comment #29)
> > Created attachment 750906 [details] [diff] [review]
> > Part 14 - Remove the lion's share of JSAutoRequests in gecko. v2
> > 
> 
> These are the ones where I think we need a cx push:
> 
> nsXPConnect::CheckForDebugMode

This gets called by the pushing machinery, so we can't.

> BluetoothService::Notify
> BroadcastSystemMessage

I will add these.

> 
> And these are the ones where I'm not sure... I might be missing something
> obvious, but I don't see the reason why we shouldn't
> 
> TabChild::DispatchMessageManagerMessage

Hm, you're right. We probably need an AutoPushJSContext for mCx there.

> NPStringIdentifierIsPermanent

We should just be using an AutoJSContext there. The cx is only used in that call to access the runtime.

> PluginScriptableObjectParent::AnswerEnumerate

Should probably use an AutoPushJSContext there.

Great catches! I'll throw those into a patch and flag you for review.
(In reply to Bobby Holley (:bholley) from comment #39)
> > TabChild::DispatchMessageManagerMessage
> 
> Hm, you're right. We probably need an AutoPushJSContext for mCx there.

Actually, TabChild does a lot of funny stuff with the JSContext. For the usage right here, I think we should just use the SafeJSContext.
Created attachment 752442 [details] [diff] [review]
Part 13.5 - Fix sketchy cx consumers identified by gabor. v1
Attachment #752442 - Flags: review?(gkrizsanits)
Created attachment 752444 [details] [diff] [review]
Part 14 - Remove the lion's share of JSAutoRequests in gecko. v4
Attachment #750906 - Attachment is obsolete: true
Attachment #750906 - Flags: review?(gkrizsanits)
Attachment #752444 - Flags: review?(gkrizsanits)
Is there some kind of documentation about how this is supposed to work after these patches?
https://tbpl.mozilla.org/?tree=Try&rev=84e3a1ab3263
(In reply to Andrew McCreight [:mccr8] from comment #43)
> Is there some kind of documentation about how this is supposed to work after
> these patches?

What is "this"?

In general, this stuff is all changing rapidly, and the invariants are in flux with the various patches I have in my repos. So for the mean time, the best documentation is probably "ask bholley". :-)
Comment on attachment 750903 [details] [diff] [review]
Part 12 - Remove the dependencies of the nsCxPusher machinery on nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1

Is bug 864363 still needed?
(In reply to Masatoshi Kimura [:emk] from comment #46)
> Comment on attachment 750903 [details] [diff] [review]
> Part 12 - Remove the dependencies of the nsCxPusher machinery on
> nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1
> 
> Is bug 864363 still needed?

Yeah, I think so. The issue in that bug was that Layout (which includes both XPConnect and nsContentUtils) hadn't been spun up yet. nsCxPusher still depends on XPConnect, so the same issue applies.

Note that Trevor is making XPConnect initialization non-lazy in bug 873622, so it'll hopefully start up predictably in nsLayoutStatics::Init.
Comment on attachment 752442 [details] [diff] [review]
Part 13.5 - Fix sketchy cx consumers identified by gabor. v1

Review of attachment 752442 [details] [diff] [review]:
-----------------------------------------------------------------

+    AutoSafeJSContext cx;
+    JSAutoRequest ar(cx);

You forgot JSAutoRequest in a few of places. Since AutoSafeJSContext always pushes, those could/should go.
Attachment #752442 - Flags: review?(gkrizsanits) → review+
Comment on attachment 752444 [details] [diff] [review]
Part 14 - Remove the lion's share of JSAutoRequests in gecko. v4

Review of attachment 752444 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bobby Holley (:bholley) from comment #39)
> > nsXPConnect::CheckForDebugMode
> 
> This gets called by the pushing machinery, so we can't.

I guess I got a bit mechanical when I got there, didn't realize it. This one still slightly worries me. I don't see any good reason here why we can just remove that JSAutoRequest. This should at very least go into a separate patch and some comment would be nice too in case it causes something weird (also since this is one of the few cases where we call a function with a JSContext* argument that is not on the stack). I have no idea what else we could do here. r+ the rest, but this one might be a good idea to be checked by someone other than me, unless you know exactly why it's safe to remove that request.
Attachment #752444 - Flags: review?(gkrizsanits) → review+
Comment on attachment 751710 [details] [diff] [review]
Part 1 - Enter a request for all of OnJSContextNew. v2

I don't know why we did it that way. DefineStaticJSVals and InternStaticDictionaryJSVals both have a JSAutoRequest which isn't really needed anymore, but might be safer to keep them.
Attachment #751710 - Flags: review?(peterv) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #49)
> This should at very least go into a separate patch

Bug 872135 addressed the problem.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #48)
> +    AutoSafeJSContext cx;
> +    JSAutoRequest ar(cx);
> 
> You forgot JSAutoRequest in a few of places. Since AutoSafeJSContext always
> pushes, those could/should go.

Yeah, they're removed in the next patch. This patch goes underneath patch 14.
This seems to bitrot every 10 minutes. Pushing to inbound and crossing my fingers it's still green like it was in comment 44.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6869a88c9461
https://hg.mozilla.org/mozilla-central/rev/a5959b273b65
https://hg.mozilla.org/mozilla-central/rev/a0e74ae591bd
https://hg.mozilla.org/mozilla-central/rev/f684730b3678
https://hg.mozilla.org/mozilla-central/rev/6d4aaf1758b3
https://hg.mozilla.org/mozilla-central/rev/a90a8cda1805
https://hg.mozilla.org/mozilla-central/rev/fe05e84ac7a0
https://hg.mozilla.org/mozilla-central/rev/71a10dfafaad
https://hg.mozilla.org/mozilla-central/rev/fa8a26e562b7
https://hg.mozilla.org/mozilla-central/rev/06f68e1253c3
https://hg.mozilla.org/mozilla-central/rev/4d80fdd4a55e
https://hg.mozilla.org/mozilla-central/rev/a8c7766cd507
https://hg.mozilla.org/mozilla-central/rev/21786120e0ea
https://hg.mozilla.org/mozilla-central/rev/3e1d6dd0b814
https://hg.mozilla.org/mozilla-central/rev/91c863ec3051
https://hg.mozilla.org/mozilla-central/rev/bd355364bc33
https://hg.mozilla.org/mozilla-central/rev/b96fa35bafe8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
So the assumption that if you call a function and pass a JSContext* cx argument, you always make sure that cx is pushed to the stack, seems to be broken all over the place. In Bug 875251 it seems it's a GetContextForEventHandlers + sc->GetNativeContext() pair in BluetoothAdapter that is used un-pushed and causes a crash, but there are many other places as well... Just a quick search on mxr "JScontext* cx =" shows quite a lot of suspicious cases :(

Updated

4 years ago
Depends on: 875790

Updated

4 years ago
Assignee: nobody → bobbyholley+bmo
Depends on: 883301
Depends on: 882897
Depends on: 886174
You need to log in before you can comment on or make changes to this bug.