Closed Bug 941317 Opened 6 years ago Closed 6 years ago

Switch Downloads.jsm to use Promise.jsm

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
1.4 S4 (28mar)

People

(Reporter: Paolo, Assigned: aus)

References

(Blocks 1 open bug)

Details

(Whiteboard: [systemsfe][p=5])

Attachments

(1 file, 1 obsolete file)

Attached patch The patch (obsolete) — Splinter Review
This patch converts the Downloads API to use Promise.jsm, fixing several
race conditions found by running the regression tests with the new system.
Attachment #8335620 - Flags: review?(enndeakin)
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?
Attachment #8335620 - Flags: review?(enndeakin) → review+
Attached patch Updated patchSplinter Review
(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.
Attachment #8335620 - Attachment is obsolete: true
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?
Flags: needinfo?(fabrice)
Aus, can you take a look?
Flags: needinfo?(fabrice) → needinfo?(aus)
Blocks: 947167
Hi Fabrice,

I'll take a look.
Assignee: paolo.mozmail → aus
Flags: needinfo?(aus)
No longer blocks: 947167
Blocks: 947167
Flags: needinfo?(aus)
Whiteboard: [systemsfe]
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.
Flags: needinfo?(aus)
Whiteboard: [systemsfe] → [systemsfe][p=5]
Target Milestone: --- → B2G C4 (2jan on)
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.
https://hg.mozilla.org/mozilla-central/rev/00e095a5bf18
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: B2G C4 (2jan on) → mozilla30
Target Milestone: mozilla30 → 1.4 S4 (28mar)
Blocks: 996671
No longer blocks: 856878
You need to log in before you can comment on or make changes to this bug.