Last Comment Bug 404572 - Get two different "Are you sure you want to quit?" dialogs when trying to quit during download
: Get two different "Are you sure you want to quit?" dialogs when trying to qui...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 3 beta3
Assigned To: Håkan Waara
:
Mentors:
: 396579 398010 414313 414756 (view as bug list)
Depends on: 414274
Blocks: 401172
  Show dependency treegraph
 
Reported: 2007-11-20 09:31 PST by Håkan Waara
Modified: 2008-01-30 08:23 PST (History)
18 users (show)
mbeltzner: blocking‑firefox3+
stephen.donner: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for review (6.21 KB, patch)
2007-11-20 15:21 PST, Håkan Waara
gavin.sharp: review+
Details | Diff | Review
Correct Patch for nsBrowserGlue.js (1.29 KB, patch)
2008-01-09 15:28 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Debug Patch (5.30 KB, patch)
2008-01-10 07:28 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
WIP (3.83 KB, patch)
2008-01-23 16:38 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
WIP (3.72 KB, patch)
2008-01-24 16:03 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Patch for review (3.93 KB, patch)
2008-01-29 06:57 PST, Graeme McCutcheon [:graememcc]
gavin.sharp: review+
Details | Diff | Review
Patch for checkin (3.90 KB, patch)
2008-01-29 11:59 PST, Graeme McCutcheon [:graememcc]
graeme.mccutcheon: review+
Details | Diff | Review

Description Håkan Waara 2007-11-20 09:31:14 PST
Firefox trunk from 20071118, Mac OS X

STR:

1. Start a download
2. Try to quit
3. You get a "A download is in progress, are you sure you want to quit?"
4. Choose "Don't quit"

Actual result:

You get an additional "Do you want to save your tabs before quitting?"-dialog

Expected result:

The quit action is already cancelled, so the extra quit confirmation is not needed.
Comment 1 Shawn Wilsher :sdwilsh 2007-11-20 12:09:51 PST
I'm not sure if the download manager is the right component for this, but this should totally block.
Comment 2 Stephen Donner [:stephend] 2007-11-20 12:42:56 PST
Also related to (actually a DUP of, I think?) bug 398010.
Comment 3 Håkan Waara 2007-11-20 15:21:38 PST
Created attachment 289568 [details] [diff] [review]
Patch for review

It turns out that all the observers of the quit event may collide. Thus the bug with multiple dialogs will occur if any of these are true at once:

* The extension manager is downloading a new extension
* The download manager is downloading something
* We have >1 window open
* We have >1 tab open

There already exists a kind of "protocol" that if you set the observer subject.data argument to true, then the quit request will be cancelled. This is what happens when you click "Don't Quit" in any of these dialogs.

The fix is to make all these quit observers check the subject.data *before* issuing a new dialog.

Attaching a patch for review (-w for clarity; I've had to reindent a lot of code due to added if-statements). Not sure who to ask for review?
Comment 4 Håkan Waara 2007-11-20 15:23:17 PST
Also, I haven't done complete testing of the patch yet. Only the session restore vs download case has been tested yet. If the approach is OK, I will do complete testing this week.
Comment 5 Håkan Waara 2007-11-20 15:24:32 PST
*** Bug 398010 has been marked as a duplicate of this bug. ***
Comment 6 Stephen Donner [:stephend] 2007-11-20 16:11:29 PST
in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=4999 ("Don't Quit" choice)
https://litmus.mozilla.org/show_test.cgi?id=5000 ("Quit" choice)

Probably a spin-off bug, but we should also think about how this interacts with currently open tabs; normally we get the "Save and Quit" choice there.
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-21 08:17:49 PST
I was pretty sure that this was a dupe, too, but yes, it blocks.

Madhava: make this part of your overall review of "what Firefox does in various different states when the user asks to quit" actions?
Comment 8 Håkan Waara 2007-11-21 08:41:44 PST
Comment on attachment 289568 [details] [diff] [review]
Patch for review

Who can review my patch? It touches toolkit and browser, so I'll randomly pick Gavin.

Gavin, if you know someone better, feel free to change the request.
Comment 9 Håkan Waara 2007-11-25 10:58:44 PST
Shawn, why was this reassigned to nobody?
Comment 10 Shawn Wilsher :sdwilsh 2007-11-25 14:22:43 PST
Probably happened when I moved components.  Sorry about that!
Comment 11 Madhava Enros [:madhava] 2007-11-26 13:11:04 PST
The flow I've suggested in this other related bug addresses this issues:

https://bugzilla.mozilla.org/show_bug.cgi?id=401172#c10
Comment 12 Håkan Waara 2007-11-26 13:48:07 PST
(In reply to comment #11)
> The flow I've suggested in this other related bug addresses this issues:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=401172#c10

Madhava, there are other shutdown observers that bring up dialogs when you quit, for example the extension manager and update manager (IIRC), and then there is both the "quit with many tabs" and "quit with many windows" cases that I haven't tested but that may also conflict.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-10 16:20:12 PST
Comment on attachment 289568 [details] [diff] [review]
Patch for review

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

> nsDownloadManager::ConfirmCancelDownloads(PRInt32 aCount,

>+  PRBool quitRequestCancelled = PR_FALSE;
>+  aCancelDownloads->GetData(&quitRequestCancelled);
>+  
>+  if (!quitRequestCancelled) {

Return early rather than indenting the entire method?

>Index: browser/components/sessionstore/src/nsSessionStore.js

>     case "quit-application-requested":
>+      // data is true if another observer already cancelled the quit request.
>+      if (!aSubject.data) {

Given that this observer doesn't do anything user-visible, and is just saving state, perhaps it should run either way? It's session store, so probably best to err on the side of caution...

I haven't tested this, I'm assuming you have (make sure each of the observers still work, etc).
Comment 14 Håkan Waara 2007-12-13 15:23:52 PST
Thanks for the review, Gavin. Good catch on the session store thing.

I've been testing and all cases are covered except that sometimes I can get the "Do you want to cancel all downloads?" and "Do you want to save the curren tabs?" dialogs to come up both. This is only reproducible in one of my two profiles though, so I'm not sure what the bug is yet.
Comment 15 Shawn Wilsher :sdwilsh 2007-12-30 17:52:45 PST
Håkan - do you plan to land this?
Comment 16 Håkan Waara 2007-12-30 17:55:13 PST
(In reply to comment #15)
> Håkan - do you plan to land this?
> 

There's still a problem if you quit with multiple tabs open + a download going; clicking "Don't Quit" in the download dialog still triggers the "Save all tabs and quit?"-dialog, even though the quit should've been dismissed.

Feel free to investigate why, I have but didn't find the bug (yet).
Comment 17 Graeme McCutcheon [:graememcc] 2008-01-09 15:28:57 PST
Created attachment 296235 [details] [diff] [review]
Correct Patch for nsBrowserGlue.js

Håkan

The bug was in nsBrowserGlue. At the point where you are testing subject.data, subject is a xpconnect wrapped nsISupports object, and not a PRBool. Thus, the data property of subject is undefined, and !undefined always evaluates to true, causing the tabs dialog to always display.

The above QueryInterfaces subject to get the PRBool, and then inspects the data property.

This should now let you land!
Comment 18 Håkan Waara 2008-01-10 00:09:50 PST
Comment on attachment 296235 [details] [diff] [review]
Correct Patch for nsBrowserGlue.js

graememcc, thanks!!

Gavin, can you review this supplementary fix to my patch?  I will incorporate it, test and finally land it.
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-10 00:13:17 PST
Why is the QI only necessary in the browserglue code? Wouldn't the other JavaScript code you're adding this to also require it?
Comment 20 Håkan Waara 2008-01-10 00:16:10 PST
(In reply to comment #19)
> Why is the QI only necessary in the browserglue code? Wouldn't the other
> JavaScript code you're adding this to also require it?
> 

Good question. The other cases work from testing, so I don't really know why, but I'd like to know as well.
Comment 21 Graeme McCutcheon [:graememcc] 2008-01-10 07:28:57 PST
Created attachment 296325 [details] [diff] [review]
Debug Patch

I think in fact they do need to QI as well.

The patch I've attached just adds dumps and printfs at the appropriate points, so you can run your build with -console to see what's happening. In all cases where I ran it, each js file was seeing subject as a xpconnect wrapped nsISupports object, and subject.data was undefined.

On further testing, I noticed the extension manager downloading/multiple tabs case does not suppress the multiple tabs dialog, but things like extension manager downloading/download manager downloading seem to work.

The things that I think made the testing seem to work:
1) Extension manager seems to get the notification first, so subject.data has never been altered when it gets the notification. Obviously, this should not be relied on, so the test should still happen.


2) The line at http://mxr.mozilla.org/firefox/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#5342

The instanceof operator will QI automatically ("The instanceof operator returns true if aFile implements the nsILocalFile interface. This also has the side effect of calling QueryInterface, so aFile will be a valid nsILocalFile afterwards" - http://www.xulplanet.com/tutorials/xultu/xpcom.html)

So, although the test of subject.data in extension manager was wrong, when the dialog gets clicked, subject.data gets set correctly, allowing Download Manager etc to correctly see the change.

3) In some cases, what was perhaps happening was that in the js, a data property might have  been added to the nsISupports object, causing future "if (subject.data)" tests to behave correctly.

Thus, I think what should happen is that at each case first do the test:
if (subject instanceof Components.interfaces.nsISupportsPRBool)

and then inspect subject.data to conditionally perform quit-request code. (In nsExtensionManager.js.in the line linked to above should also be removed: if you do the instanceof test before _ConfirmCancelDownloadsOnQuit is called, the subject parameter will already be a PRBool, making that line redundant)
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-10 10:44:41 PST
Ah, yeah, the DM QI could be exposing nsISupportsPRBool for the other callers. In any case, we should add the instanceof/QI to all users.

Håkan, can you attach a roll-up patch that includes those changes?
Comment 23 Håkan Waara 2008-01-11 05:58:50 PST
(In reply to comment #22)
> Ah, yeah, the DM QI could be exposing nsISupportsPRBool for the other callers.
> In any case, we should add the instanceof/QI to all users.
> 
> Håkan, can you attach a roll-up patch that includes those changes?
> 

In the process of testing the instanceof-solution I noticed that clicking "Don't Quit" in middle of installing an extensions seems to quit Firefox anyway. Not sure if this is an actual bug on trunk, or a consequence of my patch, but a new patch is underway...
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2008-01-22 09:56:23 PST
(In reply to comment #22)
> Håkan, can you attach a roll-up patch that includes those changes?

Any progress here? This is a P1 blocker which will be due next week, with a string change which is due Real Soon Now, so if you need help here, please feel free to give a shout!
Comment 25 Håkan Waara 2008-01-23 15:48:51 PST
Can anyone reproduce the bug I saw in comment 23? (Please try OS X, just in case it would be OS specific)

I will be away for 1.5 weeks from now. Graeme, wanna drive this patch? Otherwise, anyone else: feel free to pick it up from here. I'm sorry I haven't had more time for it.
Comment 26 Graeme McCutcheon [:graememcc] 2008-01-23 16:38:01 PST
Created attachment 298813 [details] [diff] [review]
WIP

Håkan,

Will give it a go. Will spend some time tomorrow trying to reproduce what you saw.

The attached is Håkan's patch, with Gavin's comment on sessionStore rolled in.

There are 16 testcases that need to be checked.

(In the following EM=extension manager downloading, DM=download manager downloading, BG=multiple tabs open)

1)  EM+DM, cancel on EM's prompt - should get no further prompts, browser does not quit
2)  EM+DM, quit on EM's prompt, cancel on DM's prompt - should get no further prompts, browser does not quit
3)  EM+DM, quit on EM's prompt, quit on DM's prompt - browser should quit
4)  EM+BG, cancel on EM's prompt - should get no further prompts, browser does not quit
5)  EM+BG, quit on EM's prompt, cancel on BG's prompt - should get no further prompts, browser does not quit
6)  EM+BG, quit on EM's prompt, quit on BG's prompt - browser should quit
7)  EM+BG, quit on EM's prompt, save+quit on BG's prompt - browser should quit, tabs restored next time
8)  DM+BG, cancel on DM's prompt - should get no further prompts, browser does not quit
9)  DM+BG, quit on DM's prompt, cancel on BG's prompt - should get no further prompts, browser does not quit
10) DM+BG, quit on DM's prompt, quit on BG's prompt - browser should quit
11) DM+BG, quit on DM's prompt, save+quit on BG's prompt - browser should quit, tabs restored next time
12) EM+DM+BG, cancel on EM's prompt - should get no further prompts, browser does not qui1
13) EM+DM+BG, quit on EM's prompt, cancel on DM's prompt - should get no further prompts, browser does not quit
14) EM+DM+BG, quit on EM's prompt, quit on DM's prompt, cancel on BG's prompt - should get no further prompts, browser does not quit
15) EM+DM+BG, quit on EM's prompt, quit on DM's prompt, quit on BG's prompt - browser should quit
16) EM+DM+BG, quit on EM's prompt, quit on DM's prompt, save+quit on BG's prompt - browser should quit, tabs restored next time

I haven't run in to the problem Håkan describes in comment 23. On a couple of instances though, I don't think Extension manager got the quit-application-requested notification (had dump() statements in there to see who was getting what notifications), but I put that down to my profile getting hosed by constant aborting/uninstalling of extensions, but should be investigated just in case.

As Håkan said, if anyone can give this a workout, would be appreciated
Comment 27 Graeme McCutcheon [:graememcc] 2008-01-24 16:03:01 PST
Created attachment 299055 [details] [diff] [review]
WIP

Previous WIP was slightly bitrotted, as my tree was a bit out-of-date
Comment 28 Shawn Wilsher :sdwilsh 2008-01-25 10:25:41 PST
when do you think you might be able to get the new patch up for this?
Comment 29 Graeme McCutcheon [:graememcc] 2008-01-27 15:59:36 PST
I threw together some large extensions (actually very simple extensions, but with a few jpegs shoved in them to make them larger) to allow me to effectively test - most addons on a.m.o are too small, and finish downloading before I can quit the browser!

However, I started experiencing some weirdness.

This has me puzzled:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.271#5403
> this._downloadCount = 0;

Don't know enough about extension manager code to understand why this is needed, but I think this is what has been throwing me off testing the WIP above.

It seems that because of this, if the user cancels a quit request, the extension manager's quit warning will never be displayed again until another extension download is started. 

In addition, after cancelling a quit request, the extension manager UI instantly shows the extension as not compatible. 

However, my faux extensions were sufficiently large, and there's no way the download had completed for extension manager to make this determination.

This bustage in extension mgr is not caused by my code, (reproducible in Beta 2, and 2.0.0.11).

While I'm pretty sure the patch works, this is preventing me from testing it effectively, as I want to be sure that no downloads etc are getting cancelled when they shouldn't be.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-28 02:55:11 PST
*** Bug 414313 has been marked as a duplicate of this bug. ***
Comment 31 Graeme McCutcheon [:graememcc] 2008-01-28 08:43:53 PST
Update on comment# 29:

>It seems that because of this, if the user cancels a quit request, the
>extension manager's quit warning will never be displayed again until another
>extension download is started. 

Correct.

>In addition, after cancelling a quit request, the extension manager UI
>instantly shows the extension as not compatible. 

Wrong. Realised this always happens independent of any quit requests. See Bug 414271 comment 1

>While I'm pretty sure the patch works, this is preventing me from testing it
>effectively...

Still true, however. I want to be sure the code added does not incorrectly suppress any quit warnings. In the case of extension downloads, I cannot test this, as the extension manager itself is busy incorrectly suppressing these warnings.


Comment 32 Graeme McCutcheon [:graememcc] 2008-01-28 08:46:29 PST
Today's not going to be my day, I can tell.

> See Bug 414271 comment 1

For more relevance, see bug 414274 comment 1.

Apologies for bugspam.
Comment 33 Graeme McCutcheon [:graememcc] 2008-01-29 06:57:30 PST
Created attachment 300011 [details] [diff] [review]
Patch for review

OK, I simply commented out the line in comment out the suppression of quit warnings, to allow me to test. That worked OK, but is not included in this patch, as 414274 should handle this.

So the above, which is essentially the WIP patch earlier, which is pretty much Håkan's patch with instanceof's added works in all cases mentioned in comment 26.

(Note to anyone testing this: due to bug 414161 landing, you now need to set browser.download.manager.quitBehavior to 2 in about:config in order to see the warning dialog)
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-29 11:45:04 PST
Comment on attachment 300011 [details] [diff] [review]
Patch for review

Please omit any brackets around single-line then clauses, to match file style (applies to all three files). r=me with that, and thanks a lot for all the work you did here, Graeme!
Comment 35 Graeme McCutcheon [:graememcc] 2008-01-29 11:59:13 PST
Created attachment 300101 [details] [diff] [review]
Patch for checkin

> and thanks a lot for all the work you did here, Graeme!

My bit was easy. Thanks, Håkan, for doing the hard work, and for letting me drive home my first P1!!

Carrying forward r=gavin, a=blocking-firefox3+
Comment 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-29 13:17:16 PST
Landed on the trunk for b3:
mozilla/browser/components/nsBrowserGlue.js 	1.50
mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 	1.159
mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in 	1.274

Thanks again, Håkan and Graeme.
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-30 01:07:09 PST
*** Bug 414756 has been marked as a duplicate of this bug. ***
Comment 38 Graeme McCutcheon [:graememcc] 2008-01-30 03:38:52 PST
*** Bug 396579 has been marked as a duplicate of this bug. ***
Comment 39 Henrik Skupin (:whimboo) 2008-01-30 08:23:39 PST
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008013004 Minefield/3.0b3pre ID:2008013004

Note You need to log in before you can comment on or make changes to this bug.