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?

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.

(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?

A very good plan.

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.

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.

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.

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.

No more xpc::danger, for now. ;-)

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.

(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...

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 :)

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 ;)

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?

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.

Is there some kind of documentation about how this is supposed to work after these patches?

(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.

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.

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.

(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

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 :(