Jesse Ruderman Reporter
	Description • 11 years ago

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.

	Andrea Marchesini [:baku]
	Comment 1 • 11 years ago

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

	Andrew McCreight [:mccr8]
	Comment 2 • 11 years ago

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

	Jesse Ruderman Reporter
	Comment 3 • 11 years ago

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.

	Jesse Ruderman Reporter
	Comment 4 • 11 years ago

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

	Jesse Ruderman Reporter
	Comment 5 • 11 years ago

Any luck reproducing with my suggestions?

	Andrea Marchesini [:baku]
	Comment 6 • 11 years ago

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

	Andrea Marchesini [:baku]
	Comment 7 • 11 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 8 • 11 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 11 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 11 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 11 • 11 years ago

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

	Andrea Marchesini [:baku]
	Comment 12 • 11 years ago

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

	Jesse Ruderman Reporter
	Comment 13 • 11 years ago

Future -> Promise

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

	Jesse Ruderman Reporter
	Comment 14 • 11 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 15 • 11 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 16 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

I don't understand. What is the missing edge?
What is the C++->JS edge?

(But if preserving wrappers helps here, fine)

	Andrew McCreight [:mccr8]
	Comment 18 • 11 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 19 • 11 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 20 • 11 years ago

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

	Andrew McCreight [:mccr8]
	Comment 21 • 11 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 22 • 11 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 23 • 11 years ago

Jesse, see comment 20 please.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 24 • 11 years ago

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

	Andrew McCreight [:mccr8]
	Comment 25 • 11 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 26 • 11 years ago

Just the test since it is fixed in another bug.

	Andrew McCreight [:mccr8]
	Comment 27 • 11 years ago

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...

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 28 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 29 • 11 years ago

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

	Andrew McCreight [:mccr8]
	Comment 30 • 11 years ago

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

Correct.