315697 - Program lock out if a download completes while "are you sure you want to quit" sheet is up, and pref set to close download window

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Ed Gough]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Camino/1.0b1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Camino/1.0b1

Program will lock out and not quit if a download completes while I am quitting.

I.E. The cancel quit/end download window appears, download completes, it disappears and Camino is locked up.

Reproducible: Always

Steps to Reproduce:
1.Download a file
2.Whilst downloading attempt to quit
3.Don't touch any requests to cancel, end, quit etc.
4.Wait for download to complete
Actual Results:  
Camino locks up

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Hmm, WFM 2005110708 (v1.0b1).  When the download completes, I'm able to move windows around, etc., and then click the "Cancel" button and clear the sheet.

Comment 2

[Ed Gough]

(In reply to comment #1)
> Hmm, WFM 2005110708 (v1.0b1).  When the download completes, I'm able to move
> windows around, etc., and then click the "Cancel" button and clear the sheet.
> 

I have "When Downloads finish", "Close Download Window" Activated in my preferences.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Indeed, that's the key.  Then the DL Manager disappears with the sheet still focused, and nothing else works.  Another locked state bug, fun!

Comment 4

[Nick Kreeger]

Confirming, what happens when open file is selected as a pref as well?

Comment 5

[Ed Gough]

(In reply to comment #4)
> Confirming, what happens when open file is selected as a pref as well?
> 

It hangs still.

Comment 6

[Nick Kreeger]

Thanks, i'll take

Comment 7

[Nick Kreeger]

Attachment: Proposed patch

This fixes the crash, I just added a boolean flag to help break the conditional if in |didEndDownload:|. The flag turns itself on when the confirm quit modal runs, and sets itself off when the dialog finishes.

To the Ed: Nice Catch on this bug!

Comment 8

[Nick Kreeger]

Attachment: Fixed comments

Comment 9

[Mark Mentovai]

Comment on attachment 202541 [details] [diff] [review]
Fixed comments

If the user opts not to quit, shouldn't the download window close after the modal dialog is dismissed?

Comment 10

[Nick Kreeger]

Attachment: Updated to mento's comments

Updated so that if a download completes and the user clicks continue and their pref enables the download window to close, the download window will close after they clicked cancel if a download ended in the time period that the confirm terminate modul was up.

Comment 11

[Mark Mentovai]

Comment on attachment 202735 [details] [diff] [review]
Updated to mento's comments

>+  BOOL                  mAwaitingToCloseWindow;     // true when a download completes when termination log is up

Termination what?

Call it mWaitingToCloseWindow.  Better yet, mShouldCloseWindow.

>+-(void)closeWindowByPref;

I'm glad you moved this into its own method, but I don't really like the name.  Maybe closeWindowAccordingToPref?

> -(void)didEndDownload:(id <CHDownloadProgressDisplay>)progressDisplay withSuccess:(BOOL)completedOK statusCode:(nsresult)status
>-{
>+{  

Nit, nuke extra whitespace after your brace, there shouldn't be a diff on this line.

>+  if (completedOK && !mAwaitingTermination)
>   {
>+    [self closeWindowByPref];
>   }
>+  else
>+  {
>+    mAwaitingToCloseWindow = YES;
>+  }

That's faulty logic, you'll set mAwaitingToCloseWindow if !completedOK.  You want:

if (completedOK) {
  if (!mAwaitingTermination)
    [self closeWindowByPref];
  else
    mAwaitingToCloseWindow = YES;
}
else if (NS_FAILED(status) && status != NS_BINDING_ABORTED) ...

>+-(void)closeWindowByPref
>+{
>+  BOOL gotPref;
>+  BOOL keepDownloadsOpen = [[PreferenceManager sharedInstance] getBooleanPref:"browser.download.progressDnldDialog.keepAlive" withSuccess:&gotPref];

We're not using gotPref, but we probably should be.  Not your fault, it's in the existing code, but let's take this opportunity to fix it.

Comment 12

[Nick Kreeger]

Attachment: More mento updates

Updated via mento's comments

Comment 13

[Nick Kreeger]

Attachment: Updated to match the changes in bug 307753

Updated to include the changes made from bug 307753's checkin. I also fixed a comment in |initWithDictionary:| in ProgressViewController.h.

Comment 14

[Mark Mentovai]

Comment on attachment 203027 [details] [diff] [review]
Updated to match the changes in bug 307753

>+  BOOL                  mShouldCloseWindow;

Put the comment back in.

>+    // these 2 bool's helps us from locking when the termination modal is running
>+    // and a download completes

Get rid of the apostrophe, and add a dose of subject-verb agreement.

>+      // if a request to close was set when the modal was up, 
>+      // checck if the users pref enables the download window to close

Edit this comment, too.

>-    // since we are creating a progressview from a dict, the file
>-    // will not be restarted, so setup file system notifications here

Huh?

Code looks OK, I haven't tested.

Comment 15

[Nick Kreeger]

Attachment: Update

Fixed comments, if you read my comments from the last patch posted, the comments in ProgressViewController.h are no longer valid, they weren't taken out when smfr landed the patch for bug 307753.

Comment 16

[Nick Kreeger]

Attachment: Comment fixes

Comment 17

[Nick Kreeger]

FYI. Just tested this patch with the most current source and it patches and works perfect.

Comment 18

[Mark Mentovai]

Waiting for review?

Comment 19

[Nick Kreeger]

Yes can someone please take a look at this patch?

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 203034 [details] [diff] [review]
Comment fixes

>Index: ProgressDlgController.mm
>===================================================================

>+-(void)closeWindowAccordingToPref
>+{
>+  BOOL gotPref;
>+  BOOL keepDownloadsOpen = [[PreferenceManager sharedInstance] getBooleanPref:"browser.download.progressDnldDialog.keepAlive" withSuccess:&gotPref];
>+  if (gotPref && !keepDownloadsOpen && ([self numDownloadsInProgress] == 0))
>+  {
>+    // don't call -performClose: on the window, because we don't want Cocoa to look
>+    // for the option key and try to close all windows
>+    [self close];
>+  }
>+}
>+

I don't like the name of this method; it's really "maybeCloseWindowIfAllDownloadsAreDone"
which is unweildy. -maybeCloseWindow is almost better.

Also, it would be cheaper to test for [self numDownloadsInProgress] == 0 first, and
only then read the pref.

r=me with those changes.

Comment 21

[Nick Kreeger]

Attachment: Fixed smfr's nits

Update to meet the last few comments

Comment 22

[Nick Kreeger]

Fixed by checkin from mento, landed on branch and trunk.