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

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

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.

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 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 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 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/

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.

(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 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.

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

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

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

Can you file the bug I mentioned in comment 11?