1275289 - Uncaught exception - NS_ERROR_NOT_AVAILABLE NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.responseStatus] DownloadLegacy.js:96

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: test file with link that don't exist so when you alt+click to download, you get an uncaught exception

When you attempt to download a file whose link doesn't exist, you get a failed download. However, there is an uncaught exception

NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.responseStatus]
DownloadLegacy.js:96

Comment 1

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

Review commit: https://reviewboard.mozilla.org/r/54906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54906/

Comment 2

[:Paolo Amadini]

Comment on attachment 8755967 [details]
Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

https://reviewboard.mozilla.org/r/54906/#review52120

We need a regresion test in "common_test_Download.js" for this case. If the code change unblocks you in bug 1109146 we can land this bug in two parts, but probably it won't take long to add a test case anyways.

::: toolkit/components/jsdownloads/src/DownloadLegacy.js:92
(Diff revision 1)
>  
> +      let blockedByParentalControls;

let blockedByParentalControls = false, otherwise we're leaving this uninitialized.

::: toolkit/components/jsdownloads/src/DownloadLegacy.js:93
(Diff revision 1)
> +      let blockedByParentalControls;
> +      // If it is a failed download. aRequest.responseStatus doesn't exist.
> +      try {

Bear with me, I'm working on a few other bugs and I'll only be able to try this locally in a few days.

Can you specify what "failed download" means here? Just the case of missing file on the server, or other network failures as well?

Is the aStatus argument of the function still a success code in this case? If not, we can check that instead of handling exceptions.

::: toolkit/components/jsdownloads/src/DownloadLegacy.js:100
(Diff revision 1)
> +      } catch (e if e.name == 'NS_ERROR_NOT_AVAILABLE') {
> +        this._componentFailed = true;
> +      }

We don't check the "name" property but the numeric error code, and also the "catch e if" non-standard syntax has been removed from this area of the code.

Look for code doing similar checks in the "jsdownloads" folder.

Comment 3

[Jimmy Wang (:jimicy) - works on e10s stuff]

(In reply to :Paolo Amadini from comment #2)
> Comment on attachment 8755967 [details]
> MozReview Request: Bug 1275289 - Add try catch to DownloadLegacy.js since
> aRequest.responseStatus does not exist in the case of a failed
> download;r?paolo
> 
> https://reviewboard.mozilla.org/r/54906/#review52120
> 
> We need a regresion test in "common_test_Download.js" for this case. If the
> code change unblocks you in bug 1109146 we can land this bug in two parts,
> but probably it won't take long to add a test case anyways.

Without try and catch Failing download
onStateChange     this._componentFailed: false
onStateChange     !Components.isSuccessCode(aStatus): false
NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE)
NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) 
onStatusChange    this._componentFailed: false
onStatusChange    !Components.isSuccessCode(aStatus): true

With try and catch Failing download
onStateChange                         this._componentFailed: false
onStateChange                         !Components.isSuccessCode(aStatus): false
onStatusChange                        this._componentFailed: true
onStatusChange                        !Components.isSuccessCode(aStatus): true
_deferDownload.promise
download.saver.deferCanceled.promise
download.saver.deferCanceled.promise  this._componentFailed: true

This is the only difference in what happens with the try and catch.
> download.saver.onTransferStarted(aRequest, this._cancelable instanceof Ci.nsIHelperAppLauncher);
being executed in the try and catch failing download code path.
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadLegacy.js#111

this._cancelable instanceof Ci.nsIHelperAppLauncher == false
So in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#2433. It will do this.addToHistory();. How can I check that a download was added to download history?

The strange thing is that even without the try and catch when I alt+click a link that doesn't work, the failed download still gets added to download history.

Here's a video of that happening https://www.dropbox.com/s/5mqaovfwomyrxx0/no-try-catch-still-adds-download-history.mov?dl=0
The try catch is commented out and a debugger statement is put above download.saver.onTransferStarted(...) which never gets hit.

Could you advise how I should go about writing a regression test for catching the exception?

Also should we be using DownloadLegacy.js? Right click save as on a link seems to have a different code path. It displays a prompt that says "The download cannot be saved because an unknown error occurred." for a download link whose file doesn't exist.

Comment 4

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8755967 [details]
Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54906/diff/1-2/

Comment 5

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8755967 [details]
Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54906/diff/2-3/

Comment 6

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/54906/#review52120

> Bear with me, I'm working on a few other bugs and I'll only be able to try this locally in a few days.
> 
> Can you specify what "failed download" means here? Just the case of missing file on the server, or other network failures as well?
> 
> Is the aStatus argument of the function still a success code in this case? If not, we can check that instead of handling exceptions.

Missing file on the server and network failure to download a file

aStatus is still a success code in this case.
```
if (!Components.isSuccessCode(aStatus)) {
  this._componentFailed = true;
}
```
since doesn't get triggered

Comment 7

[:Paolo Amadini]

Comment on attachment 8755967 [details]
Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

https://reviewboard.mozilla.org/r/54906/#review64454

::: toolkit/components/jsdownloads/src/DownloadLegacy.js:102
(Diff revision 3)
> +        if (e.result == Cr.NS_ERROR_NOT_AVAILABLE) {
> +          this._componentFailed = true;
> +        }

What I'm concerned about is that if we set this._componentFailed to true but aStatus is a success code, for some reason the request in aRequest might still be running, even if we subsequently ignore it.

What is the effect of explicitly calling "aRequest.cancel(Cr.NS_BINDING_ABORTED)" if the error condition you detect here occurs?

Or is there another property of aRequest that we can check to make sure that it actually failed and is going to stop?

Comment 8

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8755967 [details]
Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54906/diff/3-4/

Comment 9

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/54906/#review64454

> What I'm concerned about is that if we set this._componentFailed to true but aStatus is a success code, for some reason the request in aRequest might still be running, even if we subsequently ignore it.
> 
> What is the effect of explicitly calling "aRequest.cancel(Cr.NS_BINDING_ABORTED)" if the error condition you detect here occurs?
> 
> Or is there another property of aRequest that we can check to make sure that it actually failed and is going to stop?

aRequest.cancel(Cr.NS_BINDING_ABORTED) results in a failed download as well.

Comment 10

[:Paolo Amadini]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #3)
> How can I check that a download was added to download history?

In the browser, you can check that the download is kept across restarts. In regression tests it's more complicated because you need to use Places functions, there are some uses in "test_DownloadList.js".

> Could you advise how I should go about writing a regression test for
> catching the exception?

Since you mentioned that the file not existing on the server triggers the error, maybe you can implement an HTTP 404 response among the custom HTTP handlers:

https://dxr.mozilla.org/mozilla-central/rev/ddeb0295df692695b36295177d6790e5393e1f9a/toolkit/components/jsdownloads/test/unit/head.js#785

Comment 11

[:Paolo Amadini]

Comment on attachment 8755967 [details]
Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

https://reviewboard.mozilla.org/r/54906/#review65076

r+ since apparently this unblocks bug 1109146. I'd appreciate a regression test but I realize it might take some time to get right if you're not familiar with the code. If you don't have the time please file a bug to add the test, blocking bug 825588.

Comment 12

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0008e250eb38&selectedJob=24794560

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7638ea3c616c
Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download. r=paolo

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2ed37447b7 for being the likely cause of m(bc) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32934749&repo=mozilla-inbound

Comment 15

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8755967 [details]
Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54906/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc7e282d053
Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download. r=paolo

Comment 17

[:Paolo Amadini]

Can you file the bug I mentioned in comment 11?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fc7e282d053