Closed Bug 611076 Opened 10 years ago Closed 10 years ago

Cancelling an AddonInstall that has been redirected to a new URL isn't possible

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(2 files)

If the AddonInstall gets a http redirect then cancelling it won't work as we fail to grab the new channel.
blocking2.0: --- → betaN+
Attached patch patch rev 1Splinter Review
Simple fix that turned out difficult to test. The http activity observer allows us to wait until the end of the redirected request and verify that it was cancelled.

Also slipped in a couple of logging changes that really should have been in bug 593535
Attachment #489845 - Flags: review?(robert.bugzilla)
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review rs]
Attachment #489845 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
Landed: http://hg.mozilla.org/mozilla-central/rev/b0e1d3528ad3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
Backed out due to windows debug test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The failure is actually a problem that I guess had already existed and was never tested. In the cancel method we attempt to delete the temporary file but since the download channel closes asynchronously the file is still in use and windows fails that attempt. Adding a second attempt in onStopRequest works.
Attachment #489998 - Flags: review?(robert.bugzilla)
Attachment #489998 - Attachment is obsolete: true
Attachment #489998 - Flags: review?(robert.bugzilla)
Comment on attachment 489998 [details] [diff] [review]
properly cleanup temp files

Found another issue (bug 611755) but decided it wasn't really worth fixing here for now.
Attachment #489998 - Attachment is obsolete: false
Attachment #489998 - Flags: review?(robert.bugzilla)
Blocks: 570012
Comment on attachment 489998 [details] [diff] [review]
properly cleanup temp files

What are the chances of there being other cases where temp files need to be cleaned up? Is there a way to test this case as well?
Attachment #489998 - Flags: review?(robert.bugzilla) → review+
(In reply to comment #6)
> Comment on attachment 489998 [details] [diff] [review]
> properly cleanup temp files
> 
> What are the chances of there being other cases where temp files need to be
> cleaned up? Is there a way to test this case as well?

The xpcshell tests caught this because they verify that there are no temp files left over at the end of every run, which should mean that every case that is tested isn't leaving behind temp files. At this point it might be god to see if we can still do code coverage analysis of test runs to see whether any of hte code still isn't being tested.
Landed: http://hg.mozilla.org/mozilla-central/rev/c85961497ae9
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Dave, anything I can test manually? I have created a PHP script which redirects to an XPI file, but I don't see a difference. Probably I don't completely understand this issue.
(In reply to comment #9)
> Dave, anything I can test manually? I have created a PHP script which redirects
> to an XPI file, but I don't see a difference. Probably I don't completely
> understand this issue.

What would happen is that cancelling would appear to work in that the add-on wouldn't then get installed or anything, but the add-on's file would continue to be downloaded and get saved in the OS temp directory.
Ok, so this is kinda hard to test manually without having the download progress bar and a way to cancel a download. Looks like we have to trust the automated test you have created. Marking as verified fixed based on non-failing tests.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.