395537 - Nothing happens if you resume a download that you deleted

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

1) Start a download
2) Pause
3) Delete file
4) Resume

Expected: Resume with magic

Actual: Nothing happens (UI just sits there)

It's failing because GetFileSize fails for the Clone.

Neat to note that if you touch the file (create a blank file), it'll get a file size of 0 and "resume" from 0 and download fine.

Perhaps we should silently succeed by restarting at 0 if the file doesn't exist?

Comment 1

[Shawn Wilsher :sdwilsh]

yes we should

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Start at 0 if the file is gone. Works for same session and cross session downloads.

Comment 3

[Ed Lee :Mardak]

Attachment: ignoremeplz

Comment 4

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

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

in-litmus+

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 280223 [details] [diff] [review]
v1

r=sdwilsh

This is very low risk.

Comment 6

[Ed Lee :Mardak]

Attachment: v2

Because we're allowing resuming from position 0, we need to change our idea of "WasResumed?"

Ignore the long line, it'll be fixed by bug 394548. -> blocks

The bits for WasResumed come from bug 230870. -> depends

Comment 7

[Ed Lee :Mardak]

Attachment: v2.1

We don't need to fail just because the clone failed, we can assume 0. No need to check exist because GetSize will return failure if it doesn't.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 280750 [details] [diff] [review]
v2.1

This should have an xpcshell testcase with it as well.  This is totally testable.

Comment 9

[Ed Lee :Mardak]

Do I need to figure out how to make that testcase or just in-testsuite? before a1.9?

Comment 10

[Shawn Wilsher :sdwilsh]

need it before checkin.  Test case should just be pausing it, deleting the file, then resuming it.  Waldo might be able to help you out if you have any additional questions.

Comment 11

[Ed Lee :Mardak]

Attachment: testcase v1

Neat little test which will fail like this before this bug is fixed..

[Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIDownloadManager.resumeDownload]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/test_bug_395537.js :: run_test :: line 51"  data: no]
*** FAIL ***

Comment 12

[Ed Lee :Mardak]

Attachment: bug_395537.sqlite

sqlite file for testcase v1; contains a single download entry id 1 that is paused (downloading a nightly trunk to my desktop)

[a side note.. the test passes without the patch for this bug if I leave the file on my desktop.... neat :p but it'll fail once I delete it ;)]

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 281609 [details]
testcase v1

This will not work across platforms.  You'll have to add a download and then stop it, remove it, and then try to resume it.

Comment 14

[Ed Lee :Mardak]

What about it won't work across platforms? The file path is invalid for other platforms but it's supposed to fail accessing that file anyway. We just want to make sure it doesn't bail when resuming.

Comment 15

[Shawn Wilsher :sdwilsh]

I was looking more for a test case that was like the steps to reproduce in the bug for starters.

Secondly, would it not try to retry a download?  Something tells me that we can't rely on any web address here (which is why the other tests use the http server).

Comment 16

[Ed Lee :Mardak]

It probably would retry downloading and fail the download, but the call to Resume wouldn't fail -- it'll start the download and /later/ the download status would become failed. The test is just testing resume.

I'll come up with another testcase that tests resume and delete + resume.

Comment 17

[Ed Lee :Mardak]

Attachment: testcase idea

This should test download resume in general.. for this bug it would just need to delete the file before resuming. But some reason it complains about the file not existing when I do the check to see if the file size is the correct size (amount transferred is 0 as well, so it's not downloading).. 

Any ideas?

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsILocalFileMac.fileSize]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/test_resume.js :: anonymous :: line 90"  data: no]

Additionally, there's a bunch of...
*** Throwing trying to get CurProcD
*** Throwing trying to get MozBinD
*** Throwing trying to get cachePDir
*** Throwing trying to get ProfLD
*** Throwing trying to get cachePDir
*** Throwing trying to get ProfLD

Comment 18

[Shawn Wilsher :sdwilsh]

I have no idea with the first, but the second one is just fine.  Sorry :/

Comment 19

[Shawn Wilsher :sdwilsh]

Mossop seems to think that your second issue is because of the stat cache.  See also Bug 386256.  Maybe cloning it might help first...

Comment 20

[Ed Lee :Mardak]

Attachment: testcase idea v1.1

Any suggestions on what might be going wrong? Before it did seem to actually transfer data, but the file isn't created -- if I make the data big, the testcase takes longer to finish because it's transferring a larger file.

The actual assert *** CHECK FAILED: true == false is checking for didResume..

../../../../_tests/xpcshell-simple/test_dm/unit/test_resume.js.log:
>>>>>>>
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "Components.classes['@mozilla.org/xre/app-info;1'] has no properties" {file: "file:///Users/Ed/Desktop/mozilla/trunk/mozilla/objdir/dist/bin/components/nsBrowserContentHandler.js" line: 750}]' when calling method: [nsIModule::registerSelf]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************
*** Throwing trying to get CurProcD
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/head_download_manager.js :: anonymous :: line 65"  data: no]
************************************************************
*** Throwing trying to get MozBinD
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/head_download_manager.js :: anonymous :: line 65"  data: no]
************************************************************
*** test pending
*** Throwing trying to get cachePDir
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/test_resume.js :: run_test :: line 133"  data: no]
************************************************************
*** Throwing trying to get ProfLD
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/test_resume.js :: run_test :: line 133"  data: no]
************************************************************
*** Throwing trying to get cachePDir
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/test_resume.js :: run_test :: line 133"  data: no]
************************************************************
*** Throwing trying to get ProfLD
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/test_resume.js :: run_test :: line 133"  data: no]
************************************************************
*** exiting
*** CHECK FAILED: true == false
JS frame :: /Users/Ed/Desktop/mozilla/trunk/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 99
JS frame :: /Users/Ed/Desktop/mozilla/trunk/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 114
JS frame :: ../../../../_tests/xpcshell-simple/test_dm/unit/test_resume.js :: anonymous :: line 100
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Abort' when calling method: [nsIDownloadProgressListener::onDownloadStateChange]"  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "<unknown>"  data: no]
************************************************************
###!!! ASSERTION: attempted to open a new window with no WindowCreator: 'mWindowCreator', file /Users/Ed/Desktop/mozilla/trunk/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp, line 642

<<<<<<<

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 282710 [details]
testcase idea v1.1

>  /**
>   * 5. Busy wait the main thread until we're done
>   */
>  try {
>    var thread = Cc["@mozilla.org/thread-manager;1"].getService().currentThread;
>    while (!httpserv.isStopped())
>      thread.processNextEvent(true);
>    // get rid of any pending requests
>    while (thread.hasPendingEvents())
>      thread.processNextEvent(true);
>  } catch (e) {
>    print(e);
>  }
We should actually be using do_test_pending and do_test_finish here (there's a bug to update the rest of the DM tests to this as well).

I hope to look into the other issue later today.

Comment 22

[Ed Lee :Mardak]

Attachment: patch against testcase v1.1

Comment 23

[Ed Lee :Mardak]

Attachment: v2.1 + testcase v2

Testcase working now :) Had to fix some issues like bug 398069..

The other main issue was that real-pause causes a STATE_STOP, but because I was resuming the download from dlstate PAUSED, it would actually resume the download to DOWNLOADING *before* the STATE_STOP actually propagated. So instead of resuming right away, wait for the STATE_STOP.

Also, add some extra sanity checks for safe measure. Used do_test_pending and finished.

Comment 24

[Ed Lee :Mardak]

Attachment: v2.1 + testcase v2

Testcase working now :) Had to fix some issues like bug 398069..

The other main issue was that real-pause causes a STATE_STOP, but because I was resuming the download from dlstate PAUSED, it would actually resume the download to DOWNLOADING *before* the STATE_STOP actually propagated. So instead of resuming right away, wait for the STATE_STOP.

Also, add some extra sanity checks for safe measure. Used do_test_pending and finished.

Comment 25

[Ed Lee :Mardak]

b-ff3? This could happen if the user accidentally deletes the file such as quitting firefox with a cross session resumable download and clearing the desktop. Without the fix, the download manager will just sit there doing nothing as the user clicks resume.

Comment 26

[Shawn Wilsher :sdwilsh]

Comment on attachment 282899 [details] [diff] [review]
v2.1 + testcase v2

>+    onDownloadStateChange: function(a, aDl) {
>+      // queued -> downloading = pause the download
can you place these comments after the if statement?  I was having a hard time figuring out what they were for at first

>+      if (aDl.state == nsIDM.DOWNLOAD_DOWNLOADING && !didPause) {
>+        dm.pauseDownload(aDl.id);
>+      // downloading -> paused = remove the file
>+      } else if (aDl.state == nsIDM.DOWNLOAD_PAUSED) {
>+        didPause = true;
>+        // Test bug 395537 by removing the file before we actually resume
>+        aDl.targetFile.remove(false);
>+      // downloading (resumed) -> finished = check tests
>+      } else if (aDl.state == nsIDM.DOWNLOAD_FINISHED) {
>+        // did we pause at all?
>+        do_check_true(didPause);
>+        // did we real-resume and not fake-resume?
>+        do_check_true(didResumeDownload);
>+        // extra real-resume check for the server
>+        do_check_true(didResumeServer);
>+        // did we download the whole file?
>+        do_check_eq(data.length, aDl.targetFile.fileSize);
>+        // extra sanity checks on size
>+        // XXX remove short-circuit after bug 394548 is fixed
>+        true || do_check_eq(data.length, aDl.amountTransferred);
>+        true || do_check_eq(data.length, aDl.size);
>+
>+        httpserv.stop();
>+        // we're done with the test!
>+        do_test_finished();
>+      }
>+    },
>+    onStateChange: function(a, b, aState, d, aDl) {
>+      // paused -> stopped = resume the download
ditto

r=sdwilsh with these fixed.

Comment 27

[Ed Lee :Mardak]

Attachment: v2.1 + testcase v2.1

Made comments into /** * */ blocks and numbered the order they are processed.

Comment 28

[Ed Lee :Mardak]

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.129; previous revision: 1.128
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.47; previous revision: 1.46
done
RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/unit/test_resume.js,v
done
Checking in toolkit/components/downloads/test/unit/test_resume.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_resume.js,v  <--  test_resume.js
initial revision: 1.1
done

Comment 29

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

Verified FIXED using

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102721 Minefield/3.0a9pre

and 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102720 Minefield/3.0a9pre

I deleted download while paused, resumed them, and they all flawlessly started redownloading.