1755729 - Resume/Retry doesn’t work in case of deleted inprogress downloads

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

[Adrian Florinescu [:aflorinescu]]

Note

• Not sure if we want to have a retry available for a deleted download, but in case we don’t then fixing bug 1755728, automatically removes the scenario causing this one.

Affected versions

• Firefox 98 beta 4
• Nightly 99.0a1

Affected platforms

• all

Steps to reproduce

1. Download a big file link
2. While downloading, right click and delete.
3. Download is set into a pause state, proceed to delete again or just cancel.
4. Retry download.

Expected result

2. Download is deleted, the download panel states that successfully - see enh. File deleted from Downloads Panel contextual menu should have a different UI from the one deleted from disk
3. This case is not hit.
   .4 Download is restarted.

Actual result
2. Download is paused
3. Download is deleted (canceled)
4. Retry doesn’t work.

Regression range

• Although this is a narrow edge case to reach in, this regresses the retry download.

Comment 1

[Shane Hughes [:aminomancer]]

It seems like if this applies to files deleted by Firefox, it should apply to files with missing download targets in general. Like, are there any scenarios where retrying a download is problematic, except where the target already exists?

Comment 2

[:Gijs (he/him)]

(In reply to Shane Hughes [:aminomancer] from comment #1)

It seems like if this applies to files deleted by Firefox, it should apply to files with missing download targets in general. Like, are there any scenarios where retrying a download is problematic, except where the target already exists?

No, I think the problem here is what happens if you delete the file mid-download, and the fact that you can do that (sort of covered in bug 1755728).

IOW, I don't think the problem in comment 0 occurs if you let the download complete, then use the "delete file" item, then retry. The issue is that we were halfway and someone (in this case, the user, via UI we gave them!) wiped the file from under our feet...

Comment 3

[Shane Hughes [:aminomancer]]

Oh, actually the file is not being deleted at all. The problem here is that it's pausing the download instead of deleting the file. Originally this worked, I must have tested it dozens of times. I suspect 1750484 regressed the in-progress delete behavior, because that patch changed how we finalize the download. Maybe it's a timing issue.

Originally I did not expect there to ever be a session download item whose target was deleted, because it was intended to delete the file and simultaneously remove the download item. It would all just poof out of existence so this was a lot simpler. Now if there's going to remain a download item after we use the Delete menuitem, then we need to make the Delete menuitem cancel an in-progress download and tell the saver to basically reset everything.

I guess my problem with that is it doesn't make any sense that deleting the download item would result in it displaying as a normal, clickable element. It seems more like we just shouldn't show the Delete menuitem when the download is in progress, unless browser.download.clearHistoryOnDelete > 0. However, even in that case (when it clears the session download on delete), it's still failing to delete the file, so that needs to be fixed too.

Honestly I still don't really like the default behavior of browser.download.clearHistoryOnDelete = 0, it seems weird to me that it should remain cluttering the session download list. I think a default value of 1 makes more sense. That's where it deletes the session download but keeps the history download, such that it appears in the "all downloads" view (and the download link remains :visited) but does not appear in the downloads panel.

Comment 4

[:Gijs (he/him)]

(In reply to Shane Hughes [:aminomancer] from comment #3)

I guess my problem with that is it doesn't make any sense that deleting the download item would result in it displaying as a normal, clickable element. It seems more like we just shouldn't show the Delete menuitem when the download is in progress, unless browser.download.clearHistoryOnDelete > 0. However, even in that case (when it clears the session download on delete), it's still failing to delete the file, so that needs to be fixed too.

Fixing both of these (ie not showing the option for the default case unless the download is stopped and there is something to delete - download doesn't have to be complete! - and making sure that it works if the download is in progress and is invoked for non-default pref values) seems like the way forward here. Shane, do you have time to work on this?

Comment 5

[Shane Hughes [:aminomancer]]

(In reply to :Gijs (he/him) from comment #4)

Fixing both of these (ie not showing the option for the default case unless the download is stopped and there is something to delete - download doesn't have to be complete! - and making sure that it works if the download is in progress and is invoked for non-default pref values) seems like the way forward here. Shane, do you have time to work on this?

Yeah, got sidetracked by something else but I'm on it now 👍

Edit: Actually, it occurred to me that we can delete an in-progress download and make it unclickable as long as we add a new download property (which will support bug 1755570, adding a new label for fx-deleted downloads).

This makes it a bit more complicated and time consuming since we would need to make sure canceled+deleted downloads appear the same and have exactly the same context menu items displayed as stopped+deleted downloads. I think that's not a foregone conclusion because if the download is in progress, it's not succeeded or stopped or canceled at the time it's deleted, which changes how those values will end up when the dust settles. But I will need to look at it more carefully to be sure.

But we can start by just adding the new download property and implementing the new label, and (for the moment) hiding the "Delete" menuitem for in-progress downloads such that user can't create canceled+deleted downloads. So we can land the fix for this bug quickly, and implement the ability to delete in-progress downloads a bit later once we're sure that the UI is perfectly consistent.

Comment 6

[Shane Hughes [:aminomancer]]

Note: There's a separate but related issue where the "cancel" function doesn't refresh partFileExists even though it deletes the part files, so we wind up showing the "delete" menuitem on canceled downloads, in which case it does nothing and also might bug the download object in some way.

Comment 7

[Shane Hughes [:aminomancer]]

This was fixed by bug 1755570 FYI.

Comment 8

[Shane Hughes [:aminomancer]]

Although if there are regressions with this particular context of resume/retry on deleted or canceled downloads, put them here. It was too complicated to separate into 2 patches but they are sort of different things.

Comment 9

[Adrian Florinescu [:aflorinescu]]

Let's put this on our radar to verify.

Comment 10

[Adrian Florinescu [:aflorinescu]]

Verified as fixed on Firefox 100.0b8 and Nightly 101.0a1 (2022-04-20) on Windows 10, Ubuntu 22 and Mac 11.