1114506 - Downloads are not cleared after clearing private data

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[Cristina Madaras, QA [:CristinaM]]

Environment: 
Device: Asus Transformer TF101 (Android 4.0.3);
Build: Nightly 37.0a1 (2014-12-21);

Steps to reproduce:
1. Download a file (e.g.: 1.usa.gov/deeXKM);
2. Go to Settings -> Privacy -> Clear now;
3. Make sure that all the options are checked and tap on 'Clear data' button;
4. Go to Tools - > Downloads.

Expected result:
The downloads list is empty.

Actual result:
The downloaded file is present in the downloads list.

Regression window:
Last good: 2014-12-16
First bad: 2014-12-17

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b836016d82b5&tochange=b7eb1ce0237d

Probably Bug 901360 caused the regression.

Comment 1

[:Paolo Amadini]

[Tracking Requested - why for this release]: Regressions from bug 901360

I was just looking into some remaining nsIDownloadManager uses in the "mobile/android" folder, and I expected this regression from code inspection:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm#205

On Desktop, this now uses DownloadList.removeFinished:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#341

It seems on Android you want to remove files on disk though, so you might need your own implementation. It would require just a few lines, you can look at the current removeFinished implementation for reference:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadList.jsm#235

Comment 2

[:Margaret Leibovic]

Is this actually a regression? Bug 1107510 seems to indicate that this was an existing issue.

Comment 3

[Cristina Madaras, QA [:CristinaM]]

Bug 1107510 was reproducible only in private browsing. 
This issue is reproducible starting from 2014-12-17, I am not able to reproduce it on previous builds, so I guess is a regression.

Comment 4

[:Margaret Leibovic]

I can look into this.

Comment 5

[:Margaret Leibovic]

Looks like Sanitizer.jsm still uses the old downloads API:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm#22

Comment 6

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1114506/margaret

Comment 7

[:Margaret Leibovic]

/r/1649 - Bug 1114506 - Update Sanitizer.jsm to use Downloads.jsm to clear downloads

Pull down this commit:

hg pull review -r 622e088926a9e57c78bbd81563fc370fe127785d

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

https://reviewboard.mozilla.org/r/1647/#review1049

Comment 9

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/1647/#review1067

::: mobile/android/modules/Sanitizer.jsm
(Diff revision 1)
> +            // Delete the downloaded files themselves.
> +            let file = new FileUtils.File(download.target.path);
> +            if (file.exists()) {
> +              file.remove(false);
>              }

You should use OS.File.remove in new code. For efficiency, we recommend just invoking remove and catching exceptions, rather than checking for existence first. I'd also report real errors but continue with the next download, something similar to this:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#1995

(You might not need the becauseAccessDenied check and may want to report those errors as well.)

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/d6fb576da01d

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6fb576da01d

Comment 12

[u421692]

Verified as fixed on latest Nightly on Nexus 7(Android 5.0.1)

Comment 15

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1114506 - Update Sanitizer.jsm to use Downloads.jsm to clear downloads