Closed Bug 398665 Opened 12 years ago Closed 12 years ago

[FIX]<keygen> leaks two nsStringBuffers

Categories

(Core :: Layout: Form Controls, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: bzbarsky)

Details

(Keywords: memory-leak, testcase)

Attachments

(2 files, 1 obsolete file)

Steps to reproduce:
1. export XPCOM_MEM_LEAK_LOG=2
2. run debug firefox
3. load the testcase
4. quit

Result: trace-refcnt reports that 2 nsStringBuffers have leaked.
Flags: blocking1.9?
Attached file testcase
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #283671 - Flags: superreview?(jst)
Attachment #283671 - Flags: review?
Attachment #283671 - Flags: review? → review?(kengert)
OS: Mac OS X → All
Priority: -- → P2
Hardware: PC → All
Summary: <keygen> leaks two nsStringBuffers → [FIX]<keygen> leaks two nsStringBuffers
Target Milestone: --- → mozilla1.9 M10
Attachment #283671 - Flags: superreview?(jst) → superreview+
Comment on attachment 283671 [details] [diff] [review]
PSM bug all the way

>+  // XXXbz this leaks the strings through shutdown.  There's got to be
>+  // a better way to do this!  Of course that would involve having SOME
>+  // shutdown code somewhere here.
>   nssComponent->GetPIPNSSBundleString("HighGrade", str);
>   SECKeySizeChoiceList[0].name = ToNewUnicode(str);
> 
>   nssComponent->GetPIPNSSBundleString("MediumGrade", str);
>   SECKeySizeChoiceList[1].name = ToNewUnicode(str);


Why don't we free it in 
nsKeygenFormProcessor::~nsKeygenFormProcessor() ?

I think SECKeySizeChoiceList could even be changed to be a member variable of nsKeygenFormProcessor.
Attachment #283671 - Flags: review?(kengert) → review+
> Why don't we free it in 
> nsKeygenFormProcessor::~nsKeygenFormProcessor() ?

There seem to be provisions for having more than one of those around.  Is it actually a service?

> I think SECKeySizeChoiceList could even be changed to be a member variable

Seems like a good idea, yes.
Attachment #283671 - Flags: approval1.9?
Comment on attachment 283671 [details] [diff] [review]
PSM bug all the way

a1.9=dbaron
Attachment #283671 - Flags: approval1.9? → approval1.9+
(In reply to comment #4)
> > I think SECKeySizeChoiceList could even be changed to be a member variable
> 
> Seems like a good idea, yes.


I assume you'll check your patch in.
Once you did, we should do that additional change in a follow up patch.
Please let me know if you want me to do it.
Checked in.

> Please let me know if you want me to do it.

If you could, that would be great.  We should also get the testcase into some kind of leak suite...
Flags: blocking1.9? → in-testsuite?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #284264 - Flags: superreview?(bzbarsky)
Attachment #284264 - Flags: review?(rrelyea)
Comment on attachment 284264 [details] [diff] [review]
More leak fixes v1 [moved to bug 399318]

Kai, please ask someone else?  I'm not going to be able to really focus on reviewing this for the foreseeable future...
I think you should make a new bug report for the additional patch.
Comment on attachment 284264 [details] [diff] [review]
More leak fixes v1 [moved to bug 399318]

separate bug 399318 filed and this attachment moved over to it.
Attachment #284264 - Attachment description: More leak fixes v1 → More leak fixes v1 [moved to bug 399318]
Attachment #284264 - Attachment is obsolete: true
Attachment #284264 - Flags: superreview?(bzbarsky)
Attachment #284264 - Flags: review?(rrelyea)
I checked the testcase in as a crashtest.  Apparently no existing reftests or crashtests contained <keygen>!
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.