Closed Bug 1796745 Opened 2 years ago Closed 1 year ago

Download progress icon is visible on next restart if the download failed (canceled by closing the browser)

Categories

(Firefox :: Downloads Panel, defect, P3)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
113 Branch
Tracking Status
firefox107 --- wontfix
firefox108 --- wontfix
firefox111 --- wontfix
firefox112 --- wontfix
firefox113 --- verified

People

(Reporter: csasca, Assigned: mak)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Found in

  • Firefox 107.0b3

Affected versions

  • Firefox 107.0b3
  • Firefox 108.0a1

Tested platforms

  • Affected platforms: Windows 8.1
  • Unaffected platforms: Win 7 & 10, macOS, Ubuntu

Steps to reproduce

  1. Launch Firefox
  2. Access Thinkbroadband page and download the 1GB file
  3. While the download is in progress close Firefox
  4. Open Firefox with the same profile

Expected result

  • The download is failed and no progress is shown on the downloads icon

Actual result

  • The downloads icon keeps the progress chart even if the download is failed

Regression range

  • WIll see for a regression

Additional notes

  • The issue can be seen in the attachment
  • Only Windows 8.1 seems to be affected
Has STR: --- → yes
Priority: -- → P5

I suspect the problem may actually be in Widget, since the taskbar widget is managed there and it's likely 8.1 may have some different behavior from other versions.
That said, finding someone on that version is not trivial. I don't have any machine to try atm.

QA Whiteboard: [qa-regression-triage]

I was able to reproduce this issue on a Windows 10 machine as well and I tried to get a "manual" regression range for this issue because each build needs a restart.

This is the best we could get, hopefully this helps one of our devs identify what is causing this bug:

100.0a1 (2022-04-01) - last good - https://hg.mozilla.org/mozilla-central/rev/a9b419c8a9c8f73b7ae8f18ff13a92499b3a2fa4
100.0a1 (2022-04-02) - First Bad - https://hg.mozilla.org/mozilla-central/rev/d14c155fb35a9ae59561ca33344ecb91e42dc1b1

PushLog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a9b419c8a9c8f73b7ae8f18ff13a92499b3a2fa4&tochange=d14c155fb35a9ae59561ca33344ecb91e42dc1b1

I was able to reproduce this issue also on Ubuntu 20.04 if after reopening Firefox the first time I retry the download and reopen Firefox again - reproduced using the latest Nightly 110.0a1 and the Firefox 102.7.0 esr.

This bug is... quite strange. Is this really Windows-specific? I don't quite see why it would be. I could definitely also repro on Win10. I guess per comment 4, it isn't actually Windows-specific.

I think the confusing thing here is that the internal state of the downloads view code thinks there's progress. The root issue is probably that that's being reported even though there are no actual in-progress downloads. But I don't see why that'd be different on macOS or Linux. Anyway, I'm concerned that that would also break other things. So clearing some flags for re-triage.

Maybe more importantly: does this also happen for other downloads that aren't first blocked by the mixed content blocker?

(the regression window has no clear culprits - I expect it's a race condition of sorts, maybe?)

Severity: S4 → --
Flags: needinfo?(catalin.sasca)
OS: Windows 8.1 → All
Priority: P5 → --
Summary: [Win 8.1] Download progress icon is still visible even if the download is failed → Download progress icon is visible on next restart if the download failed (canceled by closing the browser)
Attached image download.png

Yep, I was able indeed to reproduce it on Windows 10 with 111.0b1 as well but after some several tries, not as consistently as on Windows 8.1. Seems that this is happening on other normal downloads which are not blocked as well (see attachment).

Flags: needinfo?(catalin.sasca)

I don't know if it's the same thing (there may be a relation) but I can reproduce something close with simple steps:

  1. click on a download link on thinkbroadband
  2. when the downloads panel shows the download is insecure click on allow
  3. pause the download, it will change straight to Failed, but the download icon is still showing progress

Even if it's not the same bug, maybe it's a good pointer to find the problem.

I'll see if I can debug something out of this

Flags: needinfo?(mak)

On startup _updateView is just setting percentComplete in the view (indicator in this case), the value is taken from a summary built by summarizeDownloads() that for non succeeded downloads just looks for download.hasProgress
That's an interesting choice, because it considers progress regardless of download failed state, it looks like it comes from a time where failing would remove the .part file in any case. Today that doesn't seem to be the case.
If I retry the failed download, it will actually resume from the progress, so the .part file is correctly retained.

So, there is an inconsistency between the Failed status we show in the ui and the progress indicator.
A very trivial fix would be to change the above condition from else if (download.hasProgress) to else if (download.hasProgress && !download.error), that would at least synchronize the status between the list and the indicator, a Failed download doesn't indeed show a progress bar.

Regarding my STR, I'm still not sure why Pause is Failing the download, that sounds like some bug related to blocked downloads management.

Assigning to myself to keep track of this, but if anyone wants to steal just let me know. The issue and solution are in comment 9, it just needs some kind of test.

Assignee: nobody → mak
Status: NEW → ASSIGNED
Flags: needinfo?(mak)
Severity: -- → S3
Priority: -- → P3

The problem may be a little bit more complex than I outlined. It's not actually about skipping downloads with a download.error, but it's because canceling a download may set download.error.
Unblock() is actually doing download.start().catch(ex => this.error = err) that is copying the "Canceled download" error into the download object.
That object may then be serialized and deserialized bringing the error along with it.
Our code always checks download.error before download.canceled, to determine the state of a download.

This is the reason for which my unblock then pause fails and it ends up generating a download that is .canceled, .hasPartialData, but also has an .error.

Catalin, do you remember if the cases where you noticed the failure were all for downloads you "Unblocked" in the ui (maybe because http mixed content).

Flags: needinfo?(catalin.sasca)

Another case I found, pausing a download and closing the browser, aborts the download and on the next startup it should be shown as paused, instead it has an "abort" error.

Hey Marco, it happens on normal downloads as well, tried that and mentioned in Comment 6, so all downloads should be affected by this, not only the blocked/unblocked ones.

Flags: needinfo?(catalin.sasca)

My patch should fix the issue, with it I can consistently pause/cancel/retry and restart the browser where ongoing downloads automatically continue from where they were. Though I must write some test, even if a shutdown test will likely be out of scope here and require a follow-up bug for Marionette.

Just a small summary of my findings:

Originally we used to restart downloads automatically at th next session when the user closed the browser with ongoing downloads.
That broke because:

  1. there's "network going offline" detection code, that was intended to handle the "work offline" option, but network sometimes also fires it on shutdown. It could make sense, but that cancels downloads, and then on the next startup we see they were Canceled and don't start them.
  2. The above notification is intermittent on shutdown, sometimes it happens, sometimes not, when it happens downloads are canceled, when it doesn't happen downloads are aborted. The NS_ERROR_ABORT is then copied in .error and serialized to downloads.json. On the next startup we'll see them as Failed.
  3. Additionally, the Unblock() code path is copying over any exception to .error, also the exception generated by a call to cancel(). download.error is already set by start(), there should be no reason to further set it.
  4. a download may have both .error and .errorObj, on retry() we clear the former, but not the latter, so a download that stores an errorObj, is restarted and then serialized again, will bring the original errorObj with itself and continue being marked as failed even if it was restarted succesfully
Depends on: 1821847
Attachment #9321731 - Attachment description: WIP: Bug 1796745 - Canceled downloads are shown as failed → WIP: Bug 1796745 - Canceled downloads are shown as failed. r=gijs
Attachment #9321731 - Attachment description: WIP: Bug 1796745 - Canceled downloads are shown as failed. r=gijs → Bug 1796745 - Canceled downloads are shown as failed. r=gijs
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/1277131e3259
Canceled downloads are shown as failed. r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

Verified as fixed in our latest Nightly build 113.0a1 (2023-03-15).

The patch landed in nightly and beta is affected.
:mak, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(mak)

This is broken from quite a while, and I think it will benefit from Nightly testing.
So I'd just let it ride the train to release 113.

Updating the Main status flag.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: