Closed
Bug 336922
Opened 19 years ago
Closed 19 years ago
nsAnnotationService leaks
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: fixed1.8.1, memory-leak)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
bugs
:
superreview+
bugs
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mconnor
:
review+
darin.moz
:
superreview+
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
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.
Assignee | ||
Comment 2•19 years ago
|
||
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...
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.9a1?
Flags: blocking-firefox2?
Assignee | ||
Comment 3•19 years ago
|
||
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).
Comment 4•19 years ago
|
||
Attachment #221118 -
Flags: superreview?(bugs)
Attachment #221118 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•19 years ago
|
Attachment #221118 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•19 years ago
|
Blocks: microsummaries
Assignee | ||
Comment 5•19 years ago
|
||
myk, could you just go ahead and check this in to trunk so we can check whether this fixes the balsa leaks?
Comment 6•19 years ago
|
||
(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 7•19 years ago
|
||
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+
Assignee | ||
Comment 8•19 years ago
|
||
Attachment #221267 -
Flags: superreview?
Attachment #221267 -
Flags: review?
Attachment #221267 -
Flags: approval-branch-1.8.1?(bugs)
Assignee | ||
Comment 9•19 years ago
|
||
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 10•19 years ago
|
||
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+
Updated•19 years ago
|
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+
Assignee | ||
Comment 11•19 years ago
|
||
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.
Assignee | ||
Comment 12•19 years ago
|
||
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
Assignee | ||
Comment 13•19 years ago
|
||
Checked the first patch in on 1.8 branch too.
Keywords: fixed1.8.1
Updated•18 years ago
|
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking-firefox2?
Flags: blocking-firefox2+
Comment 15•15 years ago
|
||
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
Updated•13 years ago
|
Assignee: nobody → bzbarsky
You need to log in
before you can comment on or make changes to this bug.
Description
•