Closed Bug 181374 (progressquit) Opened 22 years ago Closed 22 years ago

Downloads lost when using Progress Dialog, keep window open after download unchecked and Progress dialog is the last window

Categories

(SeaMonkey :: Download & File Handling, defect)

Product:
SeaMonkey
Component:
Download & File Handling
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: enigma2, Assigned: bzbarsky)

References

Details

(Keywords: dataloss)

Attachments

(1 file)

patch with my nit addressed
3.10 KB, patch
timeless
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.3a) Gecko/20021119
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.3a) Gecko/20021119

Downloads lost when using Progress Dialog, keep window open after download
unchecked and Progress dialog is the last window

Reproducible: Always

Steps to Reproduce:
1.download a file from anywhere, make sure the conditions in [details] are met
2.close all browser windows, make sure -turbo mode is not enabled
3.after download is complete and progress dialog closes, file downloaded is MISSING

Actual Results:  
wheres the file?

Expected Results:  
saved it like normal

I've reproduced it on the Windows and OS/2 versions of Mozilla
-> File Handling (btw: the file should be in your temp folder)
Assignee: asa → law
Component: Browser-General → File Handling
QA Contact: asa → petersen
*** Bug 181426 has been marked as a duplicate of this bug. ***
confirming based on dup
Alias: progressquit
Status: UNCONFIRMED → NEW
Ever confirmed: true
Matti: unfortunately, the lost files are not in my temp dir :(
Using 2002112304 on WinXP Pro, I see this, even with turbo on.  As long as I
keep a browser window open until the download is complete, everything is fine. 
If I close all browser windows however, and just leave the progress meter, when
the download is finished the file just disappears (I've searched the entire
drive for it).
Keywords: dataloss
*** Bug 182663 has been marked as a duplicate of this bug. ***
On one occassion I went to download the lsot file again and it completed its
download in a fraction of a second. I'm guessing this is because most of the
file was located in a temp directory somewhere - although I checked and couldn't
find it. However, on three other occassions I have had to resume the download
from the very beginning again.
This is on WinXP using Moz 1.2
Never saw this behaviour in 1.1
I see it with Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3a) Gecko/20021123.

Requesting 1.3a blocking.

pi
Flags: blocking1.3a?
I second the request for 1.3a block, this bug is serious and needs attention ASAP.
OK, I have info on this.

The file is being deleted after it is moved.

What is happening is that nsExternalAppHandler::Cancel is called when the app
quits and it then deletes mTempFile in the externalapphandler.
I can't decide who is to blame for this.

Should the download manager know the download was finshed and hence not call
CancelDownload?

Should the externalhelperappservice be smart and know that StopRequest already
happened so it shouldn't do a cancel?

I'm going to start with download manager.
Assignee: law → blaker
Component: File Handling → Download Manager
Flags: blocking1.3a? → blocking1.3a+
jetro, please do not use flags if you don't know how they work. ? is a
nomination, + is a blessing and you're not qualified to add the +. I've set it
to ? for request which is what I assume you intended.
Flags: blocking1.3a+ → blocking1.3a?
*** Bug 183576 has been marked as a duplicate of this bug. ***
law, can you help us here?
*** Bug 183845 has been marked as a duplicate of this bug. ***
Related bug 147254
Flags: blocking1.3a? → blocking1.3a-
*** Bug 184195 has been marked as a duplicate of this bug. ***
*** Bug 184977 has been marked as a duplicate of this bug. ***
Could this be related to (dependant upon):
http://bugzilla.mozilla.org/show_bug.cgi?id=55690
Bug 147254 is about Download Manager while this one is about Progress Dialog.
But they are definetely related.

heres the problem, in nsDownloadmanager.cpp, function nsDownloadManager::Observe
in this block of code:
else if (nsCRT::strcmp(aTopic, "quit-application") == 0) {   // main browser
window closed...
  nsCOMPtr<nsISupports> supports;
  nsCOMPtr<nsIRDFResource> res;
  const char* uri;
  nsCOMPtr<nsIRDFInt> intLiteral;

  gRDFService->GetIntLiteral(DOWNLOADING, getter_AddRefs(intLiteral));
  nsCOMPtr<nsISimpleEnumerator> downloads;
  rv = mDataSource->GetSources(gNC_DownloadState, intLiteral, PR_TRUE, gett
  if (NS_FAILED(rv)) return rv;

  PRBool hasMoreElements;
  downloads->HasMoreElements(&hasMoreElements);

  while (hasMoreElements) {
    downloads->GetNext(getter_AddRefs(supports));
    res = do_QueryInterface(supports);
    res->GetValueConst(&uri);

    CancelDownload(uri);        // everything gets canceled here!
                                // including download in-progress, what the hell?

a simple bit-hack of appcomps.dll (no rebuild required) changing
"quit-application" to "xuit-application" will prevent downloads from getting
trashed.  can someone make the (proper) fix to the code to prevent downloads
from getting trashed?
See also bug 147254

The problem is that we _do_ want to cancel pending downloads on application quit
(so the change suggested in comment 22 is unacceptable).  But we should _not_ be
quitting the app in this case before marking the download is complete.  Are we
racing between OnStreamComplete (which is what marks a download complete) and a
progress notification here?  What code actually closes the dialog (just finding
the code that checks the "close when download complete" pref should be a good
indication of this).
Blocks: 147254
I'll send a donation of $35usd to the first person to check in a fix for this
bug.  (HONEST!)  anyone else wish to add to the pool?
this is a _really simple_ fix, don't know why I didn't figure it out sooner.
edit nsProgressdialog.js, find
this.mCancelDownloadOnClose = true;
change to:
this.mCancelDownloadOnClose = false;
save, reload mozilla.

_enjoy_
Recipe from the previous comment seems not work here ;-(

sorry, my bad, I modified the wrong debug build........ *red faced*

That fix is also incorrect, since we _do_ want to cancel the download on close
except in this one special case when we're closing because the download is
already complete.

Now if we can set mCancelDownloadOnClose in the same place where we decide to
close because the download is complete, that would be a decent fix.
What about this one? I haven't gotten my build environment complete yet, so this
is untestet.

--- xpfe/components/download-manager/src/nsDownloadManager.cpp.orig    
2002-12-15 15:48:07.000000000 -0100
+++ xpfe/components/download-manager/src/nsDownloadManager.cpp  2002-12-15
15:48:29.000000000 -0100
@@ -547,6 +547,10 @@
   if (!download)
     return NS_ERROR_FAILURE;

+  // Don't cancel if download is already finished
+  if (internalDownload->mDownloadState == FINISHED)
+    return NS_OK;
+
   internalDownload->SetDownloadState(CANCELED);

   // if a persist was provided, we can do the cancel ourselves.
That seems promising if it fixes the problem!
Sascha: I rebuilt with this code and it doesn't seem to help, I think that this
block of code from nsProgressdialog.js tells something:
                // Disable the Pause/Resume buttons.
                this.dialogElement( "pauseResume" ).disabled = true;

                // Fix up dialog layout (which gets messed up sometimes).
                this.dialog.sizeToContent(); <- this block is called if the keep
window open is checked
            } else if ( this.dialog ) {
                this.dialog.close();  <- if this is not here, it wont get
erased(keep window open not checked)
            }
        }
        return this.mCompleted;

perhaps this.dialog.close(); is closing the dialog without returning
this.mCompleted ? (just a wild guess..  I don't know much about js..)
Ok, I've done some digging in the code and think I've found the problem (correct
me if I'm wrong, but the new patch works fine):

When nsDownload::OnStateChange is used with STATE_STOP, it calls the dialog's
OnStateChange function which in turn closes down the dialog. This causes a
CancelDownload because if it was the last open window, and CancelDownload will
delete the file as mDownloadState is only set to FINISHED _after_ the dialog's
OnStateChange. So I just reverse that order.

The only problem I see in my patch is what happens if the GetNativePath call
fails, because the dialog is not updated then (but if it fails, I think we're
already in big trouble).

--- xpfe/components/download-manager/src/nsDownloadManager.cpp.orig	2002-12-16
00:21:17.000000000 -0100
+++ xpfe/components/download-manager/src/nsDownloadManager.cpp	2002-12-16
00:22:44.000000000 -0100
@@ -547,6 +547,10 @@
   if (!download)
     return NS_ERROR_FAILURE;
     
+  // Don't cancel if download is already finished
+  if (internalDownload->mDownloadState == FINISHED)
+    return NS_OK;
+
   internalDownload->SetDownloadState(CANCELED);
 
   // if a persist was provided, we can do the cancel ourselves.
@@ -1146,9 +1150,8 @@
       internalListener->OnStateChange(aWebProgress, aRequest, aStateFlags,
aStatus, this);
   }
 
-  if (mDialogListener)
-    mDialogListener->OnStateChange(aWebProgress, aRequest, aStateFlags, aStatus);
-
+  // We need to update mDownloadState before updating the dialog, because 
+  // that will close and call CancelDownload if it was the last open window
   if (aStateFlags & STATE_STOP) {
     if (mDownloadState == DOWNLOADING || mDownloadState == NOTSTARTED) {
       mDownloadState = FINISHED;
@@ -1170,6 +1173,9 @@
       mPersist->SetProgressListener(nsnull);
   }
 
+  if (mDialogListener)
+    mDialogListener->OnStateChange(aWebProgress, aRequest, aStateFlags, aStatus);
+
   return NS_OK;
 }
 

It would be better to prevent CancelDownload even being called if the state is
FINISHED.  Or does the caller not have access to the state (eg is JS code)?

Also, I'd just change the 

if (NS_FAILED(rv)) return rv;
mDownloadManager->DownloadEnded(path.get(), nsnull);

into

if (NS_SUCCEEDED(rv)) { 
  mDownloadManager->DownloadEnded(path.get(), nsnull);
}

for two reasons:

1)  We _do_ want to fire that progress change no matter what's going on with the
    path
2)  The existing code leaks -- see the "if (mPersist)" code five lines below
    that NS_FAILED(rv) check.

You'll want to "return rv;" instead of "return NS_OK;" at the end of the
function, of course.  And move the declaration of "rv" out to the beginning of
the function:

nsresult rv = NS_OK;

Apart from that nit, this is definitely the right approach; thanks for the
patch!  (and attach the next iteration as a separate attachment, ok?  That way
reviews can be marked on it).
*** Bug 186282 has been marked as a duplicate of this bug. ***
*** Bug 186306 has been marked as a duplicate of this bug. ***
*** Bug 186919 has been marked as a duplicate of this bug. ***
*** Bug 185190 has been marked as a duplicate of this bug. ***
Nominating as a possible 1.3b blocker, although it looks like it should be fixed
before then anyway.
Flags: blocking1.3b?
*** Bug 188163 has been marked as a duplicate of this bug. ***
Attachment #110992 - Flags: review+
Attachment #110992 - Flags: superreview+
taking
Assignee: blaker → bzbarsky
patch checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
removing blocking1.3b nomination as this is already checked in

however... bz:
+  nsCOMPtr<nsIDownload> download;
+  CallQueryInterface(internalDownload, NS_STATIC_CAST(nsIDownload**,
+                                                      getter_AddRefs(download)));

can't you use do_QueryInterface here?
Flags: blocking1.3b?
no, due to ambiguous inheritance from nsISupports. See bug 181387
This patch caused bug 188877
Blocks: 188877
Verified on the Win32 2003-01-20-08 trunk build under Windows XP.
Status: RESOLVED → VERIFIED
*** Bug 187289 has been marked as a duplicate of this bug. ***
*** Bug 190713 has been marked as a duplicate of this bug. ***
*** Bug 192831 has been marked as a duplicate of this bug. ***
how the 'ell do I remove my email address from this bug so I can stop getting
**** via email??
In the upper right corner, click on your email and click "Remove selected CCs".
Then hit commit without changing anything else or making a comment.
netdemon: what a useless comment. he reported the bug.

rasta m0n: as you reported the bug, you normally get mail about it. to disable
that, click "Prefs" at the bottom of a bugzilla page, and disable getting mail
when you are the reporter, on the "Email" page there.
For the third time, I didn't notice he submitted the bug. Please don't send me
any more emails about it, I'm not stupid.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: