libXss.so.1 is unloaded while an XESetCloseDisplay callback is still registered

RESOLVED FIXED in mozilla1.9beta1

Status

()

Core
Widget: Gtk
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla1.9beta1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
nsIdleServiceGTK loads and unloads libXss, but, if XScreenSaverQueryExtension
is called, then the library registers a shutdown function callback through XESetCloseDisplay.
The library exports no shutdown functions, or means to unregister this callback and so should not be unloaded until after the display(s) is closed.

Bug 394466 comments 15 to 17 have some details.

Options here are:

1. Just don't unload the library
   This has an advantage that external libraries that may use libXss don't
   get left in the cold.

2. Unregister the callback and call the shutdown function ourselves.
   There are no appropriate interfaces provided by the library, so we would
   have to hunt for the extension, using knowledge of it name, in the
   ext_procs field of struct _XDisplay in Xlibint.h.
   This may be assuming more about the implementation than we should.

3. Link against libXss.so rather than dlopening.
   This would add an unnecessary dependency.

4. Unload the library after closing the display.
   This is a bit hard to imagine, unless NSPR is modified do this when it is
   shutdown (similar to the WIN16 implementation of _PR_ShutdownLinker),
   possibly only when another flag is set for PR_LoadLibraryWithFlags.
(Assignee)

Updated

10 years ago
Blocks: 343416
I'd say just do (1), since it's easiest, although it doesn't have the advantage you mention since if other libraries use it they'd prevent it from being unloaded.
(That said, rather than removing the PR_UnloadLibrary, I'd comment it out, with a comment explaining why.)
(Assignee)

Comment 3

10 years ago
(In reply to comment #1)
> I'd say just do (1), since it's easiest, although it doesn't have the advantage
> you mention since if other libraries use it they'd prevent it from being
> unloaded.

You are right of course, thanks:  despite the name, PR_UnloadLibrary closes a handle rather than unloading the library.
(Assignee)

Comment 4

10 years ago
Created attachment 282464 [details] [diff] [review]
don't unload, and delay loading

NS_ASSERTION(!xsslib, "created two instances of the idle service") wasn't
quite right when xsslib is never reset, so this includes a little modification
to handle more than one (sequential or concurrently existing) instance (which
probably doesn't happen).

With that change it then made sense to only load the library when necessary,
one advantage of which is that we don't hunt for the library when first
opening the bookmarks menu (and also we don't open the library when there is
no display but widget/gtk2 is still used???).
(Assignee)

Comment 5

10 years ago
Comment on attachment 282464 [details] [diff] [review]
don't unload, and delay loading

I'll ask roc for review as its in widget/gtk2, but dbaron feel free to comment if you wish.
Attachment #282464 - Flags: superreview?(roc)
Attachment #282464 - Flags: review?(roc)
Attachment #282464 - Flags: superreview?(roc)
Attachment #282464 - Flags: superreview+
Attachment #282464 - Flags: review?(roc)
Attachment #282464 - Flags: review+
Attachment #282464 - Flags: approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Checking in widget/src/gtk2/nsIdleServiceGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsIdleServiceGTK.cpp,v  <--  nsIdleServiceGTK.cpp
new revision: 1.3; previous revision: 1.2
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Depends on: 460979
You need to log in before you can comment on or make changes to this bug.