Closed Bug 611755 Opened 12 years ago Closed 11 years ago

Cannot restart a download during the onDownloadCancelled event


(Toolkit :: Add-ons Manager, defect)

Not set





(Reporter: mossop, Assigned: mossop)



(1 file)

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.
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.
Attached patch patch rev 1Splinter Review
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: nobody → dtownsend
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 ( {
>+      // 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]
Whiteboard: [has patch] → [has patch][checkin-needed-post-branch]
Whiteboard: [has patch][checkin-needed-post-branch] → [can land fx6]
Closed: 11 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.
You need to log in before you can comment on or make changes to this bug.