98792 - misuse of nsIAppShellService::UnregisterTopLevelWindow [@ nsAppShellService::UnregisterTopLevelWindow]

Status: RESOLVED FIXED

(Core Graveyard :: Cmd-line Features, defect)

Description

[Dan M]

I managed to insert a crasher bug by writing code that assumed I was being
handed an actual window in nsAppShellService::UnregisterTopLevelWindow. It turns
out there are two places in Mozilla code that are using the function for its
side effect of quitting the app. This happened to work because I was foolish
enough to to put a null check on Unregister...'s parameter.

Comment 1

[Dan M]

Attachment: fix null parameters to UnregisterTopLevelWindow

Comment 2

[Conrad Carlen (not reading bugmail)]

Dan, take a look at the code here:
http://lxr.mozilla.org/mozilla/source/xpfe/bootstrap/nsNativeAppSupportWin.cpp#765
Looks like it uses GetMostRecentWindow() and if null, enables the menu so it can
send a -kill command to itself. So, if -kill was invoked that way, we know the
last window is closed. I guess the other possibility is somebody typing "mozilla
-kill" on the command line. Just to keep things consistent, shouldn't we use
GetMostRecentWindow() in the same way was the other place?

Comment 3

[Dan M]

right you are. simpler patch attached below.

Comment 4

[Dan M]

Attachment: better fix for callers sending us a null parameter

Comment 5

[Conrad Carlen (not reading bugmail)]

Comment on attachment 48647 [details] [diff] [review]
better fix for callers sending us a null parameter

r=ccarlen

Comment 6

[Blake Ross]

Exiting from the turbo systray icon is broken because of the original change; I
had a patch in 88844 but I see it's being handled here.  As Conrad said, the
menuitem will be disabled if there aren't any open windows, so I take it the
added check is just for people calling -kill.  Not sure who'd ever do that, but
it can't hurt, so sr=blake. I'll be sad to see
SOMEBODY_SET_UP_US_THE_NULL_POINTER go.

Comment 7

[Sean Su]

FYI: the uninstaller and the installer uses the -kill feature.

Comment 8

[Alec Flett]

Comment on attachment 48647 [details] [diff] [review]
better fix for callers sending us a null parameter

sr=alecf

Comment 9

[Dan M]

Patch is in. (oops! I forgot to blame Blake, too, in the review notes.) I already 
miss SOMEBODY_SET_UP_US_THE_NULL_POINTER.

Comment 10

[greer]

Sorry for the spam guys: just adding keywords and summary changes so that 
talkback can show this one fixed.