Cannot restart a download during the onDownloadCancelled event

VERIFIED FIXED in mozilla6

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla6
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
If a download is cancelled and then one of the listeners calls install() to restart the download from the onDownloadCancelled event then things go wrong. I think mostly because we delete the temporary file after calling onDownloadCancelled but that may be the new temporary file for the download.
(Assignee)

Comment 1

8 years ago
Also we send onDownloadCancelled before the stream actually closes and dispatches onStopRequest.

Neither of these are likely to cause problems for users so I don't think it is worth blocking anything on a fix. Tests can use executeSoon to workaround it.
(Assignee)

Comment 2

8 years ago
Created attachment 513355 [details] [diff] [review]
patch rev 1

Basic problem is that immediately after we call onDownloadCancelled we attempt to delete the temporary file (kind of want it to be available during onDownloadCancelled) and then onStopRequest doesn't happen until a time after that meaning the channel and things aren't cleaned up properly.

This is a simple way around that. If startDownload sees that a download is already in progress then we set a flag to say we want to start the download when it finally wraps itself up. In onStopRequest if the request was cancelled and the flag is set then the new download is started.

I can't see any other way into startDownload when a channel exists except for the cancelled case and it allows us to remove the workaround from browser_bug553455.js. It's possible it will fix the remaining intermittent timeout in there which makes me tempted to think about landing this for 4.0, it certainly seem extremely safe to me but I'd be interested in what you thought of that Rob?
Attachment #513355 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

8 years ago
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review rs]
Comment on attachment 513355 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -5470,16 +5471,30 @@ AddonInstall.prototype = {
>                                                this.listeners, this.wrapper)
>       return;
>     }
> 
>     // If a listener changed our state then do not proceed with the download
>     if (this.state != AddonManager.STATE_DOWNLOADING)
>       return;
> 
>+    if (this.channel) {
>+      // A previous download attempt hasn't finished cleaning up yet, signal
>+      // that it should restart when complete
>+      LOG("Waiting for previous download to complete");
This makes it sound like the download has completed as in finished downloading vs. being cancelled and restarted.

As patches go this seems straightforward and I would be tempted to try to get it into Firefox 4 as well. It is a tough call though this late in the game
Attachment #513355 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
(Assignee)

Updated

8 years ago
Whiteboard: [has patch] → [has patch][checkin-needed-post-branch]
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][checkin-needed-post-branch] → [can land fx6]
(Assignee)

Comment 4

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/636763da2e9a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [can land fx6]
Target Milestone: --- → mozilla6
Marking as verified fixed based on check-in and passing tests.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.