374332 - many callers don't do PR_UnloadLibrary

Status: RESOLVED FIXED

(Core :: General, defect)

Description

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

I did a code audit of callers that should be calling PR_UnloadLibrary.  In particular, PR_LoadLibrary, PR_LoadLibraryWithFlags, PR_LoadStaticLibrary, PR_FindSymbolAndLibrary, PR_FindFunctionSymbolAndLibrary, and nsILocalFile::Load must be matched by PR_UnloadLibrary.

I skipped two things:
 1) unloading of XPCOM components
 2) nsPluginsDir*

I was hoping that this would contribute to fixing the leaks that show up in trace-malloc shutdown leak logs (and presumably valgrind "full" leak output) that are related to PR_LoadLibrary.  It didn't seem to help much; I'm guessing the XPCOM component loading is a bigger issue.

But this also might be useful for things like XPCOM restartability within process.

Comment 1

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

Attachment: patch

I've built and tested this on Linux; I should probably do some tests on other platforms as well, although most of the platform-specific changes are in GTK code.

Comment 2

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

Comment on attachment 258912 [details] [diff] [review]
patch

I've built this on Windows as well as Linux.

Comment 3

[Benjamin Smedberg]

I'm not going to be able to get to this until late next week.

Comment 4

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

Checked in to trunk earlier today.

Comment 5

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

Unloading libraries in nsNativeComponentLoader is bug 60709 / bug 345430.

Fixing the plugins code is bug 377605.

Comment 6

[Ginn Chen]

dbaron,
We intent to NOT unload atk-bridge, libgail, libatk.
As comments in mozilla/accessible/src/atk/nsAppRootAccessible.cpp.
I'm sorry I didn't make it obvious enough.

It's now causing crashes at exit with desktop accessibility on on linux/Solaris.

Would you mind back out your changes to this file?

Comment 7

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

OK, I'll comment them out.  I thought the comments said not to call the shutdown method.

Comment 8

[Michael Wu [:mwu]]

The chunk affecting extensions/pref/system-pref/src/gconf/nsSystemPrefService.cpp should be backed out. GConf should not be unloaded once it is loaded. The comments in the Init function say:

    // Don't unload past this point, since GConf's initialization of ORBit
    // causes atexit handlers to be registered.

It causes crashes when libgconf-2 is found but libgnome-2 is not. This is because having libgnome-2 installed also pins libgconf-2 and conceals the problem. This problem has been crashing my build.

Comment 9

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

(In reply to comment #8)
> The chunk affecting
> extensions/pref/system-pref/src/gconf/nsSystemPrefService.cpp should be backed
> out.

Please file a new bug for this issue, if you haven't already?