835872 - Handle download errors

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

In the jsdownloads API, errors should be reported correctly, and it should
be possible to distinguish between errors from the network or errors while
writing the target file.

Comment 1

[:Paolo Amadini]

If the download fails, a failureReason property would give access to an instance
of a DownloadError object containing the details of the failure, for example:
* DownloadError.becauseSourceFailed
* DownloadError.becauseTargetFailed
* DownloadError.becauseDiskFull
* DownloadError.result

Comment 2

[:Paolo Amadini]

Attachment: The patch

This implements the DownloadError object, with the becauseSourceFailed and
becauseTargetFailed properties, with the model of OS.File.Error. I didn't
implement becauseDiskFull yet since I don't know how to write a sane
automated test for it, but we can add this later together with other
properties if we deem those handy.

In this patch, I haven't yet renamed the status properties as previously
discussed, will do that while implementing cancel and resume.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 712951 [details] [diff] [review]
The patch

Review of attachment 712951 [details] [diff] [review]:
-----------------------------------------------------------------

off-topic nit: I dislike the because naming, I would have preferred reason. But I see OS.File is driving already here :(

btw, here is my feedback

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +102,5 @@
> +   * When the download fails, this is set to a DownloadError instance indicating
> +   * the cause of the failure.  If the download has been completed successfully
> +   * or has been canceled, this property is null.
> +   */
> +  failureReason: null,

exactly cause failureReason naming doesn't make sense if there was no failure, I suggest something more like lastError... in case of success lastError == null makes more sense than failureReason == null

@@ +291,5 @@
> + *        by a component that handles both aspects of the download.
> + *
> + * @returns The newly created DownloadError object.
> + */
> +DownloadError._fromResult = function DE_createFromResult(aResult, aInferCause)

it's a bit hard to figure if this is the right approach since there is only one caller that always passes true as second argument...

one thing I noticed is that "fromResult" is a bit too generic naming, I had no idea what a "result" was in this case. maybe .createFromResultValue could be better...

Though I'm not sure why aInferCause couldn't just be an optional param of the DownloadError constructor?

Comment 4

[Neil Deakin]

> +function DownloadError(aMessage, aResult)
> +{
> +  this.name = "DownloadError";

What is the name used for?

>+  let downloadError = new DownloadError(exception, exception.result);

You pass an exception here when calling the constructor but the constructor expects a string message.

Can you document the properties that are set by the constructor if they are meant to be public?

+    do_check_true(download.failureReason.becauseSourceFailed);

I'd also test that the becauseTargetFailed property is false. Same with the other test.

Comment 5

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] (Away 14-17 Feb) from comment #3)
> Though I'm not sure why aInferCause couldn't just be an optional param of
> the DownloadError constructor?

So, it's probably too early to draw a distinction between what's public and
what is semi-public for the DownloadError object, probably the only objects
that need to construct DownloadError objects are DownloadSaver implementations.
I've filed bug 825588 to figure out an interface when needed, for now I'll move
everything to the constructor, that is used only internally.

Comment 6

[:Paolo Amadini]

That was bug 841348.

Comment 7

[:Paolo Amadini]

Attachment: Updated patch

Comment 8

[Neil Deakin]

> +function DownloadError(aMessage, aResult)
> +{
> +  this.name = "DownloadError";
>
> What is the name used for?


>Can you document the properties that are set by the constructor if they are meant to
> be public?

I didn't see these two comments addressed.

Comment 9

[:Paolo Amadini]

(In reply to Neil Deakin from comment #8)
> I didn't see these two comments addressed.

I thought I addressed them by documenting the "result" property and adding the
comment saying that "name" is used by the Error object prototype. I didn't
document again the six Error prototype properties that are mentioned here:

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Error/prototype

Did you have something else in mind?

Comment 10

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a57be02f88

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7a57be02f88