Closed Bug 1578955 Opened 1 year ago Closed 1 year ago

downloads API: Allow extensions a way to download ignoring HTTP errors

Categories

(WebExtensions :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: nmaier, Assigned: nmaier)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Bug #1576333 will bring Firefox to parity with Chrome in regards to handling HTTP errors and cancelling downloads on HTTP errors.

A concern arose that some extensions might in fact want to continue with downloads even though the server returned an error such as 404 Not Found.

This is currently unsupported in Chrome, and implementing this in Firefox would be a new feature.

I therefore propose the following changes:

  1. Add a new, optional boolean flag allowHttpErrorsto the options of download.download(), which defaults to false.
  2. When the flag is false (the default) in calls to download.download(), then there should be no change in behavior, and downloads should continue to be cancelled upon encountering HTTP errors.
  3. When the flag is true in calls to download.download(), then upon encountering errors the download should be allowed to proceed regardless, but DownloadItem.error should be set to the appropriate InterruptReason, such as SERVER_BAD_CONTENT, as usual, and special care should be taken that any .onChanged listeners are notified of the changed DownloadItem.error value.
  4. When the flag is true in calls to download.download(), when the download finishes and changes its state to complete, the DownloadItem.error should still reflect the InterruptReason corresponding to HTTP error code encountered earlier.
  5. When the flag is true in calls to download.download() and there is a later error, such as writing the file to disk failed, then the DownloadItem.error should reflect the InterruptReason of this later error.

Preliminary patch, demonstrating the idea, tho I am not married to the details and open to suggestions.

It is supposed to stack on the patches from bug 1576333 of course.

Attachment #9091002 - Attachment description: Bug 1578955 - Add allowHttpErrors feature to downloads API r?zombie → Bug 1578955 - Add allowHttpErrors feature to downloads API r?robwu
Flags: qe-verify-
Flags: in-testsuite+
Keywords: checkin-needed

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d12f40be9fc
Add allowHttpErrors feature to downloads API r=robwu

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

The following documentation has been updated:

Please let me know if there are any changes or additions needed. Thank you!

Flags: needinfo?(maierman)
Flags: needinfo?(lgreco)
Flags: needinfo?(maierman)

Not sure what went wrong there, but fortunately I had my changes preserved in the preview page: downloads.dowload() Is now as I intended.

Flags: needinfo?(maierman)

We eventually opted to not report at all HTTP errors when allowHttpErrors is true, i.e. the errors that bug 1576333 added are ignored. Other errors, such as FILE_* and NETWORK_* errors will be reported.

I'd replace the following phrase and the listing that follows:

Errors encountered during the download are reported: [...]

with something like:

HTTP server errors such as SERVER_BAD_CONTENT will be ignored entirely and are not reported, and the download is allowed to proceed, however other errors unrelated to HTTP server errors - such as FILE_FAILED or NETWORK_FAILED - still get reported and fail the download, as usual. Using this flag e.g. allows to deliberately download server error pages.

And I'd remove the following sentence entirely:

All .onChanged listeners are notified of the changed DownloadItem.error value.

Sorry, I forgot to mention this before and created extra work.

Flags: needinfo?(maierman)

No worries, I have updated downloads.download. Please let me know if this is OK. Thanks!

Flags: needinfo?(maierman)

Looks good to me.
Thanks

Flags: needinfo?(maierman)

(clearing needinfo, Nils already provided feedback on the updated MDN api docs).

Flags: needinfo?(lgreco)
You need to log in before you can comment on or make changes to this bug.