55627 - Can crash when ab-using the 'helperAppLauncher.xul' dialog

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[John Morrison]

Overview Description:
  The dialog 'helperAppLauncher.xul' is not modal, which allows the
  user to kill the parent window. If the user does this, and then
  proceeds with the dialog to do the file save, mozilla will crash.

  See http://bugzilla.mozilla.org/show_bug.cgi?id=55626, for a bug
  on just making this dialog modal, which would prevent this crash
  from being triggered by user actions.

Steps to Reproduce:
1) go to www.mozilla.org
2) try to download one of the .zip, .gz, .bin files linked to at the
   bottom right corner of the page (e.g. "Window, Irix, ...").
3) when the dialog comes up, kill the parent window (hit the close box
   on the window).
4) Click OK to 'Save To Disk...'

Actual Results:   crashes
Expected Results: a) shouldn't be allowed to kill the parent window
                  b) no crash; do the download if possible.

Build Date & Platform Bug Found:
  20001006nn win2k, mac  branch MN6 builds
  20001006nn MN6 linux does not crash, just exits.

Comment 1

[John Morrison]

Nom. for rtm (unless the simpler fix, make it modal is done: bug 55626).

(law is hating me right now ...)

Comment 2

[don]

Bill, tell me there's an easy way to fix this ...

Comment 3

[Scott MacGregor]

Making the dialog modal is obviously the right thing to do here. So maybe we
should just dup the two bugs.

I'm not convinced it needs fixed for rtm though....

Comment 4

[John Morrison]

http://cyclone/reports/reporttemplate.cfm?style=0&reportID=1154 says that 
this (or some similar steps to reproduce) is actually a fairly common 
crasher in PR3 and other builds. There are 52 talkback reports that have
the same stack trace as this crash, almost all with build ID 2000092909 (PR3)
and later (including heikki and ftang).

Comment 5

[John Morrison]

I took a blind guess, and the following prevents the crash, and surprisingly
(to me) the native filepicker will come up, the download still correctly 
completes, and the downloaded file is saved safely (at least on windows,
on linux it just still exits silently; don't know about Mac). 

This is for the steps outlined above: get the dialog, nuke the parent window,
click OK in the dialog, "used-to-crash-here", download/file-save completes OK.
I don't know though, if there are any negative consequences to just silently
bailing out of this method, although it looks like the only caller of this
method, nsBaseFilePicker::Init, just takes a default course of action when
it can't get the nsIWidget.

H:\mozilla\mozilla\widget\src\xpwidgets>cvs diff -u
cvs server: Diffing .
Index: nsBaseFilePicker.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/xpwidgets/nsBaseFilePicker.cpp,v
retrieving revision 1.9
diff -u -r1.9 nsBaseFilePicker.cpp
--- nsBaseFilePicker.cpp        2000/09/13 02:46:35     1.9
+++ nsBaseFilePicker.cpp        2000/10/10 06:38:23
@@ -69,7 +69,7 @@
       nsCOMPtr<nsIPresShell> presShell;
       rv = docShell->GetPresShell(getter_AddRefs(presShell));

-      if (NS_SUCCEEDED(rv)) {
+      if (NS_SUCCEEDED(rv) && presShell) {
         nsCOMPtr<nsIViewManager> viewManager;
         rv = presShell->GetViewManager(getter_AddRefs(viewManager));

Comment 6

[Scott MacGregor]

nice catch John. Null ptr checks are usually easy to get by PDT too...let's see
crash or something else...=)

I can be a sr for this change.

Do you want someone to check this in for you?

Comment 7

[John Morrison]

If you could check that in that would be good, as I am not set up to do that.

I spoke briefly with pavlov after I had done this, and he thought, considering
the way that ::Init is set up to do the default (on Mac and Windows) that it
was OK to just bail on this method. (Linux will still fail, but it needs a 
JSContext to get it's filepicker up, so it's doomed either way). 

But I've probably misquoted pavlov. Hey, pav ... r=?

Comment 8

[Bill Law]

Making the dialog modal indeed solves the problem.  The only impact would be
that users would have to decide how to dispose of the file (open using app vs.
save-to-disk versus cancel) before they can do anything else on the page.
That's different than the prior "unknown content dialog" but probably not a
problem.  And it is certainly better than crashing.

I'll attach the patch and submit it to mscott for review/super-review.

Removing [rtm need info].

Comment 9

[Bill Law]

Attachment: Proposed fix.

Comment 10

[Bill Law]

Hey, I see John had a different fix.

I wonder if plugging that one hole will take care of everything?  I was
concerned that letting that Downloading dialog proceed with the underlying
window being closed might not have other code paths that could subsequently
crash.  I thought it crashed for me when I said Cancel?

I'll back out my fix and exercise it more thoroughly.  I think we should try:
1. Cancel
2. Save to disk (to completion)
3. Save to disk (then Cancel file pickert)
4. Save to disk (then cancel progress dialog)
5. Open using (to completion)
6. Open using (then cancel progress dialog).

Comment 11

[Bill Law]

John's patch doesn't seem to plug all the holes.  I applied it (and backed out 
mine) and got this crash.  It doesn't seem like it quite got to opening the file 
picker.

ns_if_addref(nsIChromeEventHandler * 0x02c17654) line 1101 + 15 bytes
nsDocShell::GetChromeEventHandler(nsDocShell * const 0x02f20a10, 
nsIChromeEventHandler * * 0x0012d828) line 545 + 19 bytes
GlobalWindowImpl::SetDocShell(GlobalWindowImpl * const 0x034e6ec0, nsIDocShell * 
0x02f20a10) line 422 + 48 bytes
nsDocShell::EnsureScriptEnvironment(nsDocShell * const 0x02f20a10) line 4384
nsWebShell::GetInterface(nsWebShell * const 0x02f20a34, const nsID & {...}, void 
* * 0x0012d8d8) line 327 + 19 bytes
nsGetInterface::operator()(const nsID & {...}, void * * 0x0012d8d8) line 37 + 31 
bytes
nsCOMPtr<nsIDOMWindowInternal>::assign_from_helper(const nsCOMPtr_helper & 
{...}, const nsID & {...}) line 856 + 18 bytes
nsCOMPtr<nsIDOMWindowInternal>::nsCOMPtr<nsIDOMWindowInternal>(const 
nsCOMPtr_helper & {...}) line 553
nsUnknownContentTypeHandler::PromptForSaveToFile(nsUnknownContentTypeHandler * 
const 0x03679754, nsISupports * 0x02f20a10, const unsigned short * 0x0366cc90, 
const unsigned short * 0x0012da88, nsILocalFile * * 0x0012db94) line 284 + 33 
bytes
nsExternalAppHandler::PromptForSaveToFile(nsILocalFile * * 0x0012db94, const 
unsigned short * 0x0366cc90) line 832 + 92 bytes
nsExternalAppHandler::SaveToDisk(nsExternalAppHandler * const 0x0366c804, 
nsIFile * 0x00000000, int 0x00000000) line 851 + 48 bytes
XPTC_InvokeByIndex(nsISupports * 0x0366c804, unsigned int 0x00000004, unsigned 
int 0x00000002, nsXPTCVariant * 0x0012dd38) line 139
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x0367cb90, 
nsXPCWrappedNative * 0x0367ab50, const XPCNativeMemberDescriptor * 0x0367e1d8, 
nsXPCWrappedNativeClass::CallMode CALL_METHOD, unsigned int 0x00000002, long * 
0x00da5fcc, long * 0x0012deec) line 913 + 43 bytes
WrappedNative_CallMethod(JSContext * 0x0367cb90, JSObject * 0x025ac318, unsigned 
int 0x00000002, long * 0x00da5fcc, long * 0x0012deec) line 228 + 34 bytes
js_Invoke(JSContext * 0x0367cb90, unsigned int 0x00000002, unsigned int 
0x00000000) line 820 + 23 bytes
js_Interpret(JSContext * 0x0367cb90, long * 0x0012ea20) line 2621 + 15 bytes
js_Invoke(JSContext * 0x0367cb90, unsigned int 0x00000001, unsigned int 
0x00000002) line 837 + 13 bytes
js_InternalInvoke(JSContext * 0x0367cb90, JSObject * 0x025701f0, long 
0x00d57b60, unsigned int 0x00000000, unsigned int 0x00000001, long * 0x0012ebb8, 
long * 0x0012eb48) line 909 + 20 bytes
JS_CallFunctionValue(JSContext * 0x0367cb90, JSObject * 0x025701f0, long 
0x00d57b60, unsigned int 0x00000001, long * 0x0012ebb8, long * 0x0012eb48) line 
3193 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x0367d420, void * 0x025701f0, 
void * 0x00d57b60, unsigned int 0x00000001, void * 0x0012ebb8, int * 0x0012ebb4, 
int 0x00000000) line 907 + 33 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x034e3a54) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03226660, 
nsIDOMEvent * 0x034e3a54, nsIDOMEventTarget * 0x032213f8, unsigned int 
0x00000004, unsigned int 0x00000007) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x036b4680, nsEvent * 
0x0012f498, nsIDOMEvent * * 0x0012f3b4, nsIDOMEventTarget * 0x032213f8, unsigned 
int 0x00000007, nsEventStatus * 0x0012f7b8) line 935 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x032213f0, nsIPresContext * 
0x036b4680, nsEvent * 0x0012f498, nsIDOMEvent * * 0x0012f3b4, unsigned int 
0x00000001, nsEventStatus * 0x0012f7b8) line 3321
PresShell::HandleEventInternal(nsEvent * 0x0012f498, nsIView * 0x00000000, 
unsigned int 0x00000001, nsEventStatus * 0x0012f7b8) line 4255 + 47 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x036b19f0, nsEvent * 
0x0012f498, nsIFrame * 0x025c20fc, nsIContent * 0x032213f0, unsigned int 
0x00000001, nsEventStatus * 0x0012f7b8) line 4236 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const 
0x0327f980, nsIPresContext * 0x036b4680, nsMouseEvent * 0x0012f8c8, 
nsEventStatus * 0x0012f7b8) line 1856 + 61 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x0327f988, 
nsIPresContext * 0x036b4680, nsEvent * 0x0012f8c8, nsIFrame * 0x025c20fc, 
nsEventStatus * 0x0012f7b8, nsIView * 0x036b40d0) line 937 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f8c8, nsIView * 0x036b40d0, 
unsigned int 0x00000001, nsEventStatus * 0x0012f7b8) line 4275 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x036b19f4, nsIView * 0x036b40d0, 
nsGUIEvent * 0x0012f8c8, nsEventStatus * 0x0012f7b8, int 0x00000001, int & 
0x00000001) line 4190 + 25 bytes
nsView::HandleEvent(nsView * const 0x036b40d0, nsGUIEvent * 0x0012f8c8, unsigned 
int 0x0000001c, nsEventStatus * 0x0012f7b8, int 0x00000001, int & 0x00000001) 
line 379
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x036b42b0, nsGUIEvent * 
0x0012f8c8, nsEventStatus * 0x0012f7b8) line 1439
HandleEvent(nsGUIEvent * 0x0012f8c8) line 68
nsWindow::DispatchEvent(nsWindow * const 0x036b1ee4, nsGUIEvent * 0x0012f8c8, 
nsEventStatus & nsEventStatus_eIgnore) line 681 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f8c8) line 702
nsWindow::DispatchMouseEvent(unsigned int 0x0000012d, nsPoint * 0x00000000) line 
3890 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 0x0000012d, nsPoint * 0x00000000) 
line 4100
nsWindow::ProcessMessage(unsigned int 0x00000202, unsigned int 0x00000000, long 
0x00aa0143, long * 0x0012fc44) line 2960 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x00a50348, unsigned int 0x00000202, unsigned int 
0x00000000, long 0x00aa0143) line 950 + 27 bytes
USER32! 77e713ed()
00aa0143()

Comment 12

[John Morrison]

Hmm. I'm surprised (now) that there is another crash path. I went through the
different scenarios (on win2k) and got the appropriate response from the app
for the circumstance (e.g., cancel cancelled, save saved ...).

But, it looks like I missed something, and making this dialog modal does make it
impossible for the user to get into this situation where they've absentmindedly
killed off the parent window, and that would prevent this crash. (Actually,
there is a small loophole to modality on Linux, but that's a different bug
(bug 55472)).

It may still be worthwhile to do the null-pointer check anyways, as there may
be some other way that people could exercise that code path (I couldn't tell
for sure from the comments in the Talkback reports), and the nsBaseFilePicker
is already set up to do the right thing if it can't get the nsIWidget for some
reason.

Comment 13

[Scott MacGregor]

I'd like to check in both the null ptr check and the modal dialog fix if we can
check both in.

At the very least sr=mscott for law's patch to make the dialog modal.

Although as John pointed out, on Linux you can still get around the modality
issue and close the parent window. But that's a linux issue with modal dialogs.

Adding rtm keywords to status white board. I'm hoping we can get don to rtm+
this once Bill gets another reviewer.

Comment 14

[don]

PDT folks, we have two patches to fix this problem.  Everybody seems happy it
fixes the majority of cases where this could happen by both forcing the window
to be modal and checking for the null pointer.  Please approve.

Comment 15

[don]

*** Bug 55626 has been marked as a duplicate of this bug. ***

Comment 16

[Michael La Guardia]

marking rtm++

Comment 17

[Bill Law]

Fixes checked in, trunk and branch.

Comment 18

[timeless]

I'm very unhappy about the modality fix.

I like being able to launch multiple save windows concurrently esp for servers 
that take a while before the transfer begins.  On windows there's an option of 
aborting close calls (i've seen nc4 ignore my click on the window close [x] 
button).

Relnote: ~file transfer dialogs are window modal, please abuse new (clone) 
window before attempting transfers.~

Comment 19

[John Morrison]

verified fixed, trunk and branch, mac/linux/win32 -- dialog is modal, and 
the null-pointer check is in the code. 

timeless -- it wasn't any particular love of modality that motivated this fix. 
It was more the fact that this was crashing for real users. 

However, nothing about this fix prevents multiple concurrent downloads on mac, 
linux or win32 -- after you have dealt with the initial dialog + filepicker,
a non-modal window is spun up to complete the download.

Comment 20

[John Morrison]

Given that we are now just like Nav4.x on mac,linux,win32 -- initial dialog
window is modal -- I'm removing the release note keyword.

Comment 21

[John Morrison]

(Pending a reopen of this bug due to bug 58669 ...). I just checked in a trunk
build on windows -- making that dialog non-modal means that pretty much by any
of that paths that law noted above (except a straight cancel), killing the 
parent now leads to a crash since, for some reason, the code where the null 
pointer check was added (which covered most of the crash situations) is never 
reached (something changed?). The stack is the same as the one that law posted
above. 

I agree though that it is preferable to open up the situation where someone 
kills the parent window of this dialog, than to have downloaded files 
mysteriously disappearing on a regular basis (since the former is less common 
an occurence than the latter).