Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007090602 Minefield/3.0a8pre ID:2007090602 1. new profile, start firefox 2. go to http://hourly-archive.localgho.st/ 3. right click on a link, save link as 4. choose a dir, and save the file so the download starts. 5. click the pause button on the active download 6. click the stop button on the puased download crsah! I will test if this is a regression from the download-resume patch

no crash with 20070905_2155_firefox-3.0a8pre.en-US.win32 crash with 20070905_2210_firefox-3.0a8pre.en-US.win32 Checkins to module PhoenixTinderbox between 2007-09-05 21:55 and 2007-09-05 22:09 : http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1189054500&maxdate=1189055399 I will guess due to bug 377243.

Any chance you got a backtrace with the crash reporter?

Unlikely, since I'm on win2k and using an hourly build. I'll try and find someone with a better OS and a nightly :)

I can confirm the crash using Vista HP, but no breakpad report was created. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre Firefox/3.0 ID:2007090604

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre ID:2007090604 http://crash-stats.mozilla.com/report/index/8f6ab438-5c7a-11dc-acbb-001a4bd43ef6 Courtesy of pal-moz from mz forums

I cannot reproduce this, but I have a patch that should probably fix it...

If someone who can reproduce this please test this, it'd be great. Once I know it works, I will request review.

The problem is that when we cancel a download.. 1) we delete the mTempFile from CancelDownload 2) call SetState with CANCELED 3) call CompleteDownload 4) call ExecuteDesiredAction that is expecting mTempFile to exist 5) try to move a nonexistant file

This fixes the crash for me. Patch makes sure we only try to "Execute" if we finished.. also add some sanity check to make sure mTempFile exists before doing stuff with it (like moving).

Comment on attachment 280035 [details] [diff] [review] v2 >+ // we need do what exthandler would have done for a finished download >+ if (aDownload->mDownloadState == nsIDownloadManager::DOWNLOAD_FINISHED && >+ aDownload->mWasResumed) nit: spacing >+ // We need to bail if for some reason the temp file got removed >+ PRBool fileExists; >+ if (NS_FAILED(aDownload->mTempFile->Exists(&fileExists)) || !fileExists) >+ return NS_ERROR_FAILURE; I'm not so sure we want to fail here, but then we don't even bother to check the return result... r=sdwilsh

fwiw, ExecuteDesiredAction seems like a bad name to me, since all it ever does is rename a file...

(In reply to comment #11) > fwiw, ExecuteDesiredAction seems like a bad name to me, since all it ever does > is rename a file... > This is what it does now. Once we start supporting "Open with" downloads we will be doing what its ext handler counterpart does -- Really execute a desired action, which might be Launch the necessary app or move the file.

(In reply to comment #10) > >+ if (aDownload->mDownloadState == nsIDownloadManager::DOWNLOAD_FINISHED && > >+ aDownload->mWasResumed) > nit: spacing Spaced. > >+ // We need to bail if for some reason the temp file got removed > >+ PRBool fileExists; > >+ if (NS_FAILED(aDownload->mTempFile->Exists(&fileExists)) || !fileExists) > >+ return NS_ERROR_FAILURE; > I'm not so sure we want to fail here, but then we don't even bother to check > the return result... Well.. what would we do instead if we didn't fail but didn't have a file to do anything? We'll want to eventually check its return value and let the user know in the case where something deletes the temp file just as it finishes downloading.

Hold on. aDownload->mTempFile->Exists(&fileExists) doesn't seem to work... ?? EXC_BAD_ACCESS(0x0001) KERN_PROTECTION_FAILURE (0x0002) at 0x00000000 console prints out the nsCOMPtr assertion mRawPtr != 0

huh? I don't even see how that is possible. We check to ensure that aDownload->mTempFile is not null, and if aDownload is null....well, I'm not even sure how the hell that could happen...

Oops. DeMorgan's law to match the comment ;)

Comment on attachment 280067 [details] [diff] [review] v3 ah shoot - that's my bad :(

Mike, we should probably take this for M8 - a lot of people can easily hit this, but it's a pretty simple patch (low risk).

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v <-- nsDownloadManager.cpp new revision: 1.113; previous revision: 1.112 done

I was able to reproduce the crash in pre-fix build: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre, but --NOT-- in today's trunk: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007091005 Minefield/3.0a8pre Verified FIXED using the steps to reproduce in comment 0.

http://litmus.mozilla.org/show_test.cgi?id=4578 in-litmus+