At this point, JSContexts store the following Gecko-accessible state: 1) The "private is isupports" options boolean. This is used only in GetScriptContextFromJSContext, which is tracked by bug 1255874 but in practice is only used for keeping JSContexts alive once bug 1276112 and bug 1276133 land. 2) The "dontReportUncaught" boolean. I believe this is dead code from Gecko's point of view: the only place that checks it also checks autoJSAPIOwnsErrorReporting and the latter is always set in Gecko. 3) autoJSAPIOwnsErrorReporting option. This is set via RAII things, and always set in practice in Gecko when we're working with a JSContext, so doesn't preclude changing which exact JSContext we're using. 4) The JSContext private. This is used only when "private is isupports" is used. 5) Pending exception. This should be dealt with automatically. 6) Current compartment. This only matters in practice, I think, when using AutoJSContext, which uses the JSContext stack, but anything that pushes things on that stack will also enter relevant compartments as needed. 7) The _second_ JSContext private (wtf?). In Gecko this is an XPCContext. This stores some state about exceptions and that's it; I _think_ it's OK if we just reuse this more aggressively. Given that, I think we can just go ahead and use the "safe js context" for everything on the main thread. Then we can start deleting various things above.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed3d2c0aa0df is a try run

Olli, I'd ask Bobby to review this, but he's on PTO. If you don't want to spend the time working through this stuff, please just tell me and I'll wait for him to get back.

Comment on attachment 8757542 [details] [diff] [review] part 1. Change AutoEntryScript to always use the safe jscontext instead of trying to find a global-specific one nah, it is good to understand what setup we will have. So trying to review the patches.

Comment on attachment 8757543 [details] [diff] [review] part 2. Eliminate nsIScriptContext::GetNativeContext >-WindowStateHolder::WindowStateHolder(nsIScriptContext* aContext, >- nsGlobalWindow* aWindow) >+WindowStateHolder::WindowStateHolder(nsGlobalWindow* aWindow) > : mInnerWindow(aWindow), >- mInnerWindowReflector(aContext->GetNativeContext(), aWindow->GetWrapper()) >+ mInnerWindowReflector(nsContentUtils::RootingCx(), aWindow->GetWrapper()) I assume we'll get rid of nsContentUtils::RootingCx() and will just have some GetJSContext() (and later just GetJSRuntime() ?) (Oh, mInnerWindowReflector is PersistentRooted. I wonder why. Not about this bug though.)

Comment on attachment 8757545 [details] [diff] [review] part 4. Change AutoCxPusher to not worry about nsIScriptContext, since its JSContext never has one anymore Hmm, need to think this some more. nsCOMPtr<nsIScriptContext> mScx; after all ends up keeping mGlobalObjectRef alive, so outer window stays alive.

Comment on attachment 8757545 [details] [diff] [review] part 4. Change AutoCxPusher to not worry about nsIScriptContext, since its JSContext never has one anymore Could you explain why it is safe to not keep the outer window alive? Or do we end up keeping it alive somehow via AutoJSAPI::mAutoNullableCompartment ? Explain and ask review again.

Comment on attachment 8757546 [details] [diff] [review] part 5. Get rid of nsJSContext::mContext ok,GetScriptContextFromJSContext is removed in the previous patch, so there is no Gecko usage for JS_GetContextPrivate

> I assume we'll get rid of nsContentUtils::RootingCx() and will just have some GetJSContext() > (and later just GetJSRuntime() ?) Yeah, I think we can do all sorts of cleanup here later on. Need to think a bit about how to name APIs appropriately. > Could you explain why it is safe to not keep the outer window alive? Hmm. So to be clear, the actual behavior change is in part 1, where we stop using a JSContext that has an associated nsIScriptContext. The change in part 4 is just dead code removal. But as to the substance of the question... That's a good question. I don't think mAutoNullableCompartment reliably keeps anything alive. While we're in that compartment, the JSContext will keep its global alive, but if some other compartment is entered, that will no longer be the case. That actually seems like an annoying footgun. Anyway, it seems like it would be a good idea to have the AutoJSAPI either hold a strong ref to the nsIGlobalObject or put its JSObject* into a Rooted. AutoEntryScript already holds a strong nsIGlobalObject ref, of course, but we can end up doing gc under AutoJSAPI. So ok, we can change this to keep the _inner_ window alive, but it will require a change. nsPIDOMWindow::mOuterWindow is a strong ref and looks like it's never cleared by anything except CC unlink, so that will keep the outer window alive too. Does that sound reasonable? Any preference between Rooted for the JSObject or refcounting the nsIGlobalObject?

(In reply to Boris Zbarsky [:bz] from comment #12) > Yeah, I think we can do all sorts of cleanup here later on. Need to think a > bit about how to name APIs appropriately. Sure. Something to do later, in a different bug. > So ok, we can change this to keep the _inner_ window alive, but it will > require a change. nsPIDOMWindow::mOuterWindow is a strong ref and looks > like it's never cleared by anything except CC unlink, so that will keep the > outer window alive too. Does that sound reasonable? yes > Any preference between > Rooted for the JSObject or refcounting the nsIGlobalObject? Not really. Looks like AutoJSAPI::InitInternal gets the JSObject for the global so Rooted might be simpler.

I just realized we can't use Rooted, because we don't have a JSContext until InitInternal is called, which means we could construct another Rooted, then call Init, then call that Rooted's dtor, then call our dtor, violating stack discipline. We can change this by moving the getting of the JSContext to the constructor, but I'd prefer to do that sort of surgery in a followup, I think, so for now I'll add a RefPtr<nsIGlobalObject>.

This slides under the rest of the patches, which get renumbered accordingly

Comment on attachment 8757545 [details] [diff] [review] part 4. Change AutoCxPusher to not worry about nsIScriptContext, since its JSContext never has one anymore By the way, feel free to steal the part 3 review too, if you know that code. ;)

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/20835baaf80f part 1. Make AutoJSAPI hold a strong ref to the nsIGlobalObject it's initialized with, so it won't go away while we're working with it. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/f70ca2787f50 part 2. Change AutoEntryScript to always use the safe jscontext instead of trying to find a global-specific one. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/d075887bfd94 part 3. Eliminate nsIScriptContext::GetNativeContext. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca18abcf32d part 4. Eliminate the specialness of nsJSContext::GetCCRefcnt, since we're never using its mContext for anything now. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/33274da6962d part 5. Change AutoCxPusher to not worry about nsIScriptContext, since its JSContext never has one anymore. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/1e95ef42c759 part 6. Get rid of nsJSContext::mContext. r=smaug

https://hg.mozilla.org/mozilla-central/rev/20835baaf80f https://hg.mozilla.org/mozilla-central/rev/f70ca2787f50 https://hg.mozilla.org/mozilla-central/rev/d075887bfd94 https://hg.mozilla.org/mozilla-central/rev/4ca18abcf32d https://hg.mozilla.org/mozilla-central/rev/33274da6962d https://hg.mozilla.org/mozilla-central/rev/1e95ef42c759