1276276 - Stop using multiple JSContexts per runtime in Gecko

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change AutoEntryScript to always use the safe jscontext instead of trying to find a global-specific one

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 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Eliminate nsIScriptContext::GetNativeContext

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Eliminate the specialness of nsJSContext::GetCCRefcnt, since we're never using its mContext for anything now

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Change AutoCxPusher to not worry about nsIScriptContext, since its JSContext never has one anymore

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Get rid of nsJSContext::mContext

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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 8

[Olli Pettay [:smaug][[e-mail]]]

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 9

[Olli Pettay [:smaug][[e-mail]]]

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 10

[Olli Pettay [:smaug][[e-mail]]]

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 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Carsten Book [:Tomcat]]

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