Closed Bug 395537 Opened 17 years ago Closed 17 years ago

Nothing happens if you resume a download that you deleted

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file, 11 obsolete files)

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?
yes we should
Attached patch v1 (obsolete) (deleted) — Splinter Review
Start at 0 if the file is gone. Works for same session and cross session downloads.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #280223 - Flags: review?(comrade693+bmo)
Attached patch ignoremeplz (obsolete) (deleted) — Splinter Review
Comment on attachment 280223 [details] [diff] [review] v1 r=sdwilsh This is very low risk.
Attachment #280223 - Flags: review?(comrade693+bmo) → review+
Attached patch v2 (obsolete) (deleted) — Splinter Review
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
Attachment #280223 - Attachment is obsolete: true
Attachment #280224 - Attachment is obsolete: true
Attachment #280436 - Flags: review?(comrade693+bmo)
Blocks: 394548
Depends on: 230870
Attached patch v2.1 (obsolete) (deleted) — Splinter Review
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.
Attachment #280436 - Attachment is obsolete: true
Attachment #280750 - Flags: review?(comrade693+bmo)
Attachment #280436 - Flags: review?(comrade693+bmo)
Comment on attachment 280750 [details] [diff] [review] v2.1 This should have an xpcshell testcase with it as well. This is totally testable.
Attachment #280750 - Flags: review?(comrade693+bmo) → review+
Do I need to figure out how to make that testcase or just in-testsuite? before a1.9?
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.
Attached file testcase v1 (obsolete) (deleted) —
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 ***
Attachment #281609 - Flags: review?(comrade693+bmo)
Attached file bug_395537.sqlite (obsolete) (deleted) —
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 ;)]
Attachment #281609 - Attachment mime type: application/x-javascript → text/plain
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.
Attachment #281609 - Flags: review?(comrade693+bmo) → review-
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.
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).
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.
Attached file testcase idea (obsolete) (deleted) —
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
Attachment #281609 - Attachment is obsolete: true
I have no idea with the first, but the second one is just fine. Sorry :/
Mossop seems to think that your second issue is because of the stat cache. See also Bug 386256. Maybe cloning it might help first...
Attached file testcase idea v1.1 (obsolete) (deleted) —
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 <<<<<<<
Attachment #281610 - Attachment is obsolete: true
Attachment #281990 - Attachment is obsolete: true
Attachment #282710 - Attachment mime type: application/x-javascript → text/plain
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.
Attached patch patch against testcase v1.1 (obsolete) (deleted) — Splinter Review
Attachment #282710 - Attachment is obsolete: true
Attached patch v2.1 + testcase v2 (obsolete) (deleted) — Splinter Review
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.
Attachment #280750 - Attachment is obsolete: true
Attachment #282898 - Flags: review?(comrade693+bmo)
Attached patch v2.1 + testcase v2 (obsolete) (deleted) — Splinter Review
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.
Attachment #282898 - Attachment is obsolete: true
Attachment #282899 - Flags: review?(comrade693+bmo)
Attachment #282898 - Flags: review?(comrade693+bmo)
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.
Flags: blocking-firefox3?
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.
Attachment #282899 - Flags: review?(comrade693+bmo) → review+
Attachment #282899 - Flags: approval1.9?
Flags: blocking-firefox3? → blocking-firefox3+
Attached patch v2.1 + testcase v2.1 (deleted) — Splinter Review
Made comments into /** * */ blocks and numbered the order they are processed.
Attachment #282897 - Attachment is obsolete: true
Attachment #282899 - Attachment is obsolete: true
Attachment #282899 - Flags: approval1.9?
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M9
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: