489135 - Lk increase due to bug 58904 (XPCOM should have nsLock strong type to match nsAutoLock)

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Mark Banner (:standard8)]

Attachment: diffbloatdump.pl output

From the landing of bug 58904 there was an apparent leak regression on Tinderbox:

Linux and Windows: approx 4kB
Mac: hard to tell

http://graphs.mozilla.org/#show=1473418,1473670,1474269 date of regression is approx 19th April 2009 21:00

Attaching the diffbloatdump.pl output of the differences between the two builds (on Linux)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There's only one bit of code that patch touched that's currently used anywhere.  And, lo and behold, that bit of code allocates a PRMonitor that's not being freed (nsStringBundle).

Interestingly enough, nsStringBundle was leaking PRMonitors in the same way before.  It was using a "cached monitor" that was never freed.  Only now are we able to see that this memory leaks.

This is an easy one-liner fix.  Coming up when I get it into work later.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix pre-existing leak and uninit warning from valgrind

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

BTW, this isn't a regression --- this memory was already being leaked, but in a way that valgrind couldn't detect.  Don't know if the keyword is all that important, so I'll leave it.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 373704 [details] [diff] [review]
Fix pre-existing leak and uninit warning from valgrind

Whoops, this is in the intl module.  Sorry Benjamin.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

smontagu, we should have had you review the nsStringBundle part of this patch: https://bugzilla.mozilla.org/attachment.cgi?id=373538&action=diff .  It was quite straightforward: PRCMonitor has been summarily deprecated (no exceptions), so we replaced the PRCMonitor with PRMonitor.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: pushable changeset [Checkin: Comment 7]

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 373741 [details] [diff] [review]
pushable changeset
[Checkin: Comment 7]


http://hg.mozilla.org/mozilla-central/rev/fbfbc1e04f4e

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bug 58904 won't land on branch and so neither should this.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> Bug 58904 won't land on branch and so neither should this.

Ah, I must have wrongly assumed that comment 1 "before" included 1.9.1 :-<

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #9)
> (In reply to comment #8)
> > Bug 58904 won't land on branch and so neither should this.
> 
> Ah, I must have wrongly assumed that comment 1 "before" included 1.9.1 :-<

It does.  But the patch in bug 58904 has so much other bleeding-edge crap that it shouldn't be backported.

I guess the previous leak wasn't all that big of a deal...

Comment 11

[Mark Banner (:standard8)]

Verified - the leaks went back down again. Thanks for fixing this.

(In reply to comment #10)
> I guess the previous leak wasn't all that big of a deal...

I just had a look through TB's trace malloc leak logs and couldn't find a PRMonitor leak logged on the 1.9.1 builds. As we saw it for this bug on trunk, I'd expect to see it in the 1.9.1. So either it wasn't a leak, or even trace malloc didn't pick it up.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #11)
> (In reply to comment #10)
> > I guess the previous leak wasn't all that big of a deal...
> 
> I just had a look through TB's trace malloc leak logs and couldn't find a
> PRMonitor leak logged on the 1.9.1 builds. As we saw it for this bug on trunk,
> I'd expect to see it in the 1.9.1. So either it wasn't a leak, or even trace
> malloc didn't pick it up.

Right.  It was leaking in a way that leak detectors can't detect.