Leaking nsIAtom... (Caused by previous RTM checkin)

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: harishd, Assigned: harishd)

Tracking

({memory-leak})

Trunk
x86
Windows NT
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-][Fix in hand][r=pollmann, sr=vidur])

Attachments

(2 attachments)

(Assignee)

Description

18 years ago
We're leaking an nsIAtom for a META charset in the document.

Patch it to fix the leak:


Index: nsObserverBase.cpp
===================================================================
RCS file: /cvsroot/mozilla/intl/chardet/src/nsObserverBase.cpp,v
retrieving revision 1.13.4.1
diff -u -w -r1.13.4.1 nsObserverBase.cpp
--- nsObserverBase.cpp	2000/11/02 00:39:26	1.13.4.1
+++ nsObserverBase.cpp	2000/11/02 04:25:24
@@ -67,6 +67,7 @@
            atom->ToString(method);
            if(method.EqualsWithConversion("POST"))
              return NS_OK; 
+           NS_RELEASE(atom);
          }
        }
      }
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Keywords: mlk, rtm
Whiteboard: [rtm+][Fix in hand][r=pollmann, sr=vidur]
(Assignee)

Comment 1

18 years ago
Fix landed on the trunk..
(Assignee)

Comment 2

18 years ago
Created attachment 18549 [details] [diff] [review]
The correct patch [ Please ignore the patch above ]
(Assignee)

Updated

18 years ago
Whiteboard: [rtm+][Fix in hand][r=pollmann, sr=vidur] → [rtm need info][Fix in hand][r=pollmann, NEED sr=]

Comment 3

18 years ago
How about using a nsCOMPtr? I'd feel more comfortable with nsCOMPtr usage and a
slightly larger patch than the addition of a NS_RELEASE and a 1-liner. Zaro
Reesk in either case. ;-)
(Assignee)

Comment 4

18 years ago
Created attachment 18563 [details] [diff] [review]
Revised patch [ and so it be V ;-)]

Comment 5

18 years ago
The new patch looks good (though the null initialization isn't, strictly
speaking, necessary). sr=vidur.

Comment 6

18 years ago
Since this is the result of a fairly recent checkin, I think this should go into
rtm...even though the actual leakage is fairly small. (Since I reviewed the
previous checkin, it'd make me feel less guilty too. :-))
(Assignee)

Updated

18 years ago
Whiteboard: [rtm need info][Fix in hand][r=pollmann, NEED sr=] → [rtm+][Fix in hand][r=pollmann, sr=vidur]

Updated

18 years ago
Summary: Leaking nsIAtom... → Leaking nsIAtom... (Caused by previous RTM checkin)
(Assignee)

Comment 7

18 years ago
Fix landed on the trunk..

Comment 8

18 years ago
Harish, how serious might this leak be?  pdt will want to make a cost/benefit
call on this.  Cost is small, but what's the benefit?  Does it affect a large
number of international users?  And can the leak be significant over, say, 10
hour time frames?
(Assignee)

Comment 9

18 years ago
I pressume all international documents will have atleast one meta charset on it 
and for every meta charset we would leak about 16 bytes ( sizeof nsIAtom ).

Comment 10

18 years ago
rtm-, not looking for small leaks now.
Whiteboard: [rtm+][Fix in hand][r=pollmann, sr=vidur] → [rtm-][Fix in hand][r=pollmann, sr=vidur]

Comment 11

18 years ago
Fix landed on the trunk, PDT beat, stomped, shot, stabbed, and insulted
developer for even trying to get patch into RTM, therefore, marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 12

18 years ago
Changed QA contact to harishd@netscape.com.
QA Contact: teruko → harishd
You need to log in before you can comment on or make changes to this bug.