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)
Toolkit
Downloads API
Tracking
()
VERIFIED
FIXED
mozilla1.9beta1
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
Attachments
(1 file, 11 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Updated•17 years ago
|
Flags: in-litmus?
Comment 1•17 years ago
|
||
yes we should
Assignee | ||
Comment 2•17 years ago
|
||
Start at 0 if the file is gone. Works for same session and cross session downloads.
Assignee | ||
Comment 3•17 years ago
|
||
Comment 4•17 years ago
|
||
Flags: in-litmus? → in-litmus+
Comment 5•17 years ago
|
||
Comment on attachment 280223 [details] [diff] [review]
v1
r=sdwilsh
This is very low risk.
Attachment #280223 -
Flags: review?(comrade693+bmo) → review+
Assignee | ||
Comment 6•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 7•17 years ago
|
||
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 8•17 years ago
|
||
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+
Assignee | ||
Comment 9•17 years ago
|
||
Do I need to figure out how to make that testcase or just in-testsuite? before a1.9?
Comment 10•17 years ago
|
||
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.
Assignee | ||
Comment 11•17 years ago
|
||
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)
Assignee | ||
Comment 12•17 years ago
|
||
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 ;)]
Updated•17 years ago
|
Attachment #281609 -
Attachment mime type: application/x-javascript → text/plain
Comment 13•17 years ago
|
||
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-
Assignee | ||
Comment 14•17 years ago
|
||
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•17 years ago
|
||
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).
Assignee | ||
Comment 16•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
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
Comment 18•17 years ago
|
||
I have no idea with the first, but the second one is just fine. Sorry :/
Comment 19•17 years ago
|
||
Mossop seems to think that your second issue is because of the stat cache. See also Bug 386256. Maybe cloning it might help first...
Assignee | ||
Comment 20•17 years ago
|
||
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
Updated•17 years ago
|
Attachment #282710 -
Attachment mime type: application/x-javascript → text/plain
Comment 21•17 years ago
|
||
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.
Assignee | ||
Comment 22•17 years ago
|
||
Attachment #282710 -
Attachment is obsolete: true
Assignee | ||
Comment 23•17 years ago
|
||
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)
Assignee | ||
Comment 24•17 years ago
|
||
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)
Assignee | ||
Comment 25•17 years ago
|
||
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 26•17 years ago
|
||
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+
Updated•17 years ago
|
Attachment #282899 -
Flags: approval1.9?
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Comment 27•17 years ago
|
||
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?
Assignee | ||
Comment 28•17 years ago
|
||
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
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•