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)

*** Bug 181426 has been marked as a duplicate of this bug. ***

confirming based on dup

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).

*** 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

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.

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.

*** 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

*** 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

duplicate of bug 147254?

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).

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.

*** Bug 188163 has been marked as a duplicate of this bug. ***

taking

patch checked in.

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?

no, due to ambiguous inheritance from nsISupports. See bug 181387

This patch caused bug 188877

Verified on the Win32 2003-01-20-08 trunk build under Windows XP.

*** 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.