321914 - Crash if a download completes while its properties window is still open

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Marco De Vitis]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; it; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; it; rv:1.8) Gecko/20051111 Firefox/1.5

I've got Firefox set up to automatically show the Download Manager when a download starts, and automatically close it when all downloads are completed.

If I start a download, then open the properties window for the currently active download (right click on it in the Download Manager, and select "Properties") and never close it, when the download completes the Download Manager window is automatically closed, but the download properties window is not, and Firefox crashes.

Reproducible: Always

Steps to Reproduce:
1. Set up Firefox to automatically show the Download Manager when a download starts, and automatically close it when all downloads are completed.

2. Be sure that the Download Manager window is closed, then start a download; the Download Manager should open automatically.

3. While the file is still downloading, right-click on its line in the Download Manager window and choose "Properties"; the download properties window will show up.

4. Wait until the download completes. Firefox should crash.

Actual Results:  
Firefox crashes.

Expected Results:  
Firefox should close the download properties window together with the Download Manager, and continue working as usual.

Talkback crash ID: TB13409971H

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Stack Signature	 0xd4069600 8dd40673
Product ID	Firefox15
Build ID	2005111116
Trigger Time	2005-12-30 07:30:08.0
Platform	MacOSX
Operating System	Darwin 8.3.0
Module	
URL visited	
User Comments	A download finished, and the download manager automatically closed (this is the setting I use), while I was still looking at the download's properties window.
Since Last Crash	2166 sec
Total Uptime	3192569 sec
Trigger Reason	SIGSEGV: Segmentation Violation: (signal 11)
Source File, Line No.	N/A
Stack Trace 	
0xd4069600
nsMacWindow::WindowEventHandler()  [mozilla/widget/src/mac/nsMacWindow.cpp, line 698]
SendEventToEventTargetInternal()
SendEventToEventTargetWithOptions()
HandleWindowEvent()
ToolboxEventDispatcherHandler()
DispatchEventToHandlers()
SendEventToEventTargetInternal()
SendEventToEventTarget()
ToolboxEventDispatcher()
TryEventDispatcher()
GetOrPeekEvent()
GetNextEventMatchingMask()
WNEInternal()
WaitNextEvent()
nsMacMessagePump::GetEvent()   nsAppShell::GetNativeEvent()  [mozilla/widget/src/mac/nsAppShell.cpp, line 211]
nsXULWindow::ShowModal()  [mozilla/xpfe/appshell/src/nsXULWindow.cpp, line 401]
nsWindowWatcher::OpenWindowJS()  [mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp, line 603]
nsGlobalWindow::OpenInternal()  [mozilla/dom/src/base/nsGlobalWindow.cpp, line 1453]
nsGlobalWindow::OpenDialog()  [mozilla/dom/src/base/nsGlobalWindow.cpp, line 4374]
_XPTC_InvokeByIndex()
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)()  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2138]
XPC_WN_CallMethod()  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1444]
js_Invoke()  [mozilla/js/src/jsinterp.c, line 1177]
js_Interpret()  [mozilla/js/src/jsinterp.c, line 3526]
js_Invoke()  [mozilla/js/src/jsinterp.c, line 1197]
nsXPCWrappedJSClass::CallMethod()  [mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1371]
PrepareAndDispatch()  [mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_ppc_rhapsody.cpp, line 184]
SharedStub()
nsEventListenerManager::HandleEventSubType()  [mozilla/content/events/src/nsEventListenerManager.cpp, line 848]
nsEventListenerManager::HandleEvent()  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1766]
nsXULElement::HandleDOMEvent()  [mozilla/content/xul/content/src/nsXULElement.cpp, line 2153]
nsXULElement::HandleDOMEvent()  [mozilla/content/xul/content/src/nsXULElement.cpp, line 144]
nsXULElement::HandleDOMEvent()  [mozilla/content/xul/content/src/nsXULElement.cpp, line 144]
nsXULElement::HandleDOMEvent()  [mozilla/content/xul/content/src/nsXULElement.cpp, line 144]
nsEventStateManager::DispatchNewEvent()  [mozilla/content/events/src/nsEventStateManager.cpp, line 4566]
nsEventListenerManager::DispatchEvent()  [mozilla/content/events/src/nsEventListenerManager.cpp, line 419]
nsDOMEventRTTearoff::DispatchEvent()   _XPTC_InvokeByIndex()
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)()  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2138]
XPC_WN_CallMethod()  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1444]
js_Invoke()  [mozilla/js/src/jsinterp.c, line 1177]
js_Interpret()  [mozilla/js/src/jsinterp.c, line 3526]
js_Invoke()  [mozilla/js/src/jsinterp.c, line 1197]
js_InternalInvoke()  [mozilla/js/src/jsinterp.c, line 1275]
JS_CallFunctionValue()  [mozilla/js/src/jsapi.c, line 4159]
nsJSContext::CallEventHandler()  [mozilla/dom/src/base/nsJSEnvironment.cpp, line 1413]
nsJSEventListener::HandleEvent()
nsEventListenerManager::HandleEventSubType()  [mozilla/content/events/src/nsEventListenerManager.cpp, line 848]
nsEventListenerManager::HandleEvent()  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1766]
nsXULElement::HandleDOMEvent()  [mozilla/content/xul/content/src/nsXULElement.cpp, line 2153]
PresShell::HandleDOMEventWithTarget()
nsMenuFrame::Execute()  [mozilla/layout/xul/base/src/nsMenuFrame.cpp, line 848]
nsMenuFrame::HandleEvent()  [mozilla/layout/xul/base/src/nsMenuFrame.cpp, line 454]
PresShell::HandleEventInternal()
PresShell::HandleEvent()
nsViewManager::HandleEvent()  [mozilla/view/src/nsViewManager.cpp, line 61]
nsViewManager::DispatchEvent()  [mozilla/view/src/nsViewManager.cpp, line 2246]
HandleEvent()  [mozilla/view/src/nsView.cpp, line 175]
nsWindow::DispatchEvent()  [mozilla/widget/src/mac/nsWindow.cpp, line 1809]
nsWindow::DispatchWindowEvent()  [mozilla/widget/src/mac/nsWindow.cpp, line 1825]
nsWindow::DispatchMouseEvent()  [mozilla/widget/src/mac/nsWindow.cpp, line 1851]
nsMacEventHandler::HandleMouseUpEvent()
nsMacEventHandler::HandleOSEvent()
nsMacWindow::DispatchEvent()  [mozilla/widget/src/mac/nsMacWindow.cpp, line 1730]
nsMacMessagePump::DispatchOSEventToRaptor()
nsMacMessagePump::DoMouseUp()
nsMacMessagePump::DispatchEvent()
nsMacMessagePump::DoMessagePump()
nsAppShell::Run()  [mozilla/widget/src/mac/nsAppShell.cpp, line 114]
nsAppStartup::Run()
XRE_main()  [mozilla/toolkit/xre/nsAppRunner.cpp, line 2315]
_start()
start()

Comment 2

[Josh Aas]

Attachment: fix v1.0

This patch really fixes another problem. When a user starts interacting with the downloads window, it should no longer autoclose. A side effect of this fix is that this bug is done away with.

We should probably open a bug on the core cause of this, which is probably a widgets issue.

Comment 3

[Mark Mentovai]

Comment on attachment 207214 [details] [diff] [review]
fix v1.0

You need to restore gCanAutoClose when the properties sheet/dialog closes.

Comment 4

[Josh Aas]

I don't want to restore the flag, but I did set the wrong flag. New patch coming up.

Comment 5

[Mark Mentovai]

Comment on attachment 207214 [details] [diff] [review]
fix v1.0

OK, I thought that making the window not close after opening a download's properties was an unintended result of this patch, but since it's intended and desired, the behavior is fine with me.

Comment 6

[Josh Aas]

Attachment: fix v2.0

This is a better way to do it. Right now, the dl manager does not take into account whether or not a user has interacted with the window in a significant way. So, I added a variable to keep track of user interaction and if that variable is true, then the dl manager does not auto-close. This fixes this particular bug at a higher level, and improves the UI a bit.

There are tougher UI decisions to make about what should be considered significant user interaction, but this is not the place for that discussion and I don't want to include anything further in my patch on that subject. I think we can all agree that opening the properties dialog is significant enough interaction to stop auto-closing.

Comment 7

[Josh Aas]

Note: my "fix v1.0" does almost exactly the same thing as "fix v2.0." The difference is that |gCanAutoClose| is used for xpinstall stuff, and is set in 2 places. In theory one of those assignments could overwrite a |true| value that got assigned as a result of user interaction. I don't know if that is likely to happen, but "fix v2.0" is a much more clear way of handling the situation, and avoids the theoretical problems.

Comment 8

[Josh Aas]

"fix v2.0" landed on trunk.

Comment 9

[Daniel Veditz [:dveditz]]

beltzner: this changes UI behavior to sidestep the crash. Is this behavior change  OK?

This has been checked into the trunk, we'd like to see more baking time.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #2)
> This patch really fixes another problem. When a user starts interacting with
> the downloads window, it should no longer autoclose. A side effect of this fix
> is that this bug is done away with.

Really? I'm not sure I agree with that. The option clearly states "when downloads are finished", not "if I don't touch it". I think the reporter is actually correct in stating that the expected behaviour would be that any attached sheets would also close.

> We should probably open a bug on the core cause of this, which is probably a
> widgets issue.

So is this the only way to work around it until that time?

Comment 11

[Josh Aas]

Beltzner - the idea behind autoclose is to keep the download manager out of your way when you're just watching something complete. But if you're doing more than that, actually interacting with it, automatically closing can be disruptive. The author of the autoclose code even noted in the code that he/she thought we may want to do this. I don't think being absolute about the wording of the option here is in the user's best interest.

If you saw the download manager and clicked on the properties dialog, were reading that or editing something in it, would you really want the download manager to close (possibly with your properties window) from under you?

Ben? Mconnor?

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #9)
> beltzner: this changes UI behavior to sidestep the crash. Is this behavior
> change  OK?

Yeah, definitely OK for a stability release. ui-approval = me for 1.8.0.x release.

(In reply to comment #11)
> thought we may want to do this. I don't think being absolute about the wording
> of the option here is in the user's best interest.

That's not really what I was trying to do. I was trying to interpret what a user might be expecting based on that option being clicked.

> If you saw the download manager and clicked on the properties dialog, were
> reading that or editing something in it, would you really want the download
> manager to close (possibly with your properties window) from under you?

So, you can't edit in that properties dialog, you can only inspect & copy. And on Mac, that properties window is an attached sheet (is that what's causing the problem in the first place?). At that point, if I'd clicked "get rid of this dialog when things are done downloading", then yes, I'd expect it to go away as it's a read only display which has an inspector for more details.

Comment 13

[Mike Connor [:mconnor]]

If you're looking at the properties of Download A while Download B is chugging away, I don't think its at all expected behaviour for that window to close while you're looking at it just because Download B is finished.  The primary use-case for autoclose is "User wants to see the download only while it is in progress" but as soon as you're interacting with completed downloads, you're shifting into a different use-case (user wants to get information about previous downloads) and auto-close shouldn't apply, just as it doesn't when you manually open the download manager.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

I guess I'm thinking of the properies window (which is an edge, edge case - c'mon, how many people really look at that thing?) as an "inspector". If we'd built the window like this:

 .---------------------------------------------.
 |*someFile*                          _Open_   |
 |                                    _Remove_ |
 |                                    _Details_|
 |---------------------------------------------|
 | someOtherFile                      _Open_   |
 |                                    _Remove_ |
 | from: http://www.foo.com/                   |
 | to  : ~/fooplace                            |
 | completed on Feb 14th, 2009   _Hide Details_|
 |---------------------------------------------|
 |                                             |
 :                                             :

then I don't think we'd be having this discussion. From a user task basis, that's pretty much all the properties window really lets you do.
 

Comment 15

[Josh Aas]

landed on MOZILLA_1_8_BRANCH

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 207594 [details] [diff] [review]
fix v2.0

approved for 180 branch, a=dveditz for drivers

Comment 17

[Josh Aas]

checked in on MOZILLA_1_8_0_BRANCH

Comment 18

[Dave Liebreich [:davel]]

Is this a 10.2-only bug, or can we verify it on 10.3/10.4?

Comment 19

[Josh Aas]

This happens on all versions of Mac OS X.

Comment 20

[Tracy Walker [:tracy]]

verified fixed and behaving as in comment #6 (not closing upon completion because the properties dialog was open)

Mac 2006-02-27-05-mozilla1.8.0