Closed
Bug 664506
Opened 14 years ago
Closed 14 years ago
Shutdown cycle collection only garbage collects once
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla7
Tracking | Status | |
---|---|---|
firefox6 | --- | fixed |
People
(Reporter: jruderman, Assigned: mccr8)
References
Details
(Keywords: memory-leak, regression, testcase)
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bent.mozilla
:
review+
asa
:
approval-mozilla-beta+
smaug
:
checkin+
|
Details | Diff | Splinter Review |
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•14 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•14 years ago
|
||
Unapplying the patch for Bug 663532 makes the leak go away. Sigh. Thanks for finding this!
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 3•14 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•14 years ago
|
Summary: Leak with localStorage → Shutdown cycle collection only garbage collects once
Assignee | ||
Updated•14 years ago
|
OS: Mac OS X → All
Hardware: x86_64 → All
Assignee | ||
Comment 4•14 years ago
|
||
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•14 years ago
|
Blocks: strongparent
Assignee | ||
Comment 5•14 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•14 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•14 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 | ||
Updated•14 years ago
|
Attachment #539684 -
Flags: checkin?
Comment 11•14 years ago
|
||
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•14 years ago
|
Comment 12•14 years ago
|
||
Would it be useful to land the testcase Jesse attached as a crashtest as well?
Assignee | ||
Comment 13•14 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•14 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!
Comment 15•14 years ago
|
||
(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•14 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•13 years ago
|
Attachment #539684 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 18•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
status-firefox6:
--- → fixed
Target Milestone: --- → mozilla7
Comment 19•13 years ago
|
||
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•13 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.
Comment 21•13 years ago
|
||
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
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•