1245602 - Implement chrome.downloads.{pause,resume,cancel}

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

Part of supporting chrome.downloads in WebExtensions

https://developer.chrome.com/extensions/downloads#method-pause
https://developer.chrome.com/extensions/downloads#method-resume
https://developer.chrome.com/extensions/downloads#method-cancel

Comment 1

[Andrew Swan [:aswan]]

At the same time we implement these methods, complete downloads.onChanged [1] handling for state, paused, canResume, and error.

[1] https://developer.chrome.com/extensions/downloads#event-onChanged

Comment 2

[Andrew Swan [:aswan]]

Paolo, I glossed over the handling of pause/resume in the first few chrome.downloads patches.  Looking a little more closely, I have a few questions, it would be helpful to get your feedback to make sure I'm not overlooking or misunderstanding anything.

First, the Chrome documentation defines DownloadItem.paused as:
> True if the download has stopped reading data from the host, but kept the connection open.
From my reading of the toolkit code, it doesn't look like we have anything analogous?  Assuming I've got that right, we need to either re-define (and clearly document) what paused means in FF, or just omit paused from DownloadItem or hard-code it to always be false.

More generally, it looks like the only way to really implement pause is to start a download with |tryToKeepPartialData| set to true, then to map pause to |download.cancel()| and resume to |download.start()|?  Is there ever a reason to start a download without having |tryToKeepPartialData| set to true?

Comment 3

[Andrew Swan [:aswan]]

(whoops meant to set this on the previous comment)

Comment 4

[Andrew Swan [:aswan]]

Got details from Paolo in person, cancelling a download while keeping partial data is indeed our only technique for pause/resume.  Proceeding with that...

Comment 5

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1245602 - Implement chrome.downloads.pause(), .resume(), .cancel() r?kmag

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

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8729350 [details]
MozReview Request: Bug 1245602 - Implement chrome.downloads.pause(), .resume(), .cancel() r?kmag

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

Comment 7

[Andrew Swan [:aswan]]

Fixed two goofy little things I found after posting the review.
Not-all-that-interesting fact of the evening: mach eslint doesn't check .sjs files.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8729350 [details]
MozReview Request: Bug 1245602 - Implement chrome.downloads.pause(), .resume(), .cancel() r?kmag

https://reviewboard.mozilla.org/r/39401/#review36241

::: toolkit/components/extensions/ext-downloads.js:64
(Diff revision 2)
>        return "complete";
>      }
>      if (this.download.stopped) {
> +      // There isn't a "haven't started yet state", so use complete
> +      if (!this.download.startTime) {
> +        return "complete";

This doesn't seem right. I'd go with "in_progress" or "interrupted", but I'm not sure which would be better.

::: toolkit/components/extensions/ext-downloads.js:71
(Diff revision 2)
>        return "interrupted";
>      }
>      return "in_progress";
>    }
> +  get paused() {
> +    return this.download.stopped && this.download.hasPartialData;

This doesn't seem right. The Chrome docs say that "paused" means that there's still a connection, but we're not transferring data.

If we want to try to mimic it without implementing that functionality, this should probably be something like `this.download.canceled && this.download.hasPartialData && !this.download.error`

::: toolkit/components/extensions/ext-downloads.js:507
(Diff revision 2)
> +            return Promise.reject({message: `Invalid download id ${id}`});
> +          }
> +          if (item.download.succeeded) {
> +            return Promise.reject({message: `Download ${id} is already complete`});
> +          }
> +          let promise = item.download.stopped ? Promise.resolve() : item.download.cancel();

The `.cancel()` method is documented to have no effect if the download is already stopped, so this check shouldn't be necessary.

::: toolkit/components/extensions/ext-downloads.js:508
(Diff revision 2)
> +          }
> +          if (item.download.succeeded) {
> +            return Promise.reject({message: `Download ${id} is already complete`});
> +          }
> +          let promise = item.download.stopped ? Promise.resolve() : item.download.cancel();
> +          return promise.then(() => item.download.removePartialData());

I think we should be using `item.download.finalize(true)` rather than `.cancel().then(remove)`.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:100
(Diff revision 2)
> +    } else {
> +      // extension functions throw on bad arguments, we can remove the extra
> +      // promise when bug 1250223 is fixed.
> +      Promise.resolve().then(() => {
> +        let fn = browser.downloads[what];
> +        let args = Array.prototype.slice.call(arguments, 1);

Just replace `function(msg)` with `function(msg, ...args)`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:101
(Diff revision 2)
> +      // extension functions throw on bad arguments, we can remove the extra
> +      // promise when bug 1250223 is fixed.
> +      Promise.resolve().then(() => {
> +        let fn = browser.downloads[what];
> +        let args = Array.prototype.slice.call(arguments, 1);
> +        return fn.apply(browser.downloads, args);

`return browser.downloads[what](...args)`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:213
(Diff revision 2)
> -  yield setup(backgroundScript);
> +    {type: "onCreated", data: {id}},
> +  ]);
> +  is(msg.status, "success", "got created and changed events");
>  
> -  let msg = yield runInExtension("download", {url: TXT_URL});
> -  is(msg.status, "success", "downoad succeeded");
> +  // wait for some data to come.
> +  yield waitForProgress(INTERRUPTIBLE_URL, INT_PARTIAL_LEN);

You should probably call `waitForProgress` before you call `.download()`, and then `yield` on it here. If you don't, the progress event could happen before you add your listener. The same goes for the calls below.

Comment 9

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/39401/#review36241

> This doesn't seem right. The Chrome docs say that "paused" means that there's still a connection, but we're not transferring data.
> 
> If we want to try to mimic it without implementing that functionality, this should probably be something like `this.download.canceled && this.download.hasPartialData && !this.download.error`

Yeah this was covered briefly in https://bugzilla.mozilla.org/show_bug.cgi?id=1245602#c2
I'm having a hard imagining how somebody using this property would care about the distinction between pause being implemented by holding the connection open versus closing the connection and using range requests to resume, so I think we should just document this distinction for the pedants and go with this implementation.
Agreed on the new condition though...

Comment 10

[Andrew Swan [:aswan]]

Ugh, while responding to Kris's review comments I got into a bit of a mess, when we cancel a download there is a brief interval before removePartialData() gets called when the download gets into a state that looks like paused (ie, stopped but still with some partial data for the moment).  I don't want to generate onChanged events that make it look like the download is paused before canceling, but still working out how to do that in a reasonable way...

Comment 11

[Andrew Swan [:aswan]]

Following up on the last comment, I'm now thinking there isn't any reliable way to distinguish the "pause" and "cancel" cases.  In addition to finalize [1] the UI has similar logic [2].  Paolo: this looks like a race condition (in both cases), since we don't serialize the remove after the cancel, we could do the remove, but some additional data could trickle in after that and either re-create the file or throw an exception or something?

Meanwhile, I'm inclined to file a bug for the quirky behavior on cancel and land this with the quirk.

[1] https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/toolkit/components/jsdownloads/src/DownloadCore.jsm#1003
[2] https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/browser/components/downloads/DownloadsViewUI.jsm#290

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8729350 [details]
MozReview Request: Bug 1245602 - Implement chrome.downloads.pause(), .resume(), .cancel() r?kmag

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

Comment 13

[:Paolo Amadini]

(In reply to Andrew Swan [:aswan] from comment #11)
> Following up on the last comment, I'm now thinking there isn't any reliable
> way to distinguish the "pause" and "cancel" cases.

So, bug 1256243 is about the case you're describing where the front-end is briefly notified with a state that may seem like a paused download, both for the "cancel" and "pause" cases. For the former, the download will clearly become canceled in the end.

I agree this intermediate state is logically unexpected, on the other hand we don't particularly care about this behavior for the Desktop front-end, the transition is very brief and for the user it seems just like a normal cancel transition. I'm not sure what the expectations of WebExtensions are, but I'd wait to call this a serious issue until an actual extension ported to Firefox is misbehaving.

In general, in Downloads.jsm we explicitly make no guarantees that you won't get multiple "onchange" events in a row where only part of the properties change, or even no property at all is different. I don't see such guarantee being made by the WebExtension API as well, but it may be implicitly relied upon. As always when handling compatibility, we'll never get 100% identical behavior and timing, so I think we should focus on the issues that have a potential to affect the real consumers the most.

> In addition to finalize
> [1] the UI has similar logic [2].  Paolo: this looks like a race condition
> (in both cases), since we don't serialize the remove after the cancel, we
> could do the remove, but some additional data could trickle in after that
> and either re-create the file or throw an exception or something?

To avoid race conditions, you should always call "removePartialData" immediately after "cancel" without waiting for the first to resolve the returned promise. This is because you don't want to have code executing between the two calls that may cause the download to restart.

We considered having a "cancel({ removePartialData: true })" option because the practice above is admittedly confusing, though this clashes with "cancel" being specified as having no effect at all if the download is already canceled. In other words, if we keep the latter definition, your call to "cancel({ removePartialData: true })" might have no effect if someone else called "cancel()" already in the meantime.

With the current solution of calling both "cancel" and "removePartialData", in the same conditions the call to "cancel" will have no effect, but "removePartialData" will still take effect.

To understand some of these choices, keep in mind that the Downloads.jsm API was designed to have front-end user interactions that can happen at any time co-exist with automated actions on the same downloads from add-ons, all with purely asynchronous controls.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8729350 [details]
MozReview Request: Bug 1245602 - Implement chrome.downloads.pause(), .resume(), .cancel() r?kmag

https://reviewboard.mozilla.org/r/39401/#review36549

::: toolkit/components/extensions/ext-downloads.js:70
(Diff revisions 2 - 3)
>    }
>    get paused() {
> -    return this.download.stopped && this.download.hasPartialData;
> +    return this.download.canceled && this.download.hasPartialData && !this.download.error;
>    }
>    get canResume() {
> -    return this.download.stopped && this.download.hasPartialData;
> +    return this.download.canceled && this.download.hasPartialData && !this.download.error;

I think we should probably just keep the `this.download.stopped && this.download.hasPartialData` check for `canResume`.

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8729350 [details]
MozReview Request: Bug 1245602 - Implement chrome.downloads.pause(), .resume(), .cancel() r?kmag

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

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/3cb1c3284e5fb5e737769a0d54c072709ffa822b
Bug 1245602 - Implement chrome.downloads.pause(), .resume(), .cancel() r=kmag

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/3cb1c3284e5f