Stop using JSRuntime outside SpiderMonkey

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(11 attachments, 1 obsolete attachment)

39.82 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.87 KB, patch
kanru
: review+
Details | Diff | Splinter Review
19.93 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
96.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
164.59 KB, patch
jandem
: review+
Details | Diff | Splinter Review
6.37 KB, patch
terrence
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
3.90 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.65 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
22.28 KB, patch
bzbarsky
: review+
terrence
: review+
Details | Diff | Splinter Review
3.50 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.17 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8778574 [details] [diff] [review]
Full patch

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

Comment 2

2 years ago
Created attachment 8778575 [details] [diff] [review]
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.
Attachment #8778575 - Flags: review?(terrence)
(Assignee)

Comment 3

2 years ago
Created attachment 8778576 [details] [diff] [review]
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.
Attachment #8778576 - Flags: review?(kchen)
(Assignee)

Comment 4

2 years ago
Created attachment 8778577 [details] [diff] [review]
Profiler, devtools
Attachment #8778577 - Flags: review?(nfitzgerald)
(Assignee)

Comment 5

2 years ago
Created attachment 8778578 [details] [diff] [review]
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.
Attachment #8778578 - Flags: review?(bzbarsky)
Attachment #8778576 - Flags: review?(kchen) → review+
Attachment #8778577 - Flags: review?(nfitzgerald) → review+
Attachment #8778575 - Flags: review?(terrence) → review+
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.
(Assignee)

Comment 7

2 years ago
(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 :)
(Assignee)

Comment 8

2 years ago
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 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.
Attachment #8778578 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8779695 [details] [diff] [review]
Part 1 - Updated patch

Thanks Boris. This addresses most of your comments, will post follow-up patches for the rest.
Attachment #8779695 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8778574 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8779699 [details] [diff] [review]
Part 2 - Some RootingContext cleanup

In particular, this moves RootingContext from the js:: namespace to JS::.
Attachment #8779699 - Flags: review?(terrence)
Attachment #8779699 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

2 years ago
Created attachment 8779706 [details] [diff] [review]
Part 3 - Pass RootingContext to ScriptErrorEvent, DispatchScriptErrorEvent
Attachment #8779706 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

2 years ago
Created attachment 8779707 [details] [diff] [review]
Part 4 - Pass RootingContext to ModuleEntry

The RootingContext::get(cx) calls are a bit annoying here...
Attachment #8779707 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

2 years ago
Created attachment 8779738 [details] [diff] [review]
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.
Attachment #8779738 - Flags: review?(terrence)
Attachment #8779738 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

2 years ago
Created attachment 8779761 [details] [diff] [review]
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.
Attachment #8779761 - Flags: review?(bzbarsky)
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.
Attachment #8779699 - Flags: review?(terrence) → review+
Attachment #8779738 - Flags: review?(terrence) → review+
Comment on attachment 8779699 [details] [diff] [review]
Part 2 - Some RootingContext cleanup

r=me
Attachment #8779699 - Flags: review?(bzbarsky) → review+
Comment on attachment 8779706 [details] [diff] [review]
Part 3 - Pass RootingContext to ScriptErrorEvent, DispatchScriptErrorEvent

r=me
Attachment #8779706 - Flags: review?(bzbarsky) → review+
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.  :(
Attachment #8779707 - Flags: review?(bzbarsky) → review+
Comment on attachment 8779738 [details] [diff] [review]
Part 5 - Replace most nsContentUtils::RootingCx() uses

r=me
Attachment #8779738 - Flags: review?(bzbarsky) → review+
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!
Attachment #8779761 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

2 years ago
Created attachment 8779794 [details] [diff] [review]
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(-)
Attachment #8779794 - Flags: review?(bzbarsky)
Comment on attachment 8779794 [details] [diff] [review]
Part 7 - Remove JSContext from IPC classes

r=me.  You rock.
Attachment #8779794 - Flags: review?(bzbarsky) → review+

Comment 24

2 years ago
Pushed by jandemooij@gmail.com:
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

Updated

2 years ago
See Also: → bug 1336478

Updated

2 years ago
See Also: bug 1336478
You need to log in before you can comment on or make changes to this bug.