257803 - Possible RegistryKey Leak in nsWindowsShellService

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Henrik Lynggaard Hansen]

Running my Inspector it found a possible RegKey leak.

Will attach a stack trace showing where it is allocated, but apperently never 
deallocated (missing RegKeyClose)

Comment 1

[Henrik Lynggaard Hansen]

Attachment: Report and allocation stackstrace

Seems like a missing RegKeyClose at bottom of method.

Comment 2

[Henrik Lynggaard Hansen]

Requesting block. 

* It is a leak (although minor) 
* Has stack trace pinpointing the problem area
* Has AFAIK a easy fix. (One missing RegKeyClose)
* Urban legends among trolls is that Firefox leaks, lets not give them ammo

note:possibly same error in OpenUserKeyForWriting

Comment 3

[rOmAz]

Attachment: adds calls to RegCloseKey in missing places

from msdn.microsoft.com: "The handle for a specified key should not be used
after it has been closed, because it will no longer be valid. Key handles
should not be left open any longer than necessary."

Most functions in nsWindowsShellService.cpp call RegOpenKeyEx without closing
it. The patch fixes the problem by adding RegCloseKey in all functions in that
file.

-rOmAz

Comment 4

[Asaf Romano (gone)]

-> rOmAz

Comment 5

[Jungshik Shin]

Comment on attachment 159326 [details] [diff] [review]
adds calls to RegCloseKey in missing places

r=jshin

Thanks for fixing it.

Perhaps, we have to fix mozilla as well.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Mike, want to review this and check in?

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 159326 [details] [diff] [review]
adds calls to RegCloseKey in missing places

r=me (not really SR, but I don't feel like bugspamming people to make the r=me
look right.  Thanks for the patch!

Comment 8

[Mike Connor [:mconnor]]

Checking in browser/components/shell/src/nsWindowsShellService.cpp;
/cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v  <--
nsWindowsShellService.cpp
new revision: 1.12; previous revision: 1.11
done

Comment 9

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

Looks like this broke the build:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: Bustage fix

Just a typo.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 159326 [details] [diff] [review]
adds calls to RegCloseKey in missing places

Casing issue...

>Index: browser/components/shell/src/nsWindowsShellService.cpp
>===================================================================

>+      ::RegCloseKey(theKey);

This is the right


>@@ -581,6 +593,9 @@
>   if (REG_FAILED(result) || strcmp(buf, aValue) != 0)
>     ::RegSetValueEx(theKey, aValueName, 0, REG_SZ, 
>                     (LPBYTE)aValue, nsDependentCString(aValue).Length());
>+  
>+  // Close the key we opened.
>+  ::RegClosekey(theKey);

RegClose*K*ey

Comment 12

[Asaf Romano (gone)]

Comment on attachment 171448 [details] [diff] [review]
Bustage fix

checked in by timeless

Comment 13

[Worcester12345]

(In reply to comment #5)
> (From update of attachment 159326 [details] [diff] [review] [edit])
> r=jshin
> 
> Thanks for fixing it.
> 
> Perhaps, we have to fix mozilla as well. 
> 
Was this just a Firefox fix? If so, WILL it go into Mozilla as well?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Was this just a Firefox fix? 

Yes.

> If so, WILL it go into Mozilla as well?

The fix was to Firefox-only code.  If you find code in Mozilla that has similar
issues, please file a bug on it.