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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
XPCOM
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: standard8, Assigned: cjones)

Tracking

({memory-leak, regression})

Trunk
mozilla1.9.2a1
memory-leak, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 373626 [details]
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)
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.
Assignee: nobody → jones.chris.g
Flags: blocking1.9.2?
Created attachment 373704 [details] [diff] [review]
Fix pre-existing leak and uninit warning from valgrind
Attachment #373704 - Flags: review?(benjamin)
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 on attachment 373704 [details] [diff] [review]
Fix pre-existing leak and uninit warning from valgrind

Whoops, this is in the intl module.  Sorry Benjamin.
Attachment #373704 - Flags: review?(benjamin) → review?(smontagu)
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.
Attachment #373704 - Flags: review?(smontagu) → review+
Created attachment 373741 [details] [diff] [review]
pushable changeset
[Checkin: Comment 7]
Attachment #373704 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 373741 [details] [diff] [review]
pushable changeset
[Checkin: Comment 7]


http://hg.mozilla.org/mozilla-central/rev/fbfbc1e04f4e
Attachment #373741 - Attachment description: pushable changeset → pushable changeset [Checkin: Comment 7]
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: blocking1.9.2? → wanted1.9.1?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Bug 58904 won't land on branch and so neither should this.
Flags: wanted1.9.1?
(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 :-<
(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...
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.
Status: RESOLVED → VERIFIED
(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.
You need to log in before you can comment on or make changes to this bug.