Get two different "Are you sure you want to quit?" dialogs when trying to quit during download

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Håkan Waara, Assigned: Håkan Waara)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

10 years ago
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.
I'm not sure if the download manager is the right component for this, but this should totally block.
Flags: blocking-firefox3?
Also related to (actually a DUP of, I think?) bug 398010.
(Assignee)

Comment 3

10 years ago
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?
Assignee: nobody → hwaara
Status: NEW → ASSIGNED
Attachment #289568 - Flags: review?
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 398010
Flags: in-litmus?
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.
Flags: in-litmus? → in-litmus+
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?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
(Assignee)

Comment 8

10 years ago
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.
Attachment #289568 - Flags: review? → review?(gavin.sharp)

Updated

10 years ago
Assignee: hwaara → nobody
Status: ASSIGNED → NEW
Component: Download Manager → General
QA Contact: download.manager → general
Whiteboard: [has patch][needs review gavin]
(Assignee)

Comment 9

10 years ago
Shawn, why was this reassigned to nobody?
Probably happened when I moved components.  Sorry about that!
Assignee: nobody → hwaara
The flow I've suggested in this other related bug addresses this issues:

https://bugzilla.mozilla.org/show_bug.cgi?id=401172#c10
(Assignee)

Comment 12

10 years ago
(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 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).
Attachment #289568 - Flags: review?(gavin.sharp) → review+

Updated

10 years ago
Priority: P2 → P1
(Assignee)

Comment 14

10 years ago
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.
Whiteboard: [has patch][needs review gavin] → [has patch][has review]
Håkan - do you plan to land this?
(Assignee)

Comment 16

10 years ago
(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).
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!
(Assignee)

Comment 18

10 years ago
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.
Attachment #296235 - Flags: review?(gavin.sharp)
Why is the QI only necessary in the browserglue code? Wouldn't the other JavaScript code you're adding this to also require it?
(Assignee)

Comment 20

10 years ago
(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.
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)
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?
(Assignee)

Comment 23

10 years ago
(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...
Attachment #296235 - Attachment is obsolete: true
Attachment #296235 - Flags: review?(gavin.sharp)
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: [has patch][has review] → [needs new patch]
Blocks: 401172
(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!
(Assignee)

Comment 25

10 years ago
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.
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
Created attachment 299055 [details] [diff] [review]
WIP

Previous WIP was slightly bitrotted, as my tree was a bit out-of-date
Attachment #298813 - Attachment is obsolete: true
when do you think you might be able to get the new patch up for this?
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.
Depends on: 414274
Duplicate of this bug: 414313
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.


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.
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)
Attachment #296325 - Attachment is obsolete: true
Attachment #299055 - Attachment is obsolete: true
Attachment #300011 - Flags: review?(gavin.sharp)
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!
Attachment #300011 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Whiteboard: [needs new patch]
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+
Attachment #300011 - Attachment is obsolete: true
Attachment #300101 - Flags: review+
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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta3
Duplicate of this bug: 414756
Duplicate of this bug: 396579
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008013004 Minefield/3.0b3pre ID:2008013004
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.