1292892 - Stop using JSRuntime outside SpiderMonkey

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jan de Mooij [:jandem]]

I have a pretty big (bust mostly trivial) patch for this, it also removes JS_GetRuntime and JS_GetContext.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Full patch

I'll cut this up to make it easier to review.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: JS changes

A number of straight-forward changes.

I moved the root list from ContextFriendFields into a js::RootingContext base class. The idea is that we can then use RootingContext in Gecko code where we don't want to expose the full JSRuntime.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Memory profiler

Kan-Ru, this is just using JSContext instead of JSRuntime in the memory profiler code. They're nowadays more or less the same thing and we want to use JSContext everywhere.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Profiler, devtools

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Misc changes

Boris, this is a big patch, sorry for that. The changes are not that complicated though:

* It removes CycleCollectedJSRuntime::mJSRuntime and its accessor methods.

* dom::GetJSRuntime is now dom::GetJSRootingContext (js::RootingContext is a tiny class that has just the context's root lists)

* xpc::GetJSRuntime is now xpc::danger::GetJSContext (like dom::danger::GetJSContext).

* We pass/store JSContext instead of JSRuntime in a number of places.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I still need to review most of this, but one comment about the rooting context setup: we should get rid of nsContentUtils::RootingCx in favor of this new method (which should probably be called mozilla::dom::RootingCx).  That should happen in a separate bug, though, because it will require a bunch of changes, not least because not all of the JS engine's rooting APIs take a RootingContext.  :(

I see no reason to have xpc::danger::GetJSContext _and_ dom::danger::GetJSContext.  Will need to read the patch in more detail to see why we have both.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> I see no reason to have xpc::danger::GetJSContext _and_
> dom::danger::GetJSContext.  Will need to read the patch in more detail to
> see why we have both.

Hm because we also had xpc::GetJSRuntime and mozilla::dom::GetJSRuntime, if I remember correctly. I'd be happy to get rid of the xpc:: variant though, preferably as a followup :)

Comment 8

[Jan de Mooij [:jandem]]

There's also the issue of xpc::Get* using XPCJSRuntime::Get (main thread only?), and mozilla::dom::* using CycleCollectedJSRuntime::Get (uses TLS, works also on workers?).

We could use the latter everywhere, unless we think the "main thread only" bit is useful information.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8778578 [details] [diff] [review]
Misc changes

I looked at the callers of xpc::danger::GetJSContext, and at first glance all of them are rare enough that the TLS lookup of dom::danger::GetJSContext is just fine.  So let's just make them use dom::danger::GetJSContext.  Followup is OK if you prefer, as long as we do it.

>+++ b/dom/base/ScriptSettings.h
>+js::RootingContext* GetJSRootingContext();

Please name this function "RootingCx".

There's also an existing nsContentUtils::RootingCx; ideally its consumers can be converted to this new thing as well, but that will require some spidermonkey changes to accept a js::RootingContext in more places where rooting is involved.  Please file a followup bug on doing that...

>+++ b/dom/base/nsFrameMessageManager.cpp
>+  js::RootingContext* rcx = CycleCollectedJSRuntime::Get()->RootingContext();

Might as well just call dom::RootingCx() here, as long as we have the convenience function.

>+++ b/dom/base/nsGlobalWindow.cpp
>+    JS::Rooted<JS::Value> ignoredVal(CycleCollectedJSRuntime::Get()->RootingContext());

Definitely RootingCx() here.

>+++ b/dom/base/nsINode.cpp
> CheckForOutdatedParent(nsINode* aParent, nsINode* aNode)
>     AutoJSContext cx;
>+    JS::Rooted<JSObject*> existingObj(cx, existingObjUnrooted);

Last I checked, the AutoJSContext constructor could gc, which is why this code was written the way it was.  Now it has explicit GC analysis suppression in it, so it's not clear to me whether it can in fact gc or not and whether mrgiggles can be trusted for it.

Probably safer to leave it as it was, using RootingCx() to root things, and file a followup to figure out whether the simpler version is ok now.

>+++ b/dom/base/nsJSEnvironment.cpp
>   ScriptErrorEvent(nsPIDOMWindowInner* aWindow,
>-                   JSRuntime* aRuntime,
>+                   JSContext* aContext,

In my opinion this should be a js::RootingContext*, and the same up the callstack through DispatchScriptErrorEvent and its callers.  This will require a PersistentRooted constructor that takes a RootingContext*.  I'm probably OK with a followup bug for this as long as it happens quickly; we don't want to be passing a full-up JSContext through this stuff.

>+++ b/dom/bindings/BindingUtils.h
> WrappedJSToDictionary(nsISupports* aObject, T& aDictionary)
>+  JS::Rooted<JSObject*> obj(CycleCollectedJSRuntime::Get()->RootingContext(),

RootingCx()

>+++ b/js/ipc/JavaScriptShared.h
>+    JSContext* cx_;

Please file a followup bug to remove this member entirely.  It's only used in the constructor of some subclasses to set up callbacks on the JSContext and in the destructor to unset the callbacks.  Both places can easily use danger::GetJSContext(), either the xpc version or the DOM version, imo.  And then the JSContext* argument can go away from the nsIContentParent/nsIContentChild alloc functions, the NewJavaScriptParent/Child bits, etc.

>+++ b/js/ipc/WrapperAnswer.h
>+    explicit WrapperAnswer(JSContext* cx) : JavaScriptShared(cx) {}

And this won't need a JSContext either.

>+++ b/js/ipc/WrapperOwner.cpp
>+WrapperOwner::WrapperOwner(JSContext* cx)

Nor this.

>+++ b/js/xpconnect/loader/mozJSComponentLoader.h
>         explicit ModuleEntry(JSContext* aCx)
>+          : mozilla::Module(), obj(aCx), thisObjectKey(aCx)

If PersistentRooted took a RootingContext, we could change this constructor to take that instead of JSContext...

>+++ b/js/xpconnect/src/XPCComponents.cpp
>@@ -2623,19 +2623,17 @@ nsXPCComponents_Utils::ForceShrinkingGC(
>+        JSContext* cx = nsXPConnect::GetRuntimeInstance()->Context();

That's a pretty complicated way of writing danger::GetJSContext(), really.  Let's do the latter instead.  ;)

Or at least XPCJSRuntime::Get()->Context(), if we want to not do the TLS thing.

>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
> JSMainRuntimeTemporaryPeakDistinguishedAmount()
>+    JSContext* cx = nsXPConnect::GetRuntimeInstance()->Context();

Again, danger::GetJSContext or XPCJSRuntime::Get()->Context().

> JSMainRuntimeCompartmentsSystemDistinguishedAmount()
>+    JSContext* cx = nsXPConnect::GetRuntimeInstance()->Context();

And here.

> JSMainRuntimeCompartmentsUserDistinguishedAmount()
>+    JSContext* cx = nsXPConnect::GetRuntimeInstance()->Context();

And here.

>+++ b/js/xpconnect/src/XPCWrappedNativeScope.cpp
>@@ -469,21 +469,21 @@ XPCWrappedNativeScope::~XPCWrappedNative
>+    mContentXBLScope.finalize(cx);
>+        mAddonScopes[i].finalize(cx);
>+    mGlobalJSObject.finalize(cx);

I guess ObjectPtr can't easily use a RootingCx because it needs to touch members other than the rooting stuff, right?

>+++ b/js/xpconnect/src/xpcpublic.h
>+DispatchScriptErrorEvent(nsPIDOMWindowInner* win, JSContext* cx, xpc::ErrorReport* xpcReport,

As noted above, ideally this would use a RootingContext.

>+namespace danger {
>+GetJSContext();

And this would not exist.

>+++ b/toolkit/components/perfmonitoring/nsPerformanceStats.cpp
>+  PendingAlertsCollector(JSContext* cx, nsPerformanceStatsService* service)

The cx argument here is totally unused, right?  Please just kill it off.

>+++ b/xpcom/base/CycleCollectedJSRuntime.h
>+  js::RootingContext* RootingContext() const

It might be worth naming this one RootingCx() too.  But either way; I don't expect people to be calling this one directly much.

>+    return js::ContextFriendFields::get(mJSContext);

Hmm.  It would be awfully nice if a JSContext* just cast to a RootingContext*....  I guess that's hard to do given the opacity of the JSContext type.  :(

r=me with the above nits.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Updated patch

Thanks Boris. This addresses most of your comments, will post follow-up patches for the rest.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Some RootingContext cleanup

In particular, this moves RootingContext from the js:: namespace to JS::.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Pass RootingContext to ScriptErrorEvent, DispatchScriptErrorEvent

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Pass RootingContext to ModuleEntry

The RootingContext::get(cx) calls are a bit annoying here...

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Replace most nsContentUtils::RootingCx() uses

This replaces *most* calls to nsContentUtils::RootingCx() with dom::RootingCx().

Note that I had to use a |const CX& cx| template argument, instead of something like |CX* cx|, so that AutoJSContext can be used.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove nsContentUtils::RootingCx

This removes nsContentUtils::RootingCx(). I changed the 4 remaining callers to use dom::danger::GetJSContext() directly.

* For nsAutoJSString::init, using JS::RootingContext is not an option, because that code uses the cx for a lot of other things (it calls JS::ToString etc).

* The 3 calls in BindingUtils.cpp use APIs like AddRawValueRoot. Maybe we could use JS::RootingContext there with a downcast to JSContext, but that requires new API overloads and I'm not sure it's worth it for these calls.

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 8779699 [details] [diff] [review]
Part 2 - Some RootingContext cleanup

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

Makes sense. We should probably just put the fields from RootLists directly in the RootingContext.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779699 [details] [diff] [review]
Part 2 - Some RootingContext cleanup

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779706 [details] [diff] [review]
Part 3 - Pass RootingContext to ScriptErrorEvent, DispatchScriptErrorEvent

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779707 [details] [diff] [review]
Part 4 - Pass RootingContext to ModuleEntry

r=me.

I agree the get() bits are a PITA.  As I said above, I'm not sure how to address that given that JSContext* is opaque.  :(

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779738 [details] [diff] [review]
Part 5 - Replace most nsContentUtils::RootingCx() uses

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779761 [details] [diff] [review]
Part 6 - Remove nsContentUtils::RootingCx

OK, so the nsAutoJSString situation is quite unfortunate.  Can you please add comments about why it's OK to use a full-up JSContext there without putting it into an "OK to run script" state?  Specifically, that's because the two-argument version of init() is very careful to not run script.  For example, it only does JS::ToString for non-object values...

For the Add/RemoveRawValueRoot thing, it really would be quite awesome from my point of view as an API consumer if those could take a RootingContext.  But I can live with this for now.

r=me.  Thank you for doing this!

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Remove JSContext from IPC classes

Removing the JSContext* argument from these IPC classes lets us remove a bunch of constructors:

 13 files changed, 20 insertions(+), 42 deletions(-)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779794 [details] [diff] [review]
Part 7 - Remove JSContext from IPC classes

r=me.  You rock.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f84bf71b399e
part 1 - Stop using JSRuntime outside SpiderMonkey. r=bz,terrence,fitzgen,kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc58a207c0fd
part 2 - Some RootingContext cleanup. r=bz,terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec20954d813
part 3 - Pass RootingContext to ScriptErrorEvent, DispatchScriptErrorEvent. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a57d4b72cf2
part 4 - Pass RootingContext to ModuleEntry. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9727a015015
part 5 - Replace most nsContentUtils::RootingCx calls with dom::RootingCx. r=bz,terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/10eaa43fad01
part 6 - Remove nsContentUtils::RootingCx. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9adefc25daa8
part 7 - Remove unnecessary JSContext arguments from IPC classes. r=bz

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f84bf71b399e
https://hg.mozilla.org/mozilla-central/rev/bc58a207c0fd
https://hg.mozilla.org/mozilla-central/rev/5ec20954d813
https://hg.mozilla.org/mozilla-central/rev/2a57d4b72cf2
https://hg.mozilla.org/mozilla-central/rev/a9727a015015
https://hg.mozilla.org/mozilla-central/rev/10eaa43fad01
https://hg.mozilla.org/mozilla-central/rev/9adefc25daa8