993413 - Remove use of JS_Add/Remove*Root in favour of JS::PersistentRooted where possible

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Using JS::PersistentRooted to hold GC thing pointers which need to be rooted is simpler and less error prone that calling JS_AddBlahRoot and JS_RemoveBlahRoot.  It also simplifies the guidance on GC wrapper types: everything on the heap should be a Heap<T> or a PersistentRooted<T>.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: remove-add-roots

Requesting review for XPConnect changes.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8404568 [details] [diff] [review]
remove-add-roots

Requesting review for browser changes.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8404568 [details] [diff] [review]
remove-add-roots

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

r=me on the XPConnect bits with those comments addressed. But I think Andrew may have something to say about removing the "why is this rooted?" strings.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1017,5 @@
>      if (js::GetObjectJSClass(obj) == &kFakeBackstagePassJSClass) {
>          MOZ_ASSERT(mReuseLoaderGlobal);
>          // tableScript stays in the table until shutdown. To avoid it being
>          // collected and another script getting the same address, we root
>          // tableScript lower down in this function.

This comment needs to be fixed. We should note that we're piggy-backing on the rooting in the ModuleEntry, under the assumption that ModuleEntries are never dynamically unloaded when mReuseLoaderGlobal is true.

::: js/xpconnect/loader/mozJSComponentLoader.h
@@ -121,5 @@
>  
>                  JS_SetAllNonReservedSlotsToUndefined(cx, obj);
> -                JS_RemoveObjectRoot(cx, &obj);
> -                if (thisObjectKey)
> -                    JS_RemoveScriptRoot(cx, &thisObjectKey);

In order to preserve the old behavior, we need to explicitly null these out here, right?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3024,5 @@
>     mVariantRoots(nullptr),
>     mWrappedJSRoots(nullptr),
>     mObjectHolderRoots(nullptr),
>     mWatchdogManager(new WatchdogManager(MOZ_THIS_IN_INITIALIZER_LIST())),
> +   mJunkScope(Runtime(), nullptr),

Doesn't this need to be MOZ_THIS_IN_INITIALIZER_LIST()->Runtime() or somesuch?

@@ -3462,5 @@
>          nsresult rv = CreateSandboxObject(cx, &v, nsContentUtils::GetSystemPrincipal(), options);
>          NS_ENSURE_SUCCESS(rv, nullptr);
>  
>          mJunkScope = js::UncheckedUnwrap(&v.toObject());
> -        JS_AddNamedObjectRoot(cx, &mJunkScope, "XPConnect Junk Compartment");

Hm, don't we want to keep these strings around? Seems like they're probably really helpful for leak debugging.

Comment 4

[Andrew McCreight [:mccr8]]

Yeah, it would be really nice to require that PersistentRooted's ctor takes a string argument that describes the root.  Is there anyway to do that that won't totally wreck perf?  Anyways, that seems like a pretty invasive change that could maybe be a followup.  I guess that would require storing a pointer to a string in every PersistentRooted field which is bad (but maybe we're basically doing that already for these named rooted objects?).  But anyways, if we don't do something like that, we're going to lose a lot of information from GC logs.

Comment 5

[Andrew McCreight [:mccr8]]

Filed bug 994799 for that.  I'll see how bad a prototype is.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8404568 [details] [diff] [review]
remove-add-roots

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

::: content/base/src/nsFrameMessageManager.cpp
@@ +1363,5 @@
>  }
>  
>  static PLDHashOperator
>  CachedScriptUnrooter(const nsAString& aKey,
>                       nsFrameScriptObjectExecutorHolder*& aData,

Maybe rename this function now that it isn't actually unrooting?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8404568 [details] [diff] [review]
remove-add-roots

r=me on the bits outside js/

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug993413-remove-add-roots v2

Thanks for the review comments.

Requesting review from Terrence for the js/src parts.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 8406177 [details] [diff] [review]
bug993413-remove-add-roots v2

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

r=me  Nice! Are there any uses of AddRoot/RemoveRoot still present with this?

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #9)

There are still a few places which are not so simple to convert - ErrorResult::mJSException, and a few in js/jsd for example.

Comment 11

[Boris Zbarsky [:bzbarsky]]

ErrorResult::mJSException is a fun one: what's needed there is some way of rooting things in a rare case which is absolutely 0 overhead (as in, no instructions executed in ctor or dtor) in the common case when nothing needs to be rooted.  So for example Maybe<PersistentRooted> is out, since it adds a branch to the destructor...

Comment 12

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df934e78525e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df934e78525e