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
Summary: Crash if you stop a paused download → Crash if you stop a paused download [@ nsDownloadManager::ExecuteDesiredAction]
I cannot reproduce this, but I have a patch that should probably fix it...
Created attachment 279900 [details] [diff] [review] v1.0 If someone who can reproduce this please test this, it'd be great. Once I know it works, I will request review.
Assignee: nobody → comrade693+bmo
Status: NEW → ASSIGNED
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
Created attachment 280035 [details] [diff] [review] v2 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).
Assignee: comrade693+bmo → edilee
Attachment #280035 - Flags: review?(comrade693+bmo)
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
Attachment #280035 - Flags: review?(comrade693+bmo) → review+
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.
Created attachment 280065 [details] [diff] [review] v2.1 (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.
Attachment #280035 - Attachment is obsolete: true
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...
Created attachment 280067 [details] [diff] [review] v3 Oops. DeMorgan's law to match the comment ;)
Comment on attachment 280067 [details] [diff] [review] v3 ah shoot - that's my bad :(
Attachment #280067 - Flags: review?(comrade693+bmo) → review+
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).
Attachment #280067 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Crash Signature: [@ nsDownloadManager::ExecuteDesiredAction]
You need to log in before you can comment on or make changes to this bug.