179151 - Need to call FreeLibrary()

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Roy Yokoyama]

I see HMODULE module = ::LoadLibrary("Shell32.dll");
but no call for FreeLibrary(module) is mode.
Memory leak!

Comment 1

[Roy Yokoyama]

Attachment: Call FreeLibrary()

shanjian: can you review?

Comment 2

[Shanjian Li]

Comment on attachment 105654 [details] [diff] [review]
Call FreeLibrary()

r=shanjian, good catch.

Comment 3

[David Bradley]

Just a sanity check, but won't the results of the GetProcAddress calls before
this FreeLibrary call get invalidated by adding this call? Those are saved off,
and I believe used later.

Comment 4

[Roy Yokoyama]

Attachment: Call FreeLibrary() in nsToolkit::Shutdown()

Though it doesn't seem to invalidate the function pointers, it's better
be safe than sorry.  I am moving the FreeLibrary() to nsToolkit::Shutdown();

While at it, I realized that it's better to calll ::UnregisterClassW() 
when unregistering the nsToolkitClass class.  It was with my surprise, however,

that the return value from ::UnregisterClass() was SUCCESS despite the fact
that
we call ::RegisterClassW(). I think we should call ::UnregisterClassW() 
in any events.

shanjian: can you review again?

Comment 5

[Shanjian Li]

Comment on attachment 106007 [details] [diff] [review]
Call FreeLibrary() in nsToolkit::Shutdown()

r=shanjian

Comment 6

[kinmoz]

Comment on attachment 106007 [details] [diff] [review]
Call FreeLibrary() in nsToolkit::Shutdown()

==== Fix indentaion of the |FreeLibrary()| call:

+    if (nsToolkit::mShell32Module)
+	 ::FreeLibrary(nsToolkit::mShell32Module);


==== I'm assuming we only ever create one Toolkit when mozilla is run, even in
the embedded world, and that we don't have to keep an instance count around to
track when we don't have to call |LoadLibrary()| and when to call
|FreeLibarary()| right?

Comment 7

[Roy Yokoyama]

rods/kevin: 
What's the intended life cycle of the Toolkit?
Do you think we need to keep the ref-count of nsToolkit::mShell32Module?

Comment 8

[Kevin McCluskey (gone)]

The toolkit is created when the first window is created and destroyed when the
last window instance is destroyed. If there aren't any nsIWidget instances then
there shouldn't be a toolkit instance.  When mozilla starts up the toolkit is
created when the profile manager window is created. The toolkit is destroyed
after the user selects a profile. A new toolkit instance is created when the
Mozilla browser window is created. This toolkit instance will not be destroyed
until the user exits the application. 

Comment 9

[Kevin McCluskey (gone)]

"I'm assuming we only ever create one Toolkit when mozilla is run"

This is a true statement. There will never more than 1 toolkit instance, but as
I stated in the previous comment the toolkit may be created and destroyed
multiple times if all of the nsIWidget instances are destroyed.

Comment 10

[Kevin McCluskey (gone)]

"Do you think we need to keep the ref-count of nsToolkit::mShell32Module?"

No. But, It will be loaded and free'd twice in sequence when Mozilla is executed
and the user has a multiple profiles.

Comment 11

[Roy Yokoyama]

checked in. Thanks all.

Comment 12

[Yuying Long]

Roy, I don't think I am the right person to verify this, can you take care of
this? thanks!  Change QA contact to [e-mail]