SuspectAllWrappers should suspect all wrappers

RESOLVED FIXED in Firefox 42



3 years ago
3 years ago


(Reporter: mccr8, Assigned: mccr8)


Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)



(3 attachments, 1 obsolete attachment)



3 years ago
I had a bunch of theories for what was going wrong in bug 1179782, and I wrote some pretty huge patches, but I think the actual fix is pretty simple. Right now, XPCWrappedNativeScope::SuspectAllWrappers() does not add wrappers to the CC graph if the refcount is > 1, which is when the XPCWN owns the JS object. But we kind of want to add the JS object if it owns the XPCWN, so I think we should just add it all the time.

I think I've tried something like this in previous investigations into XPCWN traversal (bug 927601), but hopefully just this one thing won't break too much.

Comment 1

3 years ago
Created attachment 8634504 [details] [diff] [review]
SuspectAllWrappers should suspect more wrappers.

Simple patch.

Comment 2

3 years ago
This is akin to the case with chains of promises that took a while to tear down, where Olli added some code to suspect JS objects. I still need to investigate a little more why we're not adding this wrapper to the graph normally.


3 years ago
Blocks: 1175259

Comment 3

3 years ago
In more detail, my theory is that the reference count on an XPCWN going to 1 can end up breaking a cycle, because it causes the XPCWN to drop the reference to the JS wrapper, but we do not suspect the JS object. Therefore, this cycle is invisible to the cycle collector. If the JS wrapper is indeed unreachable, then when we run the GC next we will collect the wrapper, so the garbage will go away eventually, but it can't happen in a single cycle. This test case involves a long chain of XPCWNs and XPCWJS so things don't get torn down in time.

We have an existing mechanism to suspect JS objects held by C++ objects: we trace any C++ object that dies and suspect JS it was holding onto. However, this does not work for XPCWNs because they are not technically SCRIPT_HOLDER objects, so they have no trace hook. Even if they did, because their refcount is zero, they wouldn't trace their JS wrapper.

Technically, this should be an issue for any JS held by C++: if a C++ object holds onto a JS object, then does a store so that it holds onto a different object, then any garbage cycles involving the original JS object will need to wait for a GC to happen first. I think this is not an issue in practice because typically (at least in the case of a preserved wrapper) once a C++ object holds onto a JS object, it does so until it dies (when the mechanism in the previous paragraph comes into play), but we should keep it in mind for future leaks.

With my patch, we always trace the JS wrapper for an XPCWN, even if the XPCWN does not own the JS wrapper. This covers the case where the XPCWN refcount drops to 1. It does not cover the case where the refcount of the XPCWN drops from 2 to 0 in between cycle collections. For that, we could add a new callback in the CC for suspecting JS objects, and call that in the XPCWN dtor. Maybe I'll do that in a followup.

I felt uncomfortable with how the patch reaches across a weak reference, but the finalizer for the JS wrapper clears the pointer in the XPCWN when it dies, so as long as we're not running the CC during a GC it should be okay. If we're not, then there are plenty of other things that could go wrong.

Comment 4

3 years ago
Created attachment 8634820 [details] [diff] [review]
part 1 - Make XPCJSRuntime::SuspectWrappedNative into a method on XPCWrappedNative.

I filed bug 1184638 about suspecting the wrapper of dying XPCWNs, but this bug alone fixes the shutdown leak and seems fairly simple, so I think it can go ahead by itself.

In this part, I make SuspectWrappedNative into an XPCWN method. This has bothered me for a while.
Attachment #8634820 - Flags: review?(bobbyholley)

Comment 5

3 years ago
Created attachment 8634821 [details] [diff] [review]
part 2 - XPCWrappedNative::Suspect() should suspect the wrapper even if it doesn't own it.

This simply makes the Suspect() unconditional. See comment 3 for more explanation.

try run:
Attachment #8634821 - Flags: review?(bobbyholley)


3 years ago
Summary: SuspectAllWrappers should suspect more wrappers → SuspectAllWrappers should suspect all wrappers

Comment 6

3 years ago
Created attachment 8634834 [details] [diff] [review]
part 3 - Use HasExternalReference in XPCWrappedNative::Traverse.

This is another thing I've been meaning to do. Rather than directly poking at the refcount, use the predicate function we have. This should not change the behavior.
Attachment #8634834 - Flags: review?(bobbyholley)


3 years ago
Attachment #8634504 - Attachment is obsolete: true
Attachment #8634820 - Flags: review?(bobbyholley) → review+
Comment on attachment 8634821 [details] [diff] [review]
part 2 - XPCWrappedNative::Suspect() should suspect the wrapper even if it doesn't own it.

Review of attachment 8634821 [details] [diff] [review]:

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +105,5 @@
>      // Only record objects that might be part of a cycle as roots, unless
> +    // the callback wants all traces (a debug feature). Do this even if
> +    // the XPCWN doesn't own the wrapper in case the wrapper keeps alive
> +    // other C++ things. This is safe because if the wrapper had died the

Please use the term "JS reflector object" (first time) and "reflector" (subsequent times) instead of "wrapper" here.
Attachment #8634821 - Flags: review?(bobbyholley) → review+
Attachment #8634834 - Flags: review?(bobbyholley) → review+

Comment 9

3 years ago
I saw a reference to this bug on Twitter and wanted to make a drive-by that comment #3 really belongs in a commit message. MozReview and bzexport will both copy the commit message to the attachment details, so no context is lost. If your tools are not propagating commit messages to Bugzilla when reviews are submitted, that's a bug and should be reported.

Comment 10

3 years ago
I explained the gist of it in a comment.

Comment 11

3 years ago
I should probably write more about this in the cycle collector file, though.
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.