1068664 - Add new API for downloads held temporarily in a blocked state with partial data

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

This bug is about adding support to Downloads.jsm for downloads held temporarily in a blocked state, with partial data still present on disk until the user makes a decision on how to handle them.

Some things to take into account:
- Some clients may not support displaying a UI for unblocking those downloads, and we don't want to keep the data on disk in that case, unless we can make sure we delete it promptly in all cases. We may assimilate those to failed downloads with partial data, but we may need to use a different error than "becauseBlocked", as some consumers may make assumptions and consider it a final state with no data (instead of specifically checking whether there is partial data).
- Blocked downloads with partial data will need to be persisted across sessions.
- A special API or a variant of the "start" API is needed to indicate the intention of unblocking the download.
- This system will need to be applied to the "legacy" and "copy" saver types.
- We may have different reasons for the temporary block.

Comment 1

[:Paolo Amadini]

Hey Steven, you may also want to take a look at bug 1053645 as you're working on this. The solution in this bug should prevent restart from happening automatically.

When you've familiarized yourself with the code or feel ready to ask any questions, let me know and we can evaluate the possible solutions.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Hi Steven,
  I'm assigned to bug 1068656 - do you have a WIP you can post?  That might help me get started.

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: Patch 1 - [WIP] Introduce API for downloads in a blocked state with file on disk

Here is a WIP that introduces some API that can be used to unblock / confirm a block of reputation blocked downloads. I believe this allows for the proposed use cases, and the blocked files will only be kept on disk if at least one view of the DownloadList requests it, so if no consumer is able to handle these they will just be blocked and deleted as before.

Currently this leaves the file on disk with its final filename before it is unblocked/deleted. This could be dangerous so we might want to quarantine the files somehow while they exist in this state.

Some suggestions so far include sticking in a quarantine folder, renaming back to the .part name, not renaming to the final name before blocking (if possible), and marking the file as hidden. If anyone has suggestions for what approach to take that would be great.

Comment 4

[Steven MacLeod [:smacleod]]

(In reply to :Paolo Amadini from comment #1)
> Hey Steven, you may also want to take a look at bug 1053645 as you're
> working on this. The solution in this bug should prevent restart from
> happening automatically.
I'll note that the attached patch deals with the restarting problem (Bug 1053645 ). Since information about reputation blocks was only present inside the Download.error object, serialization was throwing it away (Only the error message is serialized for the store, not the because* properties).

Since this patch introduces a new property on the Download object to track the block, which we serialize for the store, we no longer hit this problem.

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 8505750 [details] [diff] [review]
Patch 1 - [WIP] Introduce API for downloads in a blocked state with file on disk

Paolo and I did a review over Vidyo and discussed what to do moving forward. Clearing the f? until I update the patch.

Comment 6

[:Paolo Amadini]

Steven, did you do any work already on the missing deserialization of "error"?

We'll need that resolved before the Android migration in bug 1063217 and bug 901360 can fully proceed, if you have something I wonder if you could split the work into a separate bug blocking bug 901360.

Comment 7

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: bz://1068664/smacleod

Comment 8

[Steven MacLeod [:smacleod]]

/r/135 - Bug 1068664 - Introduce API for downloads in a blocked state with file on disk.

Pull down this commit:

hg pull review -r 51175b1411f0eee1f9dc2f25865fd3894820000b

Comment 9

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/133/#review49

I'm still working on moving the reputation checking inside of the Savers
themselves so that we don't rename to the final file before checking if
we should block - I thought I should toss up the code that is working
currently though.

This commit includes the error serialization into a new property so that
previous versions will keep it around in unknown properties.


Paolo, in the case of a download that doesn't use a part file, how do we
want to handle the blocking? are we fine just having it use the final
filename on disk until someone unblocks / confirmBlocks?

Comment 10

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/133/#review51

Looks good, for now I've only reviewed the DownloadError serialization part. Can you please split it into another bug so we can land it separately?

I'll do a detailed review of the rest of the blocking logic when it's moved to the DownloadSaver.

(In reply to Steven MacLeod [:smacleod] from comment #9)
> Paolo, in the case of a download that doesn't use a part file, how do we
> want to handle the blocking? are we fine just having it use the final
> filename on disk until someone unblocks / confirmBlocks?

If I remember correctly, this type of download doesn't trigger reputation checks, so it would be indifferent. In case we have this condition triggered inside a test case, we can use the final name if this is simpler.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
(Diff revision 1)
>  const kSerializableDownloadProperties = [

Maybe we should rename this to kPlainSerializableDownloadProperties or something for clarity.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
(Diff revision 1)
> -    if (this.error && ("message" in this.error)) {
> -      serializable.error = { message: this.error.message };
> +    if (this.error) {
> +      serializable.errorObj = this.error.toSerializable();
>      }

Currently, "this.error" can be a DownloadError or any other object (for example, an OS.File error).

There are two things we can do:
- Serialize only the message at this point, so it will be deserialized as a DownloadError in other sessions.
- Ensure we cast any other error type to a DownloadError when we assign the property (preserving just the message), and add a non-serializable innerException property to the DownloadError object for debugging.

From the point of view of what we end up serializing, the result should be the same, what is different is what consumers see in the .error property (and the rejection returned by "start"). I don't have a strong preference for one or the other.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
(Diff revision 1)
> -      if (property != "error" && property != "startTime" && this[property]) {
> +      if (property != "startTime" && this[property]) {

It looks like startTime is not in kSerializableDownloadProperties anymore and can be removed as well.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
(Diff revision 1)
> +      becauseBlockedByReputationCheck: this.becauseBlockedByReputationCheck

nit: comma even in last line of object initializer

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
(Diff revision 1)
> +#ifdef XP_MACOSX
> +      return false;
> +#elifdef XP_WIN
> +      return false;
> +#elifdef XP_UNIX
> +      return false;

Drive-by comment, unrelated to the DownloadError review:

I'm always confused by these definitions, but looking at other parts of the file, it seems XP_UNIX includes B2G and Android, that should be excluded using MOZ_WIDGET checks (I don't know if there is a better way).

Also, nit: does this syntax work here?

#if defined(XP_WIN) || defined(XP_UNIX) || ...

Comment 11

[Steven MacLeod [:smacleod]]

/r/135 - Bug 1068664 - Introduce API for downloads in a blocked state with file on disk. r=paolo
/r/641 - Bug 1068664 - Move reputation checking into the saver. r=paolo

Pull down these commits:

hg pull review -r 7d7ba2a9c165ac06e238418e8199de625505e21e

Comment 12

[Steven MacLeod [:smacleod]]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c846e7fa73e1

Comment 13

[:Paolo Amadini]

Comment on attachment 8514980 [details]
MozReview Request: bz://1068664/smacleod

Sorry for the delay, I spent quite some time on the review but unfortunately at this point I still have only general feedback. I still find it difficult to confidently track changes using ReviewBoard, especially when multiple commits and iterations are involved, and the code has may edge cases to take into account.

The good thing is that it looks you took care of all the edge cases correctly, even improving the existing state. There are some things I found by skimming over the patches, required for the next iteration:

- This definitely requires new tests for the new state and its interactions with the other possible state changes (cancellation, presence of partial data, request not to save partial data, legacy download flow).
- Try to move code that looks the same into reused functions.
- Please fold the two patches into a single attachment (it's been useful to look at them separately, but for the next pass it will make things simpler for me).
- Please use a traditional attachment for the next iteration.

Comment 14

[Steven MacLeod [:smacleod]]

Attachment: Patch - Introduce API for downloads in a blocked state with file on disk

This new patch has all the changes folded together and introduces tests. I've also refactored some of the common code in the two saver implementations.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5d49b37087ee

Comment 15

[Steven MacLeod [:smacleod]]

Attachment: Patch - Introduce API for downloads in a blocked state with file on disk v2

Paolo, this new patch should have all the fixes we discussed in Portland.

Comment 16

[:Paolo Amadini]

Comment on attachment 8533363 [details] [diff] [review]
Patch - Introduce API for downloads in a blocked state with file on disk v2

Review of attachment 8533363 [details] [diff] [review]:
-----------------------------------------------------------------

Fantastic job. There's still more work to do, but we've gone a long way since the first version!

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +241,5 @@
>  
>    /**
>     * Indicates whether, at this time, there is any partially downloaded data
>     * that can be used when restarting a failed or canceled download.
> +

nit: asterisk

@@ +261,5 @@
> +   * Indicates whether, at this time, there is any data that has been blocked.
> +   * Since reputation blocking takes place after the download has fully
> +   * completed a value of true also indicates 100% of the data is present.
> +   */
> +  hasBlockedData: false,

This new property should be serialized across sessions.

@@ +533,5 @@
> +   * @return {Promise}
> +   * @resolves When the steps to take after success have completed.
> +   * @rejects  JavaScript exception if any of the operations failed.
> +   */
> +  _succeed: Task.async(function* task_D_succeed() {

nit: feel free to omit function names in new code (cleaning up older code will be done later)

@@ +569,5 @@
> +   * indefinitely.
> +   */
> +  _promiseConfirmBlock: null,
> +
> +

nit: only one blank line

@@ +583,5 @@
> +   */
> +  unblock: function() {
> +    if (this._promiseUnblock) {
> +      return this._promiseUnblock;
> +    }

This can race with _promiseConfirmBlock (and the other way around), should add a check for that.

Should add two tests that call both functions in succession without waiting, in the two possible orders.

@@ +586,5 @@
> +      return this._promiseUnblock;
> +    }
> +
> +    if (this.succeeded || !this.stopped || !this.hasBlockedData ||
> +        !this.error || !this.error.becauseBlockedByReputationCheck) {

We should just check "!this.hasBlockedData" and remove all the other tests.

@@ +595,5 @@
> +
> +    this._promiseUnblock = Task.spawn(function task_D_unblock() {
> +
> +      yield OS.File.move(this.target.partFilePath,
> +                         this.target.path);

We should add some logic in order not to get stuck. So, in case this move fails:
- Invoke the "refresh" method;
- Set this._promiseUnblock to null so that the operation can be retried or can fail as appropriate.

The "refresh" method should do this when "this.hasBlockedData" is true:
- If this.target.partFilePath does not exist or the check fails, set this.hasBlockedData to false.

We should add a test for the most common case, that is "unblock" is called but the part file has been removed in the meantime.

@@ +603,5 @@
> +    this.succeeded = true;
> +    this.hasPartialData = false;
> +    this.hasBlockedData = false;
> +    this.progress = 100;
> +    this._notifyChange();

This should only be done after OS.File.move succeeds. Also, don't touch this.hasPartialData and this.progress as we should have set them already when the download was blocked.

@@ +624,5 @@
> +      return this._promiseConfirmBlock;
> +    }
> +
> +    if (this.succeeded || !this.stopped || !this.hasBlockedData ||
> +        !this.error || !this.error.becauseBlockedByReputationCheck) {

Also here, we should just check "!this.hasBlockedData" and remove all the other tests.

@@ +633,5 @@
> +
> +    this._promiseConfirmBlock = OS.File.remove(this.target.partFilePath);
> +    this.hasPartialData = false;
> +    this.hasBlockedData = false;
> +    this._notifyChange();

No need to touch this.hasPartialData.

With regard to setting hasBlockedData to false, it should probably happen only if the removal succeeds, while if it fails we should fall back to the failure mode of "unblock".

You might want to implement the two "unblock" and "confirmBlock" functions through a parameterized one internally, if it makes sense.

@@ +1968,5 @@
> +   * @return {Promise}
> +   * @resolves When the reputation check and cleanup is complete.
> +   * @rejects DownloadError if the download should be blocked.
> +   */
> +  _checkReputation: Task.async(function* task_DCS_checkReputation() {

Call this _checkReputationAndMove ?

@@ +1980,5 @@
> +      // download did not use a partial file path, meaning it
> +      // currently has its final filename.
> +      if (!DownloadIntegration.shouldKeepBlockedData() || !partFilePath) {
> +        try {
> +          download.hasPartialData = false

Should set these before the "if shouldKeepBlockedData" block:

download.progress = 100;
download.hasPartialData = false;

@@ +1987,5 @@
> +          Cu.reportError(ex);
> +        }
> +      }
> +
> +      download.hasBlockedData = true;

This should be in an "else" block, in other words "hasBlockedData" should be false if we removed the file.

You should add a test for this in one of the existing normal blocked download test functions.

@@ +1989,5 @@
> +      }
> +
> +      download.hasBlockedData = true;
> +      throw new DownloadError({ becauseBlockedByReputationCheck: true });
> +    } else if (partFilePath) {

style nit: no "else" when the true block always returns or throws.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +182,5 @@
> +   *
> +   * If a download is blocked and the partial data is kept the Download's
> +   * 'hasBlockedData' property will be true. In this state Download.unblock()
> +   * or Download.confirmBlock() may be used to either unblock the download or
> +   * remove the download respectively.

nit: "remove the downloaded data"

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +1615,5 @@
> +  do_check_false(download.succeeded);
> +  do_check_true(download.hasPartialData);
> +  do_check_true(download.hasBlockedData);
> +  do_check_true(yield OS.File.exists(download.target.partFilePath));
> +  do_check_false(yield OS.File.exists(download.target.path));

All the function up to this point, common to three tests, should be moved into a helper function.

Comment 17

[Steven MacLeod [:smacleod]]

Attachment: Patch - Introduce API for downloads in a blocked state with file on disk v3

Comment 18

[:Paolo Amadini]

Comment on attachment 8537616 [details] [diff] [review]
Patch - Introduce API for downloads in a blocked state with file on disk v3

Review of attachment 8537616 [details] [diff] [review]:
-----------------------------------------------------------------

Hurray! There are a couple of last minute comments, but looks quite good otherwise.

Thanks for all the work!

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +616,5 @@
> +        throw ex;
> +      }
> +
> +      this.succeeded = true;
> +      this.hasBlockedData = false;

We may want to do this.error = null, although this would leave no trace that the download was previously blocked. We're probably not displaying any "was blocked" indicator in the UI anyways, so it's not a big deal. What do you think?

We need to add a line to test this after unblocking, either way.

@@ +652,5 @@
> +
> +    this._promiseConfirmBlock = Task.spawn(function* () {
> +      try {
> +        yield OS.File.remove(this.target.partFilePath);
> +      } catch (e) {

nit: ex

@@ +912,5 @@
> +          }
> +
> +          // Ignore the result if the state has changed meanwhile.
> +          if (!this.stopped || this._finalized) {
> +            return;

This repeated check is designed to avoid races with progress properties being updated by a newly started saver. Thus, it should be done after the asynchronous OS.File call, but before trying to update currentBytes and other properties.

The same applies to changing hasBlockedData / hasPartialData in the catch block - we shouldn't change those if the download has been restarted or finalized in the meantime.

@@ +1120,5 @@
>    "tryToKeepPartialData",
>    "launcherPath",
>    "launchWhenSucceeded",
>    "contentType",
> +  "hasBlockedData",

nit: place this after hasPartialData (same order as source file).

Comment 19

[Steven MacLeod [:smacleod]]

(In reply to :Paolo Amadini from comment #18)
> ::: toolkit/components/jsdownloads/src/DownloadCore.jsm
> @@ +616,5 @@
> > +        throw ex;
> > +      }
> > +
> > +      this.succeeded = true;
> > +      this.hasBlockedData = false;
> 
> We may want to do this.error = null, although this would leave no trace that
> the download was previously blocked. We're probably not displaying any "was
> blocked" indicator in the UI anyways, so it's not a big deal. What do you
> think?

Yeah I purposefully kept error around to keep a trace that the download was
previously blocked. According to design in Bug 1053890 [1] there should be
an indicator.

Either way, keeping error around is probably a silly way to indicate the
download was previously blocked. Do you have any ideas for something better?
Or, do you still think it's not a big deal and we should just not bother?

[1] https://bug1053890.bugzilla.mozilla.org/attachment.cgi?id=8481334 Page 11.

Comment 20

[:Paolo Amadini]

(In reply to Steven MacLeod [:smacleod] from comment #19)
> According to design in Bug 1053890 [1] there should be an indicator.

Ah, missed that. I actually think it's only moderately useful, we actually might end up not doing exactly that, depending on how complex it will be to implement and maintain the icon.

> Either way, keeping error around is probably a silly way to indicate the
> download was previously blocked. Do you have any ideas for something better?
> Or, do you still think it's not a big deal and we should just not bother?

Not off the top of my head. On one hand I wouldn't want to add too many different properties to the interface, on the other hand having both "succeeded" and "error" set to true seems weird, and may confuse some consumers (although it may only happen if they have opted in to keeping blocked data).

Let's keep the current implementation for now, adding a check and a comment in the test, and file a bug to sort this out. We can always change the interface until we opt-in to the feature in Desktop.

Comment 21

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/integration/fx-team/rev/4f4c2f1108cc

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f4c2f1108cc

Comment 23

[Eric Shepherd [:sheppy]]

The page that needs to be updated is this one: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm

I will look this over and see what specifically needs to be done.