Closed Bug 664506 Opened 9 years ago Closed 9 years ago

Shutdown cycle collection only garbage collects once

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox6 --- fixed

People

(Reporter: jruderman, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, regression, testcase)

Attachments

(2 files)

Attached file testcase
Steps:
1. XPCOM_MEM_LEAK_LOG=2 firefox testcase
2. Quit

Result:
   Leak 1 DOMStorageImpl, 1 nsDOMStorageItem, 3 nsStringBuffer.

Probably a very recent regression.  The fuzzer first saw it when testing http://hg.mozilla.org/mozilla-central/rev/0a409e965d39 but I'm more suspicious of http://hg.mozilla.org/mozilla-central/rev/174a1d29c93e.
I'll look at it, but Bug 663532 should be just hoisting code that is never called in practice (despite the awful looking diff).
Unapplying the patch for Bug 663532 makes the leak go away.  Sigh.  Thanks for finding this!
Blocks: 663532
Assignee: nobody → continuation
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.
Summary: Leak with localStorage → Shutdown cycle collection only garbage collects once
OS: Mac OS X → All
Hardware: x86_64 → All
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 on attachment 539684 [details] [diff] [review]
Do a GC for every shutdown CC

This patch passed try.  http://tbpl.mozilla.org/?tree=Try&rev=3e5735fc85a5
Attachment #539684 - Flags: review?(bent.mozilla)
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!
Attachment #539684 - Flags: review?(bent.mozilla) → review+
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).
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.
Yeah, that's fine. I'm 99% sure it's totally safe to move it, but we can worry about that later.
Great, thanks.
Keywords: checkin-needed
Attachment #539684 - Flags: checkin?
Blocks: 665044
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Would it be useful to land the testcase Jesse attached as a crashtest as well?
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.
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!
(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.
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 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.
Attachment #539684 - Flags: approval-mozilla-beta?
Attachment #539684 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Target Milestone: --- → mozilla7
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?

Thanks!
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.
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.
Thanks.
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.