Downloads are not cleared on exit

VERIFIED FIXED in Firefox 64

Status

()

P5
normal
VERIFIED FIXED
2 years ago
4 months ago

People

(Reporter: oana.horvath, Assigned: petru)

Tracking

({privacy, regression})

Trunk
Firefox 64
ARM
Android
privacy, regression
Points:
---

Firefox Tracking Flags

(fennec+, firefox52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 verified)

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Build: Beta 53.0b7 (2017-28-03);
Devices:
-Asus ZenPad 8.0 Z380KL (Android 6.0.1)
-Huawei Honor 5X (Android 5.1.1)

Steps to reproduce:
1. Go to Settings>Privacy and tap on 'Clear private data on exit' 
2. Select 'Downloads' and tap Set.
2. Download some files: e.g. images from Google (I did not find a relation between the issue and files types).
3. Quit Firefox.
4. Downloads are cleared on the first try.
5. Download again some files and quit. Repeat until you can still see the files in about:downloads.

Expected results: The downloads are cleared on exit everytime.

Actual results: Downloads are not always cleared from the download manager, but are removed from the internal storage. If you try to open the file from about:downloads it won't do anything.
(Reporter)

Updated

2 years ago
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
(Reporter)

Updated

2 years ago
Blocks: 1343995
This sounds like a fairly bad privacy bug.
Keywords: privacy, regressionwindow-wanted

Comment 2

2 years ago
Honor 6 - about:downloads not cleared but the downloaded files get deleted from storage, thats why i unticked this some time ago. Now i lost again plenty of saved files.

Comment 3

2 years ago
Aurora 
54.0a2
(2017-03-28)
snorp, can you help investigate or find an owner for this bug? Thanks.
Flags: needinfo?(snorp)
This is more front end than platform.
Flags: needinfo?(snorp) → needinfo?(s.kaspari)
This almost sounds like it could be related to the problems we have with removing the history when quitting.

@JanH: Quick question, from fixing the history bug, could we have the same problem with downloads or do you think it's unrelated?
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari) → needinfo?(jh+bugzilla)
If this doesn't happen when clearing downloads while Firefox is running via Settings -> Clear private data, then yes, this sounds like somehow somewhere we're doing something async and not waiting for its completion, although from looking at it, at least the sanitizer code itself (https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/mobile/android/modules/Sanitizer.jsm#253) doesn't look immediately suspicious.
Flags: needinfo?(jh+bugzilla)
We did have problems in the sanitizer before, I remember. Maybe something regressed there.
status-firefox52: affected → wontfix
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #8)
> We did have problems in the sanitizer before, I remember. Maybe something
> regressed there.

I fixed a similar problem in bug 1163937; but at first glance it doesn't look like there have been any significant changes to the patch I landed for that.
tracking-fennec: ? → +
Priority: -- → P2
(Reporter)

Comment 10

2 years ago
Regression window performed:
Last good revision: 5e0140b6d11821e0c2a2de25bc5431783f03380a (2016-02-27)
First bad revision: 5e0140b6d11821e0c2a2de25bc5431783f03380a (2016-02-28)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=5e0140b6d11821e0c2a2de25bc5431783f03380a
Keywords: regressionwindow-wanted
(Reporter)

Updated

a year ago
status-firefox58: --- → affected
status-firefox59: --- → affected
[triage] Borderline critical privacy bug. To be clear, it looks like files are removed from disk but not in about:downloads (comment 2).
Priority: P2 → P1
Flags: needinfo?(sdaswani)

Comment 12

a year ago
Mike what’s the attack surface to glean those items from about:downloads? If not very high I would keep it as typical P1.
Flags: needinfo?(sdaswani) → needinfo?(michael.l.comella)
tldr; low – a user would need access to the unlocked device to see this file but I'm most concerned about the use case like:
  1) user downloads file they want to keep private
  2) deletes downloads, and assumes this accident was erased
  3) they hand their device to another user to use, assuming no private data will be exposed
  4) Other user sees this data

Full explanation: if about:downloads keeps the list of downloaded items, it's likely keeping this data in a file. Thus the data can be accessed from the UI (which would require another user to have access to the device) or from this file. Android devices are encrypted by default (starting with M (a little over 50%) and some L devices (75% of devices)) and sandboxes apps from accessing another app's data so these files should be protected from everyone without access to the unlocked device expect for users who root their device (and they're on their own).

Susheel, what do you think about the priority here? Sounds like you think it should stay P1.

Android version distribution: https://developer.android.com/about/dashboards/index.html
Flags: needinfo?(michael.l.comella) → needinfo?(sdaswani)

Comment 14

a year ago
yes, stay as p1.
Flags: needinfo?(sdaswani)

Updated

a year ago
status-firefox60: --- → unaffected
status-firefox61: --- → affected

Updated

11 months ago
status-firefox60: unaffected → affected

Updated

9 months ago
status-firefox62: --- → affected
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: P1 → P5
Keywords: regression
status-firefox53: affected → wontfix
status-firefox54: affected → wontfix
status-firefox55: affected → wontfix
status-firefox58: affected → wontfix
status-firefox59: affected → wontfix
status-firefox60: affected → wontfix
status-firefox61: affected → wontfix
status-firefox62: affected → fix-optional
I tend to think this should still be P1 since it is a clear privacy issue.
status-firefox63: --- → affected
Flags: needinfo?(sdaswani)

Comment 17

7 months ago
Added to work queue.
Flags: needinfo?(sdaswani)
Whiteboard: --do_not_change--[priority:high]
(Assignee)

Updated

6 months ago
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
(Assignee)

Comment 18

6 months ago
I've tried to reproduce this today but was not able to.
Is this still an issue?

What I've observed though is that canceled downloads (when Quit-ing) still remain both in about:downloads and on disk.
Flags: needinfo?(ioana.chiorean)

Comment 19

6 months ago
I was not able to reproduce either on several devices but not the reporting one. Petru, I think the huawei is with you guys. You might want to try with that. 

We did reproduce the canceling download issue - are you gonna report it or should we?
Flags: needinfo?(ioana.chiorean) → needinfo?(petru.lingurar)
(Assignee)

Updated

6 months ago
See Also: → bug 1492404
(Assignee)

Comment 20

6 months ago
Tested this including on the Honor 5X for which it was reported and could not reproduce.
Regarding the issue of not clearing partial downloads I've filed bug 1492404
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Flags: needinfo?(petru.lingurar)
Resolution: --- → WORKSFORME

Comment 21

6 months ago
Hello everyone,

Following the steps provided, I was able to reproduce the issue today, on Nightly 64.0a1 (2018-09-27) on HTC Desire 820 (Android 6.0.1).

Also, I checked all the boxes from Settings -> Privacy -> 'Clear private data on exit', tapped "Set" and quitted Firefox. The files were still in about:downloads, but could not be opened.

Please see attached video.

Thanks!

Comment 22

6 months ago
Posted file 1.txt
Logcat here,

Updated

6 months ago
Status: RESOLVED → REOPENED
status-firefox64: --- → affected
Resolution: WORKSFORME → ---
(Assignee)

Comment 23

6 months ago
Tested with Mira and saw that in the cases in which this fails the call to persist the current number of downloads (which after sanitizing should be 0)
https://dxr.mozilla.org/mozilla-central/rev/7e9ab0e7b608ddb4d84f36194509f498c66e3449/toolkit/components/downloads/DownloadStore.jsm#140
is never called.

This might be because the call to persist
https://dxr.mozilla.org/mozilla-central/rev/7e9ab0e7b608ddb4d84f36194509f498c66e3449/mobile/android/modules/Sanitizer.jsm#358
is not blocking (in the case of downloadFiles `clear` is a generator function but it's not used as such) and the app closes before having the chance to save the current number of downloads.
Assignee: petru.lingurar → nobody
Assignee: nobody → petru.lingurar
(Assignee)

Comment 24

5 months ago
There are 2 ways we ensure downloads are sanitized before shutdown:
1) DownloadIntegration.forceSave()
Need to use the already initialized instance of DownloadIntegration, which has an
already initialized DownloadStore to actually store the updated list of downloads.
2) A Blocker for AsyncShutdown.profileBeforeChange
Will ensure DownloadStore will finish writing the updated list of downloads.
Must use await to actually block until that operation completes.
(Assignee)

Comment 25

5 months ago
Seems like the sanitization was broken for a long time.
In the cases it worked we were just lucky to win the race from [1]
> AsyncShutdown.profileBeforeChange.addBlocker("DownloadAutoSaveView: writing data",
>                                              () => this._writer.finalize());
which didn't actually block and the actual shutdown of the app.
DownloadIntegration.forceSave() also didn't do anything as it would always return `Promise.resolve` [2] because we would use another instance of DownloadIntegration, not the one already initialized with a DownloadStore.

The above patch ensures we use the global DownloadIntegration object so that forceSave() will block until actually saving the updated list of downloads.
The Blocker for AsyncShutdown.profileBeforeChange will also block and ensure all writes (not necessarily the ones from Sanitizer) are completed before shutting down.

[1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/components/downloads/DownloadIntegration.jsm#1050
[2] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/components/downloads/DownloadIntegration.jsm#792
Attachment #9016324 - Attachment description: Bug 1351308 - Ensure sanitizing before shutdown; r?sdaswani → Bug 1351308 - Ensure sanitizing before shutdown; r?jchen
(Assignee)

Comment 26

5 months ago
Updated my initial patch to remove the DownloadIntegration part. It was more of a speculative fix.
Thanks :paolo for highlighting the incorrect usage of AsyncShutdown.profileBeforeChange()!
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 27

5 months ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbab4e3611fb
Ensure sanitizing before shutdown; r=jchen
Keywords: checkin-needed

Comment 28

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbab4e3611fb
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago5 months ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
status-firefox62: fix-optional → wontfix
(Assignee)

Updated

5 months ago
Flags: qe-verify+
(Reporter)

Comment 29

5 months ago
Verified as fixed on Nightly 64. 
LG Nexus 5 (Android 6.0.1)
Mi 4i (Android 5.0.2)
Google Pixel (Android 9)
(Reporter)

Updated

5 months ago
status-firefox64: fixed → verified
Flags: qe-verify+
Not a new issue and we're in RC week. Let's let this ride the trains.
status-firefox63: affected → wontfix

Updated

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