Closed Bug 48126 Opened 24 years ago Closed 24 years ago

60k of leaks added to smoketest

Categories

(SeaMonkey :: General, defect, P3)

Product:
SeaMonkey
Component:
General
Platform:
Sun
Solaris
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: mcafee, Assigned: saari)

References

(
URL
)

Details

(Keywords: memory-leak, smoketest, Whiteboard: [dogfood+][MLK])

Attachments

(2 files)

balance tree for leaked nsDocument (#11), --ignore-balance
60.42 KB, text/plain
Details
weak ref to doc, only fixes part of problem
2.09 KB, patch
Details | Diff | Splinter Review
60k of leaks added to smoketest: Judson Valeski wrote: > > bad (60k) new leakage by someone on this list: > saari > mkaply > pavlov > bratell > harishd > nhotts > > Please look over the new leaks to see if they are you. > > TOTAL 66108 895.60% 23826724 -0.00% > --NEW-LEAKS-------------------------------- > nsExtensibleStringBundle 32 - > nsXULElement::Slots 1276 - > nsPersistentProperties 48 - > nsFileTransportService 28 - > nsStringBundle 32 - > nsXULAttributes 2320 - > nsEventListenerManager 320 - > NameSpaceURIKey 72 - > nsHTMLTableElement 1440 - > nsCommentNode 1500 - > HTMLAttribute 2128 - > nsNodeInfoManager 40 - > nsDOMAttributeMap 48 - > nsDNSService 72 - > nsClassList 1256 - > ArenaImpl 48 - > nsHTMLFontElement 312 - > nsHTMLTableCellElement 5580 - > nsIOService 28 - > nsHTMLMappedAttributes 1680 - > nsHTMLParagraphElement 312 - > nsImageMap 40 - > nsHTMLSpanElement 1144 - > nsHTMLAnchorElement 3456 - > nsXBLAttributeEntry 432 - > nsDocument 224 - > nsBindingManager 28 - > nsHTMLTableRowElement 3248 - > nsSliderMediator 40 - > nsHTMLHtmlElement 52 - > nsHTMLImageElement 60 - > nsContentList 64 - > NameSpaceImpl 140 - > HTMLCSSStyleSheetImpl 32 - > nsHTMLTableSectionElement 1344 - > nsDateTimeFormatUnix 156 - > nsXULContentUtils 12 - > HTMLStyleSheetImpl 60 - > nsHTMLBRElement 576 - > nsHTMLHeadElement 52 - > nsTextNode 11100 - > nsXBLBinding 176 - > nsAuthURLParser 12 - > nsHTMLMapElement 56 - > nsHTMLAreaElement 60 - > CSSLoaderImpl 140 - > HTMLAttributesImpl 9420 - > AttributeKey 360 - > nsCodebasePrincipal 60 - > nsNodeInfo 1120 - > Area 28 - > nsCharsetConverterManager 24 - > nsHTMLTitleElement 52 - > HTMLColorRule 80 - > nsJISx4501LineBreaker 12 - > nsXULAttribute 1568 - > nsDOMDocumentType 160 - > nsHTMLBodyElement 64 - > GenericTableRule 32 - > nsAggregatePrincipal 72 - > NameSpaceManagerImpl 72 - > nsXMLElement 512 - > nsHTMLDivElement 208 - > nsXULElement 2552 - > AtomImpl 3808 1600.00% > nsVoidArray 1416 742.86% > nsHashtable 132 450.00% > nsHashKey 232 383.33% > nsThread 96 300.00% > nsThreadPoolRunnable 48 200.00% > nsThreadPool 96 100.00% > nsStr 1120 24.44% > TOTAL 64620
I vote for blocker, we need to fix this asap. Convince me otherwise.. and we can open the tree.
Severity: normal → blocker
Keywords: smoketest
Whiteboard: [MLK]
Hmm.. I wonder how this could be me! Any idea if anything in the smoketest would trigger strict DTD ( because my changes are only related to the strict DTD )? Okay, first thing in the morning I'll back out my changes to see if the leaks go away.
Status: NEW → ASSIGNED
Here are a few big numbers: Per-Inst Leaked HTMLAttribute 16 2128 HTMLAttributesImpl 60 9420 nsHTMLAnchorElement 64 3456 nsHTMLTableCellElement 60 5580 nsHTMLTableElement 60 1440 nsHTMLTableRowElement 56 3248 nsHTMLTableSectionElement 56 1344 nsNodeInfo 28 1120 nsTextNode 60 11100 nsXULAttribute 32 1568 nsXULAttributes 80 2320 nsXULElement 88 2552 nsXULElement::Slots 44 1276
In the non --ignore-balance version, I can see that nsImageMap's destructor is never called, so that's the leaked ref. Could this be a circularity introduced, or is there just a leak somewhere?
Putting on [dogfood+] radar.
Keywords: mlk
Whiteboard: [MLK] → [dogfood+][MLK]
I'm holding the tree for this.
Looking at tinderbox, harishd backed out his changes and the leaks are still there. Who's next?
My changes are for Macintosh only (mozilla/intl/locale/mac), they should not affect Unix leak test.
reassigning to saari
Assignee: harishd → saari
Status: ASSIGNED → NEW
FYI, you don't need to check in to have tinderbox run the bloat tests for you. All you need to do is: On Linux: setenv XPCOM_MEM_LEAK_LOG 1 [ or leaks.txt for file ] ./mozilla -f bloaturls.txt On Win: set XPCOM_MEM_LEAK_LOG=1 [or leaks.txt for file] mozilla -f bloaturls.txt On Mac: Create a file called environment in the viewer_debug directory, w/ contents: XPCOM_MEM_LEAK_LOG=leaks.txt Copy one of the files that starts mozilla with args in that same directory, and edit it to change the args to "-f bloaturls.txt" Run that file that starts mozilla. And then you will see the leaks either on stdout (1) or leaks.txt.
I backed out in my tree the changes to the following files: nsHTMLImageElement.cpp nsHTMLAreaElement.cpp nsImageFrame.cpp nsImageMap.cpp nsImageMap.h and I no longer see the leaks (someone should verify this because there's a chance seeing the leaks may be intermittent). Figuring out which part of the changes leaks will probably require looking at the code...
Thank you dbaron!
I'm next going to try it with only backing out the focus listener stuff in nsImageMap.cpp...
it looks like dbaron's all over this and we have some good traction, so I'm not holding the tree for this one. lowering to critical to drop off radar.
Severity: blocker → critical
I think this fixes it (I want to double check that I still see the leaks without this change). Saari, what does this change do? There might be another way around this with some messy refcounting hack in the |Release| method. Index: nsImageMap.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/html/base/src/nsImageMap.cpp,v retrieving revision 3.46 diff -U8 -r3.46 nsImageMap.cpp --- nsImageMap.cpp 2000/08/08 21:30:47 3.46 +++ nsImageMap.cpp 2000/08/09 19:12:26 @@ -804,29 +804,31 @@ mMap = nsnull; mDomMap = nsnull; mDocument = nsnull; mContainsBlockContents = PR_FALSE; } nsImageMap::~nsImageMap() { +#if 0 //Remove all our focus listeners PRInt32 i, n = mAreas.Count(); for (i = 0; i < n; i++) { Area* area = (Area*) mAreas.ElementAt(i); nsCOMPtr<nsIContent> areaContent; area->GetArea(getter_AddRefs(areaContent)); if (areaContent) { nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(areaContent)); if (rec) { rec->RemoveEventListenerByIID(this, NS_GET_IID(nsIDOMFocusListener)); } } } +#endif FreeAreas(); if (nsnull != mDocument) { mDocument->RemoveObserver(NS_STATIC_CAST(nsIDocumentObserver*, this)); NS_RELEASE(mDocument); } NS_IF_RELEASE(mDomMap); NS_IF_RELEASE(mMap); @@ -978,21 +980,23 @@ nsImageMap::AddArea(nsIContent* aArea) { nsAutoString shape, coords, baseURL, noHref; aArea->GetAttribute(kNameSpaceID_None, nsHTMLAtoms::shape, shape); aArea->GetAttribute(kNameSpaceID_None, nsHTMLAtoms::coords, coords); PRBool hasURL = (PRBool)(NS_CONTENT_ATTR_HAS_VALUE != aArea->GetAttribute(kNameSpaceID_None, nsHTMLAtoms::nohref, noHref)); PRBool suppress = PR_FALSE;/* XXX */ +#if 0 //Add focus listener to track area focus changes nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(aArea)); if (rec) { rec->AddEventListenerByIID(this, NS_GET_IID(nsIDOMFocusListener)); } +#endif Area* area; if ((0 == shape.Length()) || shape.EqualsIgnoreCase("rect") || shape.EqualsIgnoreCase("rectangle")) { area = new RectArea(aArea, suppress, hasURL); } else if (shape.EqualsIgnoreCase("poly") ||
That will break image map tabbing. There may be another, cleaner way as suggested by Hyatt... release the listeners on SetDocument. XUL need to do this sort of stuff in a few places.
The cycle is that the: nsImageMap owns nsDocument nsEventListenerManager owns nsImageMap (this is the new piece) nsDocument owns nsEventListenerManager Why does the nsImageMap need to own the nsDocument? Could it instead own the corresponding content element and get the document from that when needed?
Yeah you're right, ImageMap shouldn't be owning the document as far as I can tell
Bruce pointed out that it should be able to get away with just a weak ref to the document (rather than a strong ref to the content as I suggested). (I don't think a weak ref to the content would work, though, since it is held by the ESM which is held by the document, which could outlast the content.)
Changing to a weak reference to the doc (patch coming) fixes the doc leak, but it doesn't fix all the leaks because there's a second cycle (aargh). The relevant nsEventListenerManager is created by nsGenericElement::GetListenerManager (the mInner of the nsHTMLAreaElement) and the area element is held by the nsImageMap in a strong ref added in AddArea.
Could whatever these event listeners were intended to fix be done by changing the HandleDOMEvent method of some element, or something?
Not going to hold the tree for this, as i've heard tell of traction/fix.
We could listen for focus at the frame level if we have to. So the cycle is this? nsHTMLAreaElement-> nsEventListenerManager -> nsImageMap -> back to nsHTMLAreaElement The other patch is good.
Oh, it is nsImageMap -> nsIContent (image map area) -> nsEventListenerManager -> back to nsImageMap
fixed, I think I got both the leaks Hopefully there arn't any more...
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Reopening. You didn't get the second cycle. Making the nsImageMap's pointer to the DOMMap doesn't fix it. IIRC the nsImageMap still owns the nsHTMLAreaElements, which owns the ESM (in the DOMSlots or whatever), which owns the nsImageMap. The leak stats on tinderbox still show the nsImageMap, nsHTMLAreaElement, etc. leaking.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't see how nsImageMap owns the elements... show me the line of code.
class Area has member |nsCOMPtr<nsIContent> mArea|. nsImageMap::AddArea creates an object derived from Area, which owns the nsHTMLAreaElement, and adds it to the mAreas nsVoidArray.
But nsImageMap::AddArea doesn't addref the entry in the nsVoidArray. Thanks for the info, I'll keep looking at this (obviously still mentally missing something)
The Area objects (in the mAreas array) aren't refcounted. However, *they* addref the area element.
So I checked in a fix for this cycle; I added the Destroy method to nsImageMap which unregisters the event listeners. This is called from the nsImageFrame::Destroy method just before releasing nsImageMap. This should break the cycle.
Okay, fixed it again!
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
could someone verify this with a sun build? thanks.
Verified based on tinderbox leak stats. You don't need Solaris (and Purify) for XPCOM leak logging.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: