Bug 883683's mochitest is passing (as part of mochitest-2), but the original testcase still leaks. Running with XPCOM_MEM_LEAK_LOG=2 shows this leaking nsDocument.

I cannot reproduce it. Can it be that GC is not cycled yet?

No, we run the GC and CC repeatedly on shutdown.

With a Tinderbox debug build, I don't get a leak but I do hit ###!!! ASSERTION: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file xpcom/base/nsCycleCollector.cpp, line 2756 With a local debug build, I get a leak.

Does it leak for you if you use an array with 10 elements instead of 5?

Any luck reproducing with my suggestions?

Yes, I can reproduce it. I'm writing a patch :)

I really would like to know removing this https://mxr.mozilla.org/mozilla-central/source/dom/future/FutureResolver.cpp#167 it doesn't leak. In the meantime I optimized the NS_DROP/HOLD_OBJECTS use.

Comment on attachment 770723 [details] [diff] [review] patch Review of attachment 770723 [details] [diff] [review]: ----------------------------------------------------------------- Does optimizing HOLD and DROP like this really matter? Is this really a perf bottleneck? In general, using mRooted flags is dangerous, but I think here it is okay because you are only using mRooted to decide to DROP in Unlink and the destructor, when you are definitely going to clear the wrapper cache. ::: dom/future/Future.cpp @@ +261,5 @@ > > void > +Future::SetResult(JS::Handle<JS::Value> aValue) > +{ > + if (JSVAL_IS_GCTHING(aValue)) { You can use IS_TRACEABLE here, because null is a GCTHING but not TRACEABLE, or something. Also, shouldn't this guard on mRooted, too, or you'll hit the assert in RootResultVal?

So far I haven't managed to reproduce any real leak. I got ###!!! ASSERTION: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS' once and kind of temporary leak if I loaded the testcase immediately after browser startup and closed browser asap (so that we only got shutdown CCs, not any normal CCs). But if browser is up any normal time, I don't see the leak, and based on CC graph a Future is there for a short period.

Comment on attachment 770723 [details] [diff] [review] patch So I don't understand why this helps, and I can't really reproduce any permanent leak.

How relevant is this since Promises has changed a bit since then?

it's not actually a real leak. I and Smaug tested it months ago. Probably we can close this bug.

Future -> Promise Still reproduces (leak in local debug build, extra-CC assertion in tbox debug)

This bug requires me to ignore the extra-CC assertion and all leak reports from fuzz testcases that use DOM Promises. I don't think that's a good situation to be in. Does this bug allow a page to make cleanup require an arbitrary number of CCs? If so, that's as real a leak as any.

nsm and I looked into this. I think the problem is that we have a JS -> C++ -> JS -> C++ chain where the JS object (all of the JS objects are wrappers) is unreachable, so we have a missing edge in the graph and the C++ object stays alive when we run the CC. Then we run the GC again and the JS object dies, which kills the C++ object, but not until after the GC is done, so the second wrapper is still alive, and the pattern repeats. So, essentially, the unreachable JS wrapper prevents the CC from freeing the chain, and the GC intentionally never knows about C++, so it can't see through the whole chain and free things, so we end up having to run the GC once per wrapper. The most direct solution to this problem would be to always add a C++ objects wrapper to the CC graph. I think we do something like this for XPCWNs. That wouldn't help in cases where some other kind of JS thing owns a C++ thing, of course, but maybe that's not a problem. This could happen for anything that owns and can be owned from JS, but it seems like an especially bad problem here where chains are a normal thing to do with promises.

We could fix this up for promises by making them always wrapper preserved.

I don't understand. What is the missing edge? What is the C++->JS edge? (But if preserving wrappers helps here, fine)

The missing edge is from the wrapper of the promise to the promise, because the wrapper is unreachable, so we don't end up traversing it.

The easiest way to fix this might be to always preserve the wrapper for a Promise. Then we'd always trace it when we trace the Promise.

Seems to work, Jesse do you want to try it out?

Comment on attachment 8358050 [details] [diff] [review] Preserve Promise wrapper so CC is aware of it. Review of attachment 8358050 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable, thanks. ::: dom/promise/Promise.cpp @@ +193,5 @@ > NS_IMPL_CYCLE_COLLECTION_UNLINK(mWindow) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mResolveCallbacks); > NS_IMPL_CYCLE_COLLECTION_UNLINK(mRejectCallbacks); > tmp->mResult = JS::UndefinedValue(); > + tmp->ReleaseWrapper(tmp); You shouldn't need that, as that's just what NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER. @@ +234,5 @@ > Promise::~Promise() > { > MaybeReportRejected(); > mResult = JS::UndefinedValue(); > + ReleaseWrapper(this); I think this isn't needed either.

I filed bug 958315 for khuey's idea for a more general purpose solution.

Jesse, see comment 20 please.

On shutdown, don't attempt reporting a rejected promise.

This should be fixed by bug 958315, so we should just need the test.

Just the test since it is fixed in another bug.

Does this test actually fail without bug 958315? I mean, if you run it by itself it would, but I'd think as part of Mochitest as a whole it won't. You need to do something like have some object you can have a weak reference to being kept alive by the end of the chain, then force a couple of GC/CCs, then check to make sure the object is no longer alive. I don't know if that's worth the hassle or not...

Bug 958315 does indeed fix this, but the test isn't useful since it won't show up in automation when running with other tests. I've verified it locally.

We didn't land this wrapper preserving thing, right?

Comment on attachment 8363794 [details] [diff] [review] Preserve Promise wrapper so CC is aware of it. Correct.