382388 - nsDownload::OnStateChange doesn't check aStatus for failure

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

Attachment: FailDownloads if aStatus FAILED

Downloads can get a failure status when stopping, but the download manager reports "Done."

Other observers like nsExternalAppHandler will honor the failure even though the transfer completed [1], and it potentially will delete the file if it was requested to be opened. If the file is being saved to disk, it'll remain as a partial download.

In either case, the download manager probably shouldn't be reporting "Done" and perhaps "Failed" instead.

The attachment extracts the "failing" code of OnStatusChange to be reused for failures in OnStateChange, but it doesn't use the prompter to show the modal dialog. This means the downloads will just appear in the download manager with "Failed" (and no indication of why).

[1] http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2129

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 266557 [details] [diff] [review]
FailDownloads if aStatus FAILED

Looks good to me, but I'm not a reviewer.

Comment 2

[Ed Lee :Mardak]

If bug 383716 is fixed, there's less of a need to check for failure because nsDownload will get OnStatus (and fail downloading) and won't be getting OnState Stop anyway.

This also avoids the issue of making more error strings because there isn't one handy for OnState failures.

Comment 3

[Ed Lee :Mardak]

Comment on attachment 266557 [details] [diff] [review]
FailDownloads if aStatus FAILED

From discussion with biesi, we should probably also display a prompter message on failure from OnState and not just OnStatus.

This will need a new generic error failure message in downloads.properties. Specific error messages /should/ be arriving from exthandler or webbrowserpersist OnStatus (i.e., OnState won't get called).

If Bug 383716 is fixed, both exthandler and webbrowserpersist *won't* call OnState = stop. Are there any other observers that have nsDownload as a ProgressListener? Is it part of the "contract" that OnStatus /will/ be called on failure and *not* OnState?

Comment 4

[Shawn Wilsher :sdwilsh]

Edward, are you still working on this bug?

Comment 5

[Ed Lee :Mardak]

Attachment: v2

Comment 6

[Ed Lee :Mardak]

Patch's diff needs bug 394548. Tested with a modified WBP that always gave a failure OnStop because this bug was originally needed for Link Fingerprints.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 282968 [details] [diff] [review]
v2

I know that you were just moving code around for the most part, but I'd be happier if you checked the result or cast to void (probably checking for most of those).

Comment 8

[Ed Lee :Mardak]

Attachment: v2.1

NS_ENSURE_SUCCESS for everything and return the last thing (Alert).

Comment 9

[Ed Lee :Mardak]

Oops. I've fixed the spacing locally (in 2 places for GetStringFromName(NS_LITERAL_STRING... 2 spaces instead of 4)

Comment 10

[Shawn Wilsher :sdwilsh]

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

nit: s/NULL/nsnull/

r=sdwilsh

Comment 11

[Ed Lee :Mardak]

Supposed to use nsnull instead of NULL even for raw PRUnichar pointers?

Comment 12

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

yes, use nsnull for everything

Comment 13

[Shawn Wilsher :sdwilsh]

That's my understanding - technically they are the same thing (0) as far as I know

Comment 14

[Ed Lee :Mardak]

Attachment: for checkin

NULL -> nsnull

Comment 15

[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.131; previous revision: 1.130
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.49; previous revision: 1.48
done
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties,v  <--  downloads.properties
new revision: 1.10; previous revision: 1.9
done