Closed Bug 1276276 Opened 7 years ago Closed 7 years ago

Stop using multiple JSContexts per runtime in Gecko

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Whiteboard: btpp-active)

Attachments

(6 files)

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.
Depends on: 1276309
Blocks: 1276310
Blocks: 1267297
Depends on: 1276317
Blocks: 1255874
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.
Attachment #8757542 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Blocks: 1276399
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.
Attachment #8757542 - Flags: review?(bugs) → review+
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.)
Attachment #8757543 - Flags: review?(bugs) → review+
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.
Attachment #8757545 - Flags: review?(bugs)
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
Attachment #8757546 - Flags: review?(bugs) → review+
> 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?
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
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
Attachment #8757728 - Flags: review?(bugs)
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.  ;)
Attachment #8757545 - Flags: review?(bugs)
Attachment #8757545 - Flags: review?(bugs) → review+
Attachment #8757728 - Flags: review?(bugs) → review+
Whiteboard: btpp-active
Attachment #8757544 - Flags: review?(peterv) → review+
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
Blocks: 1278223
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.