Closed
Bug 55627
Opened 24 years ago
Closed 24 years ago
Can crash when ab-using the 'helperAppLauncher.xul' dialog
Categories
(SeaMonkey :: UI Design, defect, P1)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: jrgmorrison, Assigned: law)
References
()
Details
(Keywords: arch, crash, Whiteboard: [rtm++] Fix in hand, reviewed and approved)
Attachments
(1 file)
1.02 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•24 years ago
|
||
Nom. for rtm (unless the simpler fix, make it modal is done: bug 55626). (law is hating me right now ...)
Keywords: rtm
Bill, tell me there's an easy way to fix this ...
Priority: P3 → P1
Whiteboard: [rtm need info]
Comment 3•24 years ago
|
||
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....
Reporter | ||
Comment 4•24 years ago
|
||
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).
Reporter | ||
Comment 5•24 years ago
|
||
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•24 years ago
|
||
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?
Reporter | ||
Comment 7•24 years ago
|
||
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=?
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].
Status: NEW → ASSIGNED
Whiteboard: [rtm need info]
Assignee | ||
Comment 10•24 years ago
|
||
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).
Assignee | ||
Comment 11•24 years ago
|
||
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()
Reporter | ||
Comment 12•24 years ago
|
||
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•24 years ago
|
||
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.
Whiteboard: [rtm need info]
Comment 14•24 years ago
|
||
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.
Whiteboard: [rtm need info] → [rtm+] Fix in hand, reviewed and approved
Comment 15•24 years ago
|
||
*** Bug 55626 has been marked as a duplicate of this bug. ***
Comment 16•24 years ago
|
||
marking rtm++
Whiteboard: [rtm+] Fix in hand, reviewed and approved → [rtm++] Fix in hand, reviewed and approved
Assignee | ||
Comment 17•24 years ago
|
||
Fixes checked in, trunk and branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 18•24 years ago
|
||
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.~
Keywords: arch,
relnoteRTM
Reporter | ||
Comment 19•24 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 20•24 years ago
|
||
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.
Keywords: relnoteRTM
Reporter | ||
Comment 21•24 years ago
|
||
(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).
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
You need to log in
before you can comment on or make changes to this bug.
Description
•