611755 - Cannot restart a download during the onDownloadCancelled event

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 2

[Dave Townsend [:mossop]]

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

Comment 3

[Robert Strong (they/them - no direct email)]

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

Comment 4

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/636763da2e9a

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed based on check-in and passing tests.