941317 - Switch Downloads.jsm to use Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Attachment: The patch

This patch converts the Downloads API to use Promise.jsm, fixing several
race conditions found by running the regression tests with the new system.

Comment 1

[Neil Deakin]

Comment on attachment 8335620 [details] [diff] [review]
The patch

>+          // The exception will be changed in the "catch" block if this is a
>+          // cancellation request rather than a block.
>           throw new DownloadError({ becauseBlockedByReputationCheck: true });

The comment is a bit confusing here as you uses two different definitions of the word 'block'. (code block and blocked by reputation). Maybe just say: If this is actually a cancellation, this exception will be changed in the catch block below.


>   /**
>+   * True after the "execute" method of the saver has been called.
>+   */
>+  _saverExecuting: false,

The comment says 'after' but you actually set it before the execute is called.


>       // The main request has just started.  Wait for the associated Download
>       // object to be available before notifying.
>       this._deferDownload.promise.then(download => {
>+        // If the request was blocked, now that we have the download object we
>+        // should set a flag that can be retrieved later when handling the
>+        // cancellation so that the proper error can be thrown.
>+        if (blockedByParentalControls) {
>+          download._blockedByParentalControls = true;
>+        }

This falls through and calls all of onTransferStarted, setting the referrer and adding to the history. The old code doesn't do this, but maybe this change is intentional?


>-    do_throw("The download should have been canceled.");
>+    // If we get here, it means that the first attempt actually succeeded.  In
>+    // fact, this could be a valid outcome, because the cancellation request may
>+    // not have been processed in time, before the download finished.
>+    do_print("The download should have been canceled.");

Remove the comma between 'time' and 'before'.


I'm assuming these changes are all bugs that could also occur with the existing promises as well?

Comment 2

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Neil Deakin from comment #1)
> >+        if (blockedByParentalControls) {
> >+          download._blockedByParentalControls = true;
> >+        }
> 
> This falls through and calls all of onTransferStarted, setting the referrer
> and adding to the history. The old code doesn't do this, but maybe this
> change is intentional?

Yes, basically this is because it makes little difference, and keeping the previous behavior would add more unneeded complexity. It is probably good to set the referrer for blocked downloads anyways.

> I'm assuming these changes are all bugs that could also occur with the
> existing promises as well?

Yes, the only change that was not a pre-existing race condition was the handling of Windows Parental Controls blocking, that worked correctly assuming synchronous resolution of promises.

Comment 3

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/1e8f83c69e80

Comment 4

[Carsten Book [:Tomcat]]

had to backout this change in https://hg.mozilla.org/integration/fx-team/rev/b4284cae2bff because of test failures after this checkin like https://tbpl.mozilla.org/php/getParsedLog.php?id=31373611&tree=Fx-Team

Comment 5

[:Paolo Amadini]

It seems that B2G tests don't run on debug builds, so the debug tryserver run didn't detect the failures.

The most probable reason for the failure is that B2G tests (the recently landed DOM Downloads API) make assumptions about the order of resolution of promises.

Fabrice, can you or someone familiar with the DOM Downloads API look into these B2G failures, after applying the patch in this bug?

Comment 6

[[:fabrice] Fabrice Desré]

Aus, can you take a look?

Comment 7

[Aus Lacroix [:aus]]

Hi Fabrice,

I'll take a look.

Comment 8

[Aus Lacroix [:aus]]

Once 947167 is resolved I'll be landing the patch from the original author as is. That's the only issue that's preventing us from closing this out.

Comment 9

[Aus Lacroix [:aus]]

Will try and land this in the coming day or two.

Comment 10

[Aus Lacroix [:aus]]

I think there's something wrong with the patch that switches to using Promise.jsm. The tests themselves make the correct assumptions, imo. The failures stem from clearAllDone() clearing the downloads as expected but the fresh list that's fetched includes a download that's "succeeded" which it should not.

Comment 11

[:Paolo Amadini]

Is it possible that previous tests scheduled an addition without waiting for it to be processed, and it is now processed just after the list is cleared instead of just before?

If the DOM API tests seem to do things correctly, and you can reduce the failure to one simple test case, I can add it to the test suite of Downloads.jsm.

Comment 12

[Aus Lacroix [:aus]]

It's possible. I'll try and write a paired down test that shows that failure and nothing else. I'll post a patch with the test to this bug.

Comment 13

[:Paolo Amadini]

Now that the test in bug 947167 is disabled, the tryserver build looks green on B2G:

https://tbpl.mozilla.org/?tree=Try&rev=b0a0f7ad360f

I've pushed the change to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/00e095a5bf18

This change in the underlying Promise implementation will probably make it easier to debug the front-end test in bug 947167.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00e095a5bf18