Last Comment Bug 664506 - Shutdown cycle collection only garbage collects once
: Shutdown cycle collection only garbage collects once
: mlk, regression, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla7
Assigned To: Andrew McCreight [:mccr8]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 326633 strongparent 663532 665044
  Show dependency treegraph
Reported: 2011-06-15 11:04 PDT by Jesse Ruderman
Modified: 2011-08-23 01:59 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (126 bytes, text/html)
2011-06-15 11:04 PDT, Jesse Ruderman
no flags Details
Do a GC for every shutdown CC (6.20 KB, patch)
2011-06-15 16:34 PDT, Andrew McCreight [:mccr8]
bent.mozilla: review+
asa: approval‑mozilla‑beta+
bugs: checkin+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-06-15 11:04:28 PDT
Created attachment 539594 [details]

1. XPCOM_MEM_LEAK_LOG=2 firefox testcase
2. Quit

   Leak 1 DOMStorageImpl, 1 nsDOMStorageItem, 3 nsStringBuffer.

Probably a very recent regression.  The fuzzer first saw it when testing but I'm more suspicious of
Comment 1 Andrew McCreight [:mccr8] 2011-06-15 11:11:28 PDT
I'll look at it, but Bug 663532 should be just hoisting code that is never called in practice (despite the awful looking diff).
Comment 2 Andrew McCreight [:mccr8] 2011-06-15 14:26:07 PDT
Unapplying the patch for Bug 663532 makes the leak go away.  Sigh.  Thanks for finding this!
Comment 3 Andrew McCreight [:mccr8] 2011-06-15 14:43:19 PDT
My patch changed the behavior of synchronous cycle collections (nsCycleCollector::Collect).  It used to run a GC every time it looped, but now it only runs it once.  Cycles involving both JS and DOM can require multiple runs of both the GC and the CC to fix.  I'd guess that the synchronous cycle collector is only run at shut down, so it probably doesn't have any effect on the browser while it is running, it mostly just wrecks the ability to automatically find real shutdown leaks.  Should be easy enough to fix.
Comment 4 Andrew McCreight [:mccr8] 2011-06-15 16:34:09 PDT
Created attachment 539684 [details] [diff] [review]
Do a GC for every shutdown CC

This patch pulls the code to invoke the JS GC from the CC back out of PrepareForCollection, and puts it in a new method GCIfNeeded (name suggestions welcome).  This function is called right before we signal the CC thread, and inside the shutdown loop (with the force shutdown flag as before).  I carefully looked at the sequence that things are called in, so it should perform the same as before Bug 663532, except the JS GC is pulled into the main thread.

With this patch, the example in this bug does not leak DOMStorageImpl,  nsDOMStorageItem, or nsStringBuffer.  I still have a few random other things (Mutex, ReentrantMonitor, nsThread, nsTArray_base, nsTimerImpl).  I also confirmed that the test in Bug 625302 still works with this patch.
Comment 5 Andrew McCreight [:mccr8] 2011-06-16 07:16:49 PDT
Comment on attachment 539684 [details] [diff] [review]
Do a GC for every shutdown CC

This patch passed try.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-16 11:35:31 PDT
Comment on attachment 539684 [details] [diff] [review]
Do a GC for every shutdown CC

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

r=me with one change.

::: xpcom/base/nsCycleCollector.cpp
@@ +3410,5 @@
>          NS_ASSERTION(!mListener, "Should have cleared this already!");
>          mListener = aListener;
> +        mCollector->GCIfNeeded(PR_FALSE);

You know... I'd feel an awful lot better if we moved this before the Mutex. Calling JS_GC under this lock has always scared me, and now we can separate it out!
Comment 7 Andrew McCreight [:mccr8] 2011-06-16 13:56:24 PDT
That seems like it may be doable, but I don't really understand what the lock is used for.  GCIfNeeded accesses a few fields of the cycle collector, and a field of an instance of nsXPConnect, so it doesn't seem like it would be a great idea to just move the call to mCollector->GCIfNeeded(PR_FALSE) above MutexAutoLock autoLock(mLock).
Comment 8 Andrew McCreight [:mccr8] 2011-06-17 10:03:21 PDT
bent, do you mind if I spin out pulling the GC out of the CC mutex off into a separate bug and land this as is?  It does sound like a good idea, but with mNeedGCBeforeCC and so forth being set in various places, I'm not sure we can just move the call to GCIfNeeded outside of the mutex.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-17 10:07:29 PDT
Yeah, that's fine. I'm 99% sure it's totally safe to move it, but we can worry about that later.
Comment 10 Andrew McCreight [:mccr8] 2011-06-17 10:19:57 PDT
Great, thanks.
Comment 11 Olli Pettay [:smaug] 2011-06-20 08:19:58 PDT
Comment on attachment 539684 [details] [diff] [review]
Do a GC for every shutdown CC
Comment 12 :Ehsan Akhgari 2011-06-20 08:33:44 PDT
Would it be useful to land the testcase Jesse attached as a crashtest as well?
Comment 13 Andrew McCreight [:mccr8] 2011-06-20 08:39:14 PDT
With a crashtest, does it shut down immediately after the test?  I'm not  familiar with the inner workings of the test suites.  The problem was that the CC shutdown code was broken, so if it just sits in the middle of a bunch of other tests, it will probably be okay, because regular CCs and GCs run during the test suite will clean it up.
Comment 14 Andrew McCreight [:mccr8] 2011-06-20 08:41:28 PDT
Similarly, Bug 666353 fixed a problem that only shows up when you run a particular test on startup.  Is there some way to do tests like that, that must be run either on startup or shutdown?

Thanks for landing this, Olli!
Comment 15 :Ehsan Akhgari 2011-06-20 11:35:14 PDT
(In reply to comment #14)
> Similarly, Bug 666353 fixed a problem that only shows up when you run a
> particular test on startup.  Is there some way to do tests like that, that must
> be run either on startup or shutdown?

Unfortunately not...  The only way I know of to do this kind of testing is to write an xpcshell test, but that would probably not be useful here, since we don't open any windows in those tests.
Comment 16 Andrew McCreight [:mccr8] 2011-06-20 11:51:03 PDT
I could look into writing some debug-only code that checks the number of times the JS GC has run before shutdown, and then confirm that it has incremented by the correct number of times afterwards.  I'm not entirely sure how that works, though.

A similar sort of thing could probably work for Bug 663532 (not 666353 as I said above).  For that bug, I added an assert that checks for the particular problem, but it is not a general fix.

I guess it could be the case that it is okay some times for the JS GC to not run when we tell it to, but I can't think of any off hand.
Comment 17 Bill McCloskey (:billm) 2011-07-11 14:44:44 PDT
Comment on attachment 539684 [details] [diff] [review]
Do a GC for every shutdown CC

Requesting approval as this is needed for bug 660778, which fixes a topcrash.
Comment 18 Andrew McCreight [:mccr8] 2011-07-13 07:50:33 PDT
Comment 19 Mihaela Velimiroviciu (:mihaelav) 2011-08-19 05:57:22 PDT
Can anyone please provide a test case or guidelines that I can use to verify this fix? Do I need to create any specific environment to test the fix?

Comment 20 Andrew McCreight [:mccr8] 2011-08-19 06:12:51 PDT
Comment 0 has a test case.   Start Firefox with leak logging, load the test case, then quit right away.  There shouldn't be any leaks reported when Firefox exits.
Comment 21 Trif Andrei-Alin[:AlinT] 2011-08-23 01:59:37 PDT
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0

After following the instructions in comment 20, i loaded the testcase and quit right away.There were no leaks reported when Firefox close.
Setting resolution to Verified Fixed.

Note You need to log in before you can comment on or make changes to this bug.