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

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Downloads API
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jimicy, Assigned: jimicy)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Created attachment 8755882 [details]
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
Assignee: nobody → jimmyw22
Blocks: 1109146
Created 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 commit: https://reviewboard.mozilla.org/r/54906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54906/
Attachment #8755967 - Flags: review?(paolo.mozmail)

Updated

2 years ago
Attachment #8755967 - Flags: review?(paolo.mozmail)

Comment 2

2 years ago
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.
(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.
Flags: needinfo?(paolo.mozmail)
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/
Attachment #8755967 - Attachment description: 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 → Bug 1275289 - Add try catch to DownloadLegacy.js since aRequest.responseStatus does not exist in the case of a failed download;
Attachment #8755967 - Flags: review?(paolo.mozmail)
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/
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

2 years ago
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?
Attachment #8755967 - Flags: review?(paolo.mozmail)
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/
Attachment #8755967 - Flags: review?(paolo.mozmail)
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

2 years ago
(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
Flags: needinfo?(paolo.mozmail)

Updated

2 years ago
Attachment #8755967 - Flags: review?(paolo.mozmail) → review+

Comment 11

2 years ago
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.
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-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
Flags: needinfo?(jimmyw22)
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by ryanvm@gmail.com:
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

2 years ago
Can you file the bug I mentioned in comment 11?

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0fc7e282d053
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: needinfo?(jimmyw22)
Blocks: 1297728
You need to log in before you can comment on or make changes to this bug.