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

RESOLVED FIXED in Camino1.0

Status

defect
--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: ed.gough, Assigned: nick.kreeger)

Tracking

({fixed1.8, hang})

unspecified
Camino1.0
PowerPC
macOS
Dependency tree / graph
Bug Flags:
camino1.0 +

Details

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

14 years ago
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
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.
Reporter

Comment 2

14 years ago
(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.
Indeed, that's the key.  Then the DL Manager disappears with the sheet still focused, and nothing else works.  Another locked state bug, fun!
Blocks: 294129
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: hang
Target Milestone: --- → Camino1.0
Assignee

Comment 4

14 years ago
Confirming, what happens when open file is selected as a pref as well?
Reporter

Comment 5

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

It hangs still.
Assignee

Comment 6

14 years ago
Thanks, i'll take
Assignee: mikepinkerton → nick.kreeger
Assignee

Comment 7

14 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
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!
Assignee

Comment 8

14 years ago
Posted patch Fixed comments (obsolete) — Splinter Review
Assignee

Updated

14 years ago
Attachment #202522 - Attachment is obsolete: true

Comment 9

14 years ago
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?
Assignee

Comment 10

14 years ago
Posted patch Updated to mento's comments (obsolete) — Splinter Review
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.
Attachment #202541 - Attachment is obsolete: true
Flags: camino1.0+

Comment 11

14 years ago
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.
Attachment #202735 - Flags: review-
Assignee

Comment 12

14 years ago
Posted patch More mento updates (obsolete) — Splinter Review
Updated via mento's comments
Attachment #202735 - Attachment is obsolete: true
Summary: Program lock out if a download completes while quitting → 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
Assignee

Comment 13

14 years ago
Updated to include the changes made from bug 307753's checkin. I also fixed a comment in |initWithDictionary:| in ProgressViewController.h.
Attachment #202833 - Attachment is obsolete: true

Comment 14

14 years ago
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.
Assignee

Comment 15

14 years ago
Posted patch Update (obsolete) — Splinter Review
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.
Attachment #203027 - Attachment is obsolete: true
Assignee

Comment 16

14 years ago
Posted patch Comment fixes (obsolete) — Splinter Review
Attachment #203031 - Attachment is obsolete: true
Assignee

Comment 17

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

Comment 18

14 years ago
Waiting for review?
Assignee

Comment 19

14 years ago
Yes can someone please take a look at this patch?

Updated

14 years ago
Attachment #203034 - Flags: review?
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.
Attachment #203034 - Flags: review? → review+
Assignee

Comment 21

14 years ago
Update to meet the last few comments
Attachment #203034 - Attachment is obsolete: true
Assignee

Comment 22

14 years ago
Fixed by checkin from mento, landed on branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.