Crash if you stop a paused download [@ nsDownloadManager::ExecuteDesiredAction]

VERIFIED FIXED in mozilla1.9alpha8

Status

()

Toolkit
Downloads API
--
critical
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: stevee, Assigned: Mardak)

Tracking

({crash, regression})

Trunk
mozilla1.9alpha8
crash, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
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.
Blocks: 377243
Flags: blocking-firefox3?
Keywords: regression
Any chance you got a backtrace with the crash reporter?
(Reporter)

Comment 3

10 years ago
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
(Reporter)

Comment 5

10 years ago
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
(Assignee)

Comment 8

10 years ago
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
(Assignee)

Comment 9

10 years ago
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)
(Assignee)

Updated

10 years ago
OS: Windows 2000 → All
Hardware: PC → All
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.
(Assignee)

Comment 13

10 years ago
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
(Assignee)

Comment 14

10 years ago
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...
(Assignee)

Comment 16

10 years ago
Created attachment 280067 [details] [diff] [review]
v3

Oops. DeMorgan's law to match the comment ;)
Attachment #279900 - Attachment is obsolete: true
Attachment #280065 - Attachment is obsolete: true
Attachment #280067 - Flags: review?(comrade693+bmo)
Comment on attachment 280067 [details] [diff] [review]
v3

ah shoot - that's my bad :(
Attachment #280067 - Flags: review?(comrade693+bmo) → review+

Updated

10 years ago
Target Milestone: --- → Firefox 3 M8

Updated

10 years ago
Attachment #280067 - Flags: approval1.9?
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).

Updated

10 years ago
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: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Blocks: 230870
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?
http://litmus.mozilla.org/show_test.cgi?id=4578

in-litmus+
Flags: in-litmus? → in-litmus+

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Product: Firefox → Toolkit
Crash Signature: [@ nsDownloadManager::ExecuteDesiredAction]
You need to log in before you can comment on or make changes to this bug.