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

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Steve England [:stevee]]

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

Comment 1

[Steve England [:stevee]]

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.

Comment 2

[Shawn Wilsher :sdwilsh]

Any chance you got a backtrace with the crash reporter?

Comment 3

[Steve England [:stevee]]

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

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 5

[Steve England [:stevee]]

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

Comment 6

[Shawn Wilsher :sdwilsh]

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

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

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

Comment 8

[Ed Lee :Mardak]

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

Comment 9

[Ed Lee :Mardak]

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

Comment 10

[Shawn Wilsher :sdwilsh]

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

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 12

[Srirang G Doddihal (Brahmana)]

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

Comment 13

[Ed Lee :Mardak]

Attachment: 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.

Comment 14

[Ed Lee :Mardak]

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

Comment 15

[Shawn Wilsher :sdwilsh]

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

Comment 16

[Ed Lee :Mardak]

Attachment: v3

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

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 280067 [details] [diff] [review]
v3

ah shoot - that's my bad :(

Comment 18

[Shawn Wilsher :sdwilsh]

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

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

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

in-litmus+