398665 - [FIX]<keygen> leaks two nsStringBuffers

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: PSM bug all the way

Comment 3

[Kai Engert [:KaiE:]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

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

Comment on attachment 283671 [details] [diff] [review]
PSM bug all the way

a1.9=dbaron

Comment 6

[Kai Engert [:KaiE:]]

(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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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...

Comment 8

[Kai Engert [:KaiE:]]

Attachment: More leak fixes v1 [moved to bug 399318]

Comment 9

[Boris Zbarsky [:bzbarsky]]

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...

Comment 10

[Jesse Ruderman]

I think you should make a new bug report for the additional patch.

Comment 11

[Kai Engert [:KaiE:]]

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.

Comment 12

[Jesse Ruderman]

I checked the testcase in as a crashtest.  Apparently no existing reftests or crashtests contained <keygen>!