211834 - Crash when trying to show prompts without a parent window

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Marco Pesenti Gritti]

I got this trace from an epiphany user. I cant reproduce it because it requires
to have mailnews compiled in, which I havent.

0x4002d63f in GtkPromptService::GetGtkWindowForDOMWindow(nsIDOMWindow*) ()
   from /usr/lib/mozilla-1.4/libgtkembedmoz.so
(gdb) bt
#0  0x4002d63f in GtkPromptService::GetGtkWindowForDOMWindow(nsIDOMWindow*) ()
   from /usr/lib/mozilla-1.4/libgtkembedmoz.so
#1  0x4002cabf in GtkPromptService::Confirm(nsIDOMWindow*, unsigned short
const*, unsigned short const*, int*) () from /usr/lib/mozilla-1.4/libgtkembedmoz.so
#2  0x410223cf in NSGetModule ()
   from /usr/lib/mozilla-1.4/components/libembedcomponents.so
#3  0x42abe923 in NSGetModule ()
   from /usr/lib/mozilla-1.4/components/libmsgnews.so
#4  0x42a8fab4 in nsMsgProtocol::AsyncOpen(nsIStreamListener*, nsISupports*) ()

Anyway I think the problem is that nsIDOMWindow is NULL and we dont handle that
case. Confirm is used several times with first argument NULL, for example in
nsNNTPProtocol.cpp.

Comment 1

[Marco Pesenti Gritti]

Attachment: Check for NULL

Comment 2

[Marco Pesenti Gritti]

You can find a reproducable testcase here:
http://www.peelo.com/crashephy.html

If my patch (or another one) is committed, it would be good if it could go also
on the 1.4 branch.

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 127110 [details] [diff] [review]
Check for NULL

I'd like this to be:

NS_ENSURE_TRUE(aDOMWindow, NULL);

so that a warning is printed out in debug builds.  Opening an alert dialog with
no parent is bad bad bad.

Comment 4

[Brian Ryner (not reading)]

Attachment: null check with warning

Comment 5

[Christopher Blizzard (:blizzard)]

Aren't there some situations where dialogs _are_ opened without a parent?  I
seem to remember the SSL dialog and the prefs dialog being without a parent.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 128360 [details] [diff] [review]
null check with warning

Well, it does suck, but you're right, the API doesn't warn against using a null
parent.  Let's go with the warning-less patch.

Comment 7

[Christopher Blizzard (:blizzard)]

Comment on attachment 127110 [details] [diff] [review]
Check for NULL

Hang on, so does this mean that we're going to just not show some prompts?  So
instead of crashing we're breaking some functionality?	That doesn't sound
right either.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 127110 [details] [diff] [review]
Check for NULL

Er, no, it just means we'll use a null parent window for those prompts.

Comment 9

[Christopher Blizzard (:blizzard)]

Comment on attachment 127110 [details] [diff] [review]
Check for NULL

Sold.

Comment 10

[Marco Pesenti Gritti]

Comment on attachment 127110 [details] [diff] [review]
Check for NULL

This is a critical issue for embed. We get bug reports everyday about it.
The patch is simple (just a NULL check for when dialogs has not a parent), so I
think there are no risks at all.
I'm requesting approval also for 1.4.x because Epiphany (GNOME 2.4 browser)
will be based on that version of mozilla.

Comment 11

[Mike Kaply [:mkaply]]

Comment on attachment 127110 [details] [diff] [review]
Check for NULL

a=mkaply for  1.4.1 and 1.5b

Comment 12

[Mike Kaply [:mkaply]]

Please add the fixed1.4.1 keyword when this is checked in.

If you need someone to check this in, please send me email.

Comment 13

[Mike Kaply [:mkaply]]

Fix checked in.