The default bug view has changed. See this FAQ.

Shutdown cycle collection only garbage collects once

VERIFIED FIXED in Firefox 6

Status

()

Core
DOM
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {mlk, regression, testcase})

Trunk
mozilla7
mlk, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6 fixed)

Details

Attachments

(2 attachments)

126 bytes, text/html
Details
6.20 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
smaug
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Created attachment 539594 [details]
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.
(Assignee)

Comment 1

6 years ago
I'll look at it, but Bug 663532 should be just hoisting code that is never called in practice (despite the awful looking diff).
(Assignee)

Comment 2

6 years ago
Unapplying the patch for Bug 663532 makes the leak go away.  Sigh.  Thanks for finding this!
(Assignee)

Updated

6 years ago
Blocks: 663532
(Assignee)

Updated

6 years ago
Assignee: nobody → continuation
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Updated

6 years ago
Summary: Leak with localStorage → Shutdown cycle collection only garbage collects once
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 4

6 years ago
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.

Updated

6 years ago
Blocks: 335998
(Assignee)

Comment 5

6 years ago
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+
(Assignee)

Comment 7

6 years ago
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).
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 10

6 years ago
Great, thanks.
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #539684 - Flags: checkin?
(Assignee)

Updated

6 years ago
Blocks: 665044
Comment on attachment 539684 [details] [diff] [review]
Do a GC for every shutdown CC

http://hg.mozilla.org/mozilla-central/rev/a9c243918ad5
Attachment #539684 - Flags: checkin? → checkin+

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Would it be useful to land the testcase Jesse attached as a crashtest as well?
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Comment 16

6 years ago
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?

Updated

6 years ago
Attachment #539684 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/b28490409dfc
(Assignee)

Updated

6 years ago
status-firefox6: --- → fixed
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!
(Assignee)

Comment 20

6 years ago
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
You need to log in before you can comment on or make changes to this bug.