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 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?

(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.

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

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?

Aus, can you take a look?

Hi Fabrice, I'll take a look.

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.

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

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.

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.

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.

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.