Closed Bug 993413 Opened 6 years ago Closed 6 years ago

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


(Core :: JavaScript: GC, defect)

Not set





(Reporter: jonco, Assigned: jonco)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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>.
Assignee: nobody → jcoppeard
Attached patch remove-add-roots (obsolete) — Splinter Review
Requesting review for XPConnect changes.
Attachment #8404568 - Flags: review?(bobbyholley)
Comment on attachment 8404568 [details] [diff] [review]

Requesting review for browser changes.
Attachment #8404568 - Flags: review?(bzbarsky)
Comment on attachment 8404568 [details] [diff] [review]

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.
Attachment #8404568 - Flags: review?(bobbyholley)
Attachment #8404568 - Flags: review+
Attachment #8404568 - Flags: feedback?(continuation)
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.
Filed bug 994799 for that.  I'll see how bad a prototype is.
Comment on attachment 8404568 [details] [diff] [review]

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?
Attachment #8404568 - Flags: feedback?(continuation) → feedback+
Comment on attachment 8404568 [details] [diff] [review]

r=me on the bits outside js/
Attachment #8404568 - Flags: review?(bzbarsky) → review+
Blocks: 994799
OS: Mac OS X → All
Hardware: x86 → All
Thanks for the review comments.

Requesting review from Terrence for the js/src parts.
Attachment #8406177 - Flags: review?(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?
Attachment #8406177 - Flags: review?(terrence) → review+
(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.
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...
Attachment #8404568 - Attachment is obsolete: true
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.