48373 - 1.2 MB leak regression on Tinderbox

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There was a 1.2 MB leak regression on the tinderbox leak stats late on August 9
or in the early morning hours of August 10.  It happened while the tree was red
and lots of people were checking in, so it's hard to point to the exact cause
from tinderbox.

The people who checked in between 23:14 and 00:29 were:
warren, morse, scc, valeski, mcafee, cmanske, alecf
[ some of the checkins couldn't have caused anything this bad, though ]

We now leak 15 of the 17 nsDocument objects created during the run, with 17
leaked references total.  We leak 1 of 3 GlobalWindowImpl, and 2 of 2
nsXULDocument.

Comment 1

[Judson Valeski]

I was backing conrad's changes out.

Comment 2

[Charles Manske]

What code in the editor, if any, is run for the leak tests?
I have one suspicious area in my checkin, but I doubt if it is run: it would 
be switching to "HTML Source" editmode, then back to "Normal" mode again.

Comment 3

[Judson Valeski]

over to warren for now.

Comment 4

[Chris McAfee]

isn't a textarea using editor code?

Comment 5

[Chris McAfee]

it's hard to tell, but there are 5 possible checkin blocks where the leak showed
up:

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=SeaMonkeyAll&date=explicit&mindate=965891700&maxdate=965892539
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=SeaMonkeyAll&date=explicit&mindate=965891040&maxdate=965891699
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=SeaMonkeyAll&date=explicit&mindate=965889660&maxdate=965891039
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=SeaMonkeyAll&date=explicit&mindate=965888880&maxdate=965889659
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=SeaMonkeyAll&date=explicit&mindate=965888040&maxdate=965888879

new leaks reported: attached

Comment 6

[Chris McAfee]

Attachment: new leaks

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem looks like warren's changes to the CSSLoader.  Testing fix now.

Comment 8

[Chris McAfee]

putting on blocker list to get some traction on this before
the tree opens.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: proposed fix

Comment 10

[Warren Harris]

What I posted to porkjockeys last night... (I'm going to investigate today)...

Subject: 
         nsStringKey and hashtable changes
   Date: 
         Thu, 10 Aug 2000 02:07:18 -0700
   From: 
         Warren Harris <[e-mail]>
      To: 
         [e-mail]




I checked in my changes tonight to optimize nsStringKey's memory
consumption. I split it into 2 classes nsStringKey and nsCStringKey, and
it turns out that nsCStringKey is what is usually called for in our
code. There are assertions in place if you try to mix different types of
keys in the same hashtable, so beware.

Making these changes seemed to reduce the tinderbox bloat figure by
about 1M. Looking at the log, I see almost all of this was due to nsStr
(as expected, nsStr 9628980  -5.36%). This is good.

However, at the same time I seem to have introduced a leak of about 43k
which I need to track down. Looks like keys _and_ hashtables are being
leaked (among other things). I have no idea why more hashtables would be
leaked than before, but I'm investigating...

Warren

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Your changes to nsCSSLoader turned a weak-reference-to-pointer hash key into a
strong-reference-to-pointer hash key, which created circular ownership between
document and CSS loader.  That's the 1.2M leak.

Comment 12

[Jon Granrose]

sounds like there's some traction on this. should we hold the tree closed today 
for this?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Anybody want to review the fix?

Comment 14

[Warren Harris]

Fix looks good to me.

Comment 15

[Warren Harris]

And it's in... marking fixed.

Comment 16

[Asa Dotzler [:asa]]

warren, david, can one of you verify this bug.  Thanks.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK.  Tinderbox currently shows 1-2K.