1645337 - downloads.download should not unconditionally swallow all errors from the internal Download API

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[Rob Wu [:robwu]]

Someone wanted to contribute a patch in the downloads API implementation, but did not get any useful error messages (despite there being apparent errors in the patch): https://phabricator.services.mozilla.com/D66732#2395719

There are no error messages because the ext-downloads.js implementation unconditionally swallows all errors. This is wrong and makes debugging harder. The unconditional .catch(() => {}) was added in bug 1576333. If there is any need for swalling some errors, then the error should be checked, and unexpected errors should be thrown or logged.

Comment 1

[Harsh]

Hi Rob,
I would like to work on this.
What are some of the known errors that need to be not logged?
Maybe we can check if the error needs to be logged or not and then either swallow them or log them using Cu.reportError(); ?

Comment 2

[Rob Wu [:robwu]]

At least one known error that I'm aware of is the one from when a request is cancelled (as you might have noticed while working on bug 1579911).

There are roughly two ways to discover other potential errors of interest:

1. Reading the source code, starting from the start() method at https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/toolkit/components/downloads/DownloadCore.jsm#323-585
   This requires reading lots of code. While it may be a good exercise in getting familiar with Searchfox, it could be a bit too much.
2. Catch when .start() results in a rejected promise and report the error and run unit tests that use downloads.download() (and watch the log output for your reported errors).
   Or not even automated tests, but install an extension that uses the downloads.download API and watch the console while manually testing the downloads.download API.

We should find a good balance between code complexity and filtering errors of interest. Common errors (e.g. download error due to aborted request or network issues) should be filtered, anything else is probably fine.

Comment 3

[agentcoulsonphil3]

Hi, I was going through some good-first bugs and stumbled upon this. I think this might be an incredibly beginner level question, but could you please explain a bit more about the bug? I went through the above comments and couldn't quite understand what needs to be done here.

Comment 4

[Rob Wu [:robwu]]

Harsh has already expressed interest in taking this patch, and he already has some background on the bug since the bug came up while working on another feature. Therefore I'll assign this bug to him.

If you are interested in contributing, I am willing to mentor you on another bug. There are plenty of beginner-friendly bugs, even if they are not marked as such. Bug 1640665 for example. Feel free to comment on that bug or send me an e-mail if you have questions.

Comment 5

[Harsh]

I have been tracking all the rejections that the start method returns (like these: https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/toolkit/components/downloads/DownloadCore.jsm#359-363,367-371). How can I use this to differentiate between different errors? I mean I know we can catch them using download.start().catch((e) => { /* Something here */ }); , but how can we use that to filter them? Considering the fact that most of these errors are being thrown in the form of a message like this message: "Cannot start after finalization." ?

Comment 6

[Rob Wu [:robwu]]

I suggest to look at the members of the DownloadError "class". name is an attempt to mimic the standard name property (which defaults to the name of the function or class, but in this case an anonymous function was used, so the code had to explicitly override the name property).

You need to double-check, but I guess that DownloadError instances often show expected errors that are already handled elsewhere in the downloads.download extension API. So it would probably be good to unconditionally filter all DownloadErrors. We should only report unexpected errors that are actionable. The purpose of this bug is to make sure that unexpected errors are not silently swallowed.

Comment 7

[Harsh]

I looked at the above mentioned links, and I think that any error which isn't a DownloadError will be an unexpected error that we need to show.
I am thinking to add this in order to handle this bug.

download.start().catch((e) => { 
     if(e.name!=="DownloadError"){
             Cu.reportError(e);
     } 
}); 

What are your thoughts on this?

Comment 8

[Rob Wu [:robwu]]

Did you verify that it also prevents unwanted errors from appearing when a download is aborted?

If it does, then the proposed change looks good to me.

Comment 9

[Harsh]

I installed an extension, in which I used downloads.cancel immediately after calling the downloads.download, in order to simulate an aborted download.

I don't get any error messages in the browser console which I used for inspecting my extension.
However I did receive JavaScript error: resource://gre/modules/DownloadCore.jsm, line 2222: NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIStreamListener.onDataAvailable] , on the cmd from which I ran the mach run command.

Is this one of the unexpected errors we don't want to see, or is this normal due to my extension aborting a download?

Comment 10

[Rob Wu [:robwu]]

I can't really tell without the stack trace. But if that error only appeared when you added Cu.reportError, then yes, that would be one of the errors that we want to hide.

If you aren't sure about the source of the error, consider (temporarily) prepending a prefix to the error message to make it easier to identify the source of the error.

Comment 11

[Harsh]

I attached a prefix as you said and this error wasn't being reported through my Cu.reportError().
I will be attaching a patch to this soon.

Comment 12

[Harsh]

Attachment: Bug 1645337 - downloads.download should not unconditionally swallow all errors from the internal Download API. r=robwu

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/407d85a8cdc0
downloads.download should not unconditionally swallow all errors from the internal Download API. r=robwu

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/407d85a8cdc0