Closed Bug 336922 Opened 19 years ago Closed 19 years ago

nsAnnotationService leaks

Categories

(Firefox :: Bookmarks & History, defect)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8.1, memory-leak)

Attachments

(2 files, 1 obsolete file)

BUILD: Current trunk build STEPS TO REPRODUCE: 1) Start Firefox with leak logging 2) Open preferences 3) Cycle through all the pref panels twice (just click on all the icons in left to right order twice). 4) Shut down firefox. ACTUAL RESULTS: nsAnnottationService leaks, which leaks mozStorageStatement objects, which entrain mozStorageConnection objects, which entrain the mozStorageService. All this stuff also entrains lots of other objects of various sorts (strings, arrays, etc). I was actually trying to debug string leaks when I got here... EXPECTED RESULTS: No leak.
Most likely this leaks due to the following: Created wrapped native [xpconnect wrapped nsIAnnotationService @ 0x9315170 (native @ 0xb2204760)], flat JSObject is 0x930a738 0930a738 object 0x9315170 XPCWrappedNative_NoHelper via nsXPCWrappedJS::mJSObj[nsITimerCallback,0x82dfb60,0x82cbec0](Object @ 0x082cbec0)._svc(Object @ 0x08242500).__ans(XPCWrappedNative_NoHelper @ 0x0930a738). So you have a timer that's holding a reference to some callback object that has a "_svc" property that's an object whose "__ans" property is the annotation service.
OK, I think the problem is in microsummaries. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js.in&rev=1.1&mark=215,217,220,228#213 The highlighted lines are relevant in the following way: Line 215 gives |this| a reference to the timer. Line 217 gives the callback object a reference to |this| Line 220 gives the timer a reference to the callback object. Line 228 doesn't break the resulting cycle. I suggest canceling the timer and setting this._timer to null on line 228...
Flags: blocking1.9a1?
Flags: blocking-firefox2?
Note, however that I tried making that change and I'm still leaking. So there might be other ways this stuff leaks... unfortunately, my build that can produce GC traces is stuck in a debugger right now until Brendan looks at that crash, so there's not much more I can say till then. Whoever knows this code should look over timer use carefully, though (esp. what happens if we shut down while the timer is pending).
Attachment #221118 - Flags: superreview?(bugs)
Attachment #221118 - Flags: review?(bzbarsky)
Attachment #221118 - Flags: review?(bzbarsky) → review+
myk, could you just go ahead and check this in to trunk so we can check whether this fixes the balsa leaks?
(In reply to comment #5) > myk, could you just go ahead and check this in to trunk so we can check whether > this fixes the balsa leaks? Yup, I've just checked it in to the trunk.
Comment on attachment 221118 [details] [diff] [review] patch v1: fix the microsummary service part of the problem sr+a=ben@mozilla.org
Attachment #221118 - Flags: superreview?(bugs)
Attachment #221118 - Flags: superreview+
Attachment #221118 - Flags: approval-branch-1.8.1+
Attached patch Patch to make the first patch work (obsolete) (deleted) — Splinter Review
Attachment #221267 - Flags: superreview?
Attachment #221267 - Flags: review?
Attachment #221267 - Flags: approval-branch-1.8.1?(bugs)
Attached patch Address comments (deleted) — Splinter Review
Attachment #221267 - Attachment is obsolete: true
Attachment #221268 - Flags: superreview?(darin)
Attachment #221268 - Flags: review?(mconnor)
Attachment #221268 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #221267 - Flags: superreview?
Attachment #221267 - Flags: review?
Attachment #221267 - Flags: approval-branch-1.8.1?(bugs)
Comment on attachment 221268 [details] [diff] [review] Address comments now, i wonder why we need to removeObserver for profile-after-change. why not just do the same there? sr=darin w/ that
Attachment #221268 - Flags: superreview?(darin) → superreview+
Attachment #221268 - Flags: review?(mconnor)
Attachment #221268 - Flags: review+
Attachment #221268 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #221268 - Flags: approval-branch-1.8.1+
Comment on attachment 221268 [details] [diff] [review] Address comments Checked this in, trunk and 1.8 branch. Leaks are gone locally; we'll see about tinderbox.
OK, this fixed most of the tinderbox leaks. Marking fixed; I'll file more bugs on the rest, I guess.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Checked the first patch in on 1.8 branch too.
Keywords: fixed1.8.1
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking-firefox2?
Flags: blocking-firefox2+
The annotation service is not in Firefox 2.
Flags: blocking-firefox2+
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Assignee: nobody → bzbarsky
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: