User Agent: Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160604131506 Firefox for Android Steps to reproduce: Uninstalling Mozilla Firefox latest beta on Android 6.0. Actual results: All with firefox downloaded (in nde "Download" folder) files were deleted. Expected results: None of the should have been deleted.

This works for me using a Nexus 5x, Firefox 47 and Android N.

This also works for using a Z3C, Firefox 48.0, Android 6.0.1.

Looks like we should not do `context.getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS);` there[1] to setup download environment because the downloaded files will be removed when the application is uninstalled[2]. [1]: https://github.com/mozilla/gecko-dev/blob/master/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/GeckoLoader.java#L85-L100 [2]: https://developer.android.com/reference/android/content/Context.html#getExternalFilesDir(java.lang.String)

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #3) > Looks like we should not do > `context.getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS);` there[1] to > setup download environment because the downloaded files will be removed when > the application is uninstalled[2]. But this is only used for UPDATES_DIRECTORY. For the DOWNLOADS_DIRECTORY we use getExternalStoragePublicDirectory().

By works for me I meant I could not reproduce.

I still can reproduce the issue on Firefox 48.0(and current mozilla-central build[1]), Android 6.0.1, Z3C. Kevin, are you in same environment(Comment 1) for not reproducing it? [1]: user: Wes Kocher <wkocher@mozilla.com> date: Mon Aug 22 16:00:34 2016 -0700 summary: Backed out 2 changesets (bug 1279086) for causing painting issue

> But this is only used for UPDATES_DIRECTORY. For the DOWNLOADS_DIRECTORY we > use getExternalStoragePublicDirectory(). Yes, you're right. But the files are still removed after the app is uninstalled by unknown reasons. Need to investigate deeper...

Looks like we store downloaded files in the `Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS)` directory. But the files are still removed after the app is uninstalled. So weird. Maybe we have some code somewhere to delete them? [1]: http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/DownloadsIntegration.java#123

Bug 1097619? Bug 1140938? I personally think that decision was 100% incorrect, and once a file is downloaded we should never ever delete it, even if you clear your downloads. I doubt that's the cause here, but it *might* be related…

New finding: On Z3C, downloaded files are always stored in "internal storage" even a sd card is inserted. On Android, the internal storage files are always removed after the app is uninstalled[1]. It might be the root cause. So I think the solution could be that ensure Firefox for Android downloads files in "external storage" once there is a sd card inserted. Then I was trying store the downloaded files in "external storage"(a sd card) with following the instruction[2]. But I could not make it work. Could anyone make Firefox for Android download files in "external storage" successfully? [1]: https://developer.android.com/training/basics/data-storage/files.html [2]: https://support.mozilla.org/en-US/questions/1019397#answer-626698

I meant I was trying to test that the files stored in "external storage" will not be remove automatically after Firefox for Android is uninstalled. But I cannot make Firefox for Android download files in "external storage". Anyone can do that?

I can't reproduce this on a Z3C. After uninstalling Firefox (Beta in this case) the downloaded file is still visible in the Downloads app and can be opened.

Interesting... I still can reproduce on Firefox Beta, Z3C. What is your system version? My version is Android version: 6.0.1 Kernel version: 3.4.0-perf Build number: 23.5.A.0.575

We should make sure we all are talking about the same steps. Sebastian and I are using * Download some files to populate the downloads list * Uninstall Firefox * Use a 3rd party file browser to inspect /sdcard/download/ * Files that were downloaded will be present If your steps are * Download some files * Uninstall Firefox * Reinstall Firefox * Open about:downloads or menu > tools > downloads * Files that were previously downloaded will not be present We do not have a way to persist this data on uninstall inside Firefox and we do not know if we should (UX/Product decision).

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #13) > I still can reproduce on Firefox Beta, Z3C. What is your system version? > My version is > Android version: 6.0.1 My Z3C is still on Android 5.1.1.

I'm pretty sure I've experienced this twice now (moto e running marshmallow) Interestingly, yesterday it didn't delete ALL my pictures and videos from the Download album, some were still there. Also, it's not only deleting pictures and videos that were saved from Firefox, it deleted pictures and videos that my friend sent to me via the Skype app for android (those also go to the Download album) This seems very serious and something should be done right away

Please check https://youtu.be/WsniOFK_Cbk I can't reproduce this bug. All files exist after Fennec uninstalled. Sorry the video is too big so I can't just attach it

I tested with the pdf file[1] before. [1]: http://www.pdf995.com/samples/pdf.pdf

I still can reproduce this on Android 6.0.1.

Thanks Evan. I can reproduce it in Android 6.0 with Nexus 5. I think it's an Android 6 feature. I'll fix it later

I've tested in Nexus 5 and Asus Zenfone 2 Laser with Android 6.0.1. In Android 6.0.1, files saved with getExternalStoragePublicDirectory() will be automatically deleted when app is uninstalled, so I use getExternalStorageDirectory() instead

Comment on attachment 8809304 [details] Bug 1280184 - Use getExternalStorageDirectory() to avoid files being deleted automatically when app is uninstalled in Android 6.0.1 https://reviewboard.mozilla.org/r/91890/#review91902 Is this only in Android 6.0.1 - e.g. did they fix it later? And did you find any bug reports about that? If only 6.0.1 is affected then let's only fallback to getExternalFilesDir() on Android 6.0.1 (Are all 6.x version affected?) - See constants in AppConstants for version checks. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/GeckoLoader.java:95 (Diff revision 2) > - if (downloadDir == null) { > + > - downloadDir = new File(Environment.getExternalStorageDirectory().getPath(), "download"); > - } > if (updatesDir == null) { > updatesDir = downloadDir; > + nit: Remove this new empty line ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/GeckoLoader.java:199 (Diff revision 2) > // Disable on-demand decompression of the linker on devices where it > // is known to cause crashes. > String forced_ondemand = System.getenv("MOZ_LINKER_ONDEMAND"); > if (forced_ondemand == null) { > if ("HTC".equals(android.os.Build.MANUFACTURER) && > - "HTC Vision".equals(android.os.Build.MODEL)) { > + "HTC Vision".equals(android.os.Build.MODEL)) { nit: Unrelated whitespace change ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/GeckoLoader.java:545 (Diff revision 2) > > @JNITarget > public static void abort(final String msg) { > final Thread thread = Thread.currentThread(); > final Thread.UncaughtExceptionHandler uncaughtHandler = > - thread.getUncaughtExceptionHandler(); > + thread.getUncaughtExceptionHandler(); nit: Another unrelated change (I guess the IDE did this automatically)

I've traced in Android source code and found the only difference between 7.6 vs 5 is that it delete extra files when app uninstalled. see below http://androidxref.com/5.1.1_r6/xref/frameworks/base/services/core/java/com/android/server/pm/PackageManagerService.java#removeDataDirsLI http://androidxref.com/6.0.0_r1/xref/frameworks/base/services/core/java/com/android/server/pm/PackageManagerService.java#15839 http://androidxref.com/7.0.0_r1/xref/frameworks/base/services/core/java/com/android/server/pm/PackageManagerService.java#19555 And I've tested in Chrome browser app this is also the expected behavior. After talking to Sebastian. Since this is by Android's design, we shouldn't do anything against it. But we can improve the UX by letting user select his/her own directory. There's already another bug 677001 so I'll close this bug.

For some unfathomable reason reproducibility is somewhat flaky with my local build, but with the official Nightly build I can reproduce this without problems. Therefore, I've tested with a Try build along the lines of the above patch (but also taking care of https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/DownloadsIntegration.java#126) and it made no difference. Thinking more carefully about it this should have been expected, as there's nothing magic about getExternalStoragePublicDirectory(), especially since we're simply taking the path and converting it to a simple string. On my phone, the only difference between paths retrieved through "Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS)" and "new File(Environment.getExternalStorageDirectory().getPath(), "download")" is that the latter uses a small "l" for "download", but as Android treats the internal SD card as case insensitive, it doesn't make any difference. Using a completely separate download directory didn't help either. The real culprit is the system download manager, which associates all files in its database with the application that downloaded (or called addCompletedDownload for) that file, and in Android 6 gained the "feature" of cleaning up files of uninstalled apps: https://issuetracker.google.com/issues/37115671 https://stackoverflow.com/questions/35209375/android-6-0-external-storage-files-being-deleted-upon-app-uninstall https://commonsware.com/blog/2016/02/09/changes-downloadmanager-behavior.html So the only way to fix this would be to turn off again our system download manager integration (bug 816318) on Android 6 and above until maybe some future Android version allows specifying whether files added to the system download manager should be cleaned up or left behind upon app uninstall.

how about using a file name for example <orginal_file_name>.downloading for download manager, and change it back once it downloads completed(add a BroadcastReceiver for ACTION_DOWNLOAD_COMPLETE )?

(In reply to Jan Henning [:JanH] from comment #26) > uses a small "l" for "download" Argh, that should have been a "d" vs "D", of course. (In reply to Nevin Chen [:nechen] from comment #27) > how about using a file name for example <orginal_file_name>.downloading for > download manager, and change it back once it downloads completed(add a > BroadcastReceiver for ACTION_DOWNLOAD_COMPLETE )? That wouldn't make any sense for our setup - don't forget that we're not using the system download manager to actually download the files. We're just calling addCompletedDownload() after we've downloaded them ourselves (https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/mobile/android/base/java/org/mozilla/gecko/DownloadsIntegration.java#170), so users can find their downloads in the Android "Downloads" app as well. So calling addCompletedDownload() for a temporary file won't achieve anything useful - the resulting entry in the system download manager won't work once the file has been renamed and will eventually be garbage collected, so we might as well not bother calling addCompletedDownload() at all in that case.

Hi Julian, do you have any idea how to fix this?

Comment on attachment 8903953 [details] Bug 1280184 - Disable system download manager integration on Android 6+. https://reviewboard.mozilla.org/r/175704/#review180948 I think this is something product should decide. Just not adding downloads to Android's download manager is probably affecting more *actual* users - and I definitely care more about those users than users that uninstalled Firefox. It's not the uninstall flow that I want to optmize. :) Anyways, I see two options: A) We stop supporting Android's download manager and downloads won't show up there anymore (That's your patch). B) We continue to live with Android removing downloads after app uninstall - it seems like this is the intended system behavior and not limited to Firefox.

^-- Joe, I think that's something for the product team to decide.

Comment on attachment 8903953 [details] Bug 1280184 - Disable system download manager integration on Android 6+. https://reviewboard.mozilla.org/r/175704/#review180948 True, although this also has some bearing on bug 1392768, so any decision here should also be coordinated with the decision for that. Namely the current situation is that clearing our own download history will also remove the corresponding system download manager entry, so no matter whether we delete the files ourselves, on Android 6+ the system download manager will do it for us whether we want it or not. So if we want to give users the choice of clearing up their download history without deleting the files at the same time, we have to either - stop adding the files to the download manager in the first place (i.e. this bug) - or else at least no longer synchronise deletions if we want to keep the file. That is if a download is removed from our own history we can't remove it from the system download manager if the user wants to keep the file.

My understanding is same as Sebasitian and Jan's comments. I agree this is a product decision.

Wake me up when you've come to a decision...

I don't think it's major. This is Android's system behavior if we want to support current installed user using DownloadManager ( can continue the download while network issue or reboot . see https://developer.android.com/reference/android/app/DownloadManager.html) Since there's an NI, I'll clear the tracking flag.

Agree with what Sebastian is saying in comment 31 to provide a better download experience for *actual* users who are using Firefox. I'm curious about Jan's comment 33 on "- or else at least no longer synchronise deletions if we want to keep the file. That is if a download is removed from our own history we can't remove it from the system download manager if the user wants to keep the file." Does this mean that Firefox for Android can no longer delete actual downloaded files from the Fennec download list? That is the DELETE options are removed and becomes "remove from list" option?

(In reply to Nevin Chen [:nechen](pto till Jan 3rd) from comment #36) > This is Android's system behavior if we want to support current installed > user using DownloadManager ( can continue the download while network issue > or reboot . see > https://developer.android.com/reference/android/app/DownloadManager.html Er, we don't use the DownloadManager that way, we *always* handle the download process itself completely by ourselves. The only thing that happens is that once the download has *completed successfully*, we tell the DownloadManager about it so users can access the downloaded file not only via our own download manager (about:downloads), but also by using Android's Downloads app. This then has the possibly (for some users) unwanted side effect of coupling the lifetime of that file with that of Firefox itself - at least as long as the user doesn't move the file out of the dowbkoads directory.

@JoeCheng: As mentioned in my comment, see bug 1392768 for that. That is if we WONTFIX this bug, but go ahead with bug 1392768, we still have to do something about the System DownloadManager anyway, i.e. the second option I described above.

Users uninstalling Firefox might not be our primary target audience (although deleting their downloaded files might be viewed as adding insult to injury), there are also things like bug 1391268 (if the corrupted APK theory holds, affected users need to reinstall Firefox - technically I think you can install the same APK version again, but via the Play Store you need to uninstall first if you don't want to wait for the next update) or bug 1435420 (people trying to remove a PWA shortcut accidentally uninstall Firefox as well).

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195 Needinfo :susheel if you think this bug should be re-triaged.

I don't seem to have the Downloads app on my Pixel anymore. Does the reasoning for DownloadManager integration still hold?

Is that universal? Some cursory searching around suggests that the new Files app was indeed supposed to replace the old Downloads app (i.e. the user-facing part of the system download manager), but on the other hand I've found some posts from people claiming that even after updating to Oreo they still only had the old Downloads app present in their app drawer. Secondly, for those that have the Files app, does it indeed show any file placed in the Downloads folder when opening the Downloads view? I.e. if you manually copy some file into the Downloads folder (possibly with some other file manager just to be sure) and then look at Downloads in the Files app, does it show that file? If yes, then the Downloads Integration might indeed no longer be worth it, and for consistency regarding uninstallation behaviour maybe it should be disabled on M and N as well, even if there the file manager if somewhat more hidden and the Downloads app still exists.

(In reply to Jan Henning [:JanH] from comment #43) > Is that universal? Some cursory searching around suggests that the new Files > app was indeed supposed to replace the old Downloads app (i.e. the > user-facing part of the system download manager), but on the other hand I've > found some posts from people claiming that even after updating to Oreo they > still only had the old Downloads app present in their app drawer. I couldn't say how universal it is. Perhaps some other manufacturers are doing their own thing here. > Secondly, for those that have the Files app, does it indeed show any file > placed in the Downloads folder when opening the Downloads view? I.e. if you > manually copy some file into the Downloads folder (possibly with some other > file manager just to be sure) and then look at Downloads in the Files app, > does it show that file? I used Amaze file manager to copy a file into the Download directory and looked in the Files app. The copied file showed in the Downloads view.

Additional (potentially interesting) information: * known: 6.0 to 7.1 removes[1] all downloaded files on app uninstall * Since 8.0 Android "disowns downloads that live in shared storage" prior to deletion[2]. Unfortunately this doesn't include Firefox downloads, since those have a destination of `DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD` This change[3] means that other browsers and apps that use the DownloadManager to download files to external storage have their downloaded files remain after app uninstall, while we're stuck with getting the files deleted since the deletion logic still indiscriminately targets non-DownloadManager downloads. So this now also becomes a parity-[any-browser-using-DownloadManager-for-downloads] issue. [1] http://androidxref.com/7.1.2_r36/xref/packages/providers/DownloadProvider/src/com/android/providers/downloads/DownloadReceiver.java#140 [2] http://androidxref.com/8.0.0_r4/xref/packages/providers/DownloadProvider/src/com/android/providers/downloads/DownloadReceiver.java#140 [3] https://android.googlesource.com/platform/packages/providers/DownloadProvider/+/50522c738bf68d2784ce4f52dd34188a491065b8

Could you get someone to think about a decision on this again? To summarise: - Generally, we're not using the downloads manager to actually download files, but we're still calling addCompletedDownload() so that our downloads also show up in the system's "Downloads" app for users that prefer using that instead of our own about:downloads page. - Starting from Android 6.0, the system download manager has started to delete all files downloaded by an app (including those that were added after the fact, like in our case) when that app gets uninstalled. - As Johannes pointed out above, starting from Android 8.0 files downloaded to a public folder are no longer deleted, but this only applies to downloads downloaded directly via the download manager. Our downloads, which are registered after the fact are still deleted when we get uninstalled. - Additionally, as far as I'm aware in Android 8.0 Google has also started to replace the user-facing part of the download manager, i.e. the Downloads app, with a more general Files app. The Files app works like any normal file manager and its Downloads view simply shows all files present in the Downloads folder on the shared storage, so there's no longer any need to register our downloads with the system download manager. So: - Starting from Android 8.0, we're at a disadvantage compared to browsers downloading directly through the system download manager, because their files remain available after uninstallation, while our users' downloads are still deleted. - Likewise, our downloads integration doesn't actually have any benefit on Android 8.0 and above any more, because the Downloads app is going away and the replacement Files app doesn't require any "downloads integration" on our part other than saving to the default downloads folder. Therefore I'd propose to disable our downloads integration again. For consistent behaviour regarding deletion of files on uninstall, I'd recommend disabling it starting from Android 6.0, even though the Downloads app is still in use on Android 6 and 7.

And until the Mozreview redirection service gets live, this was my old patch: https://hg.mozilla.org/mozreview/gecko/rev/7c975d34dad2

This is a product decision.

Out of interest, what is the reason we're not using the system download manager to handle downloads? Otherwise, JanH's proposal in comment 46 sounds good to me.

Hard to say - I wasn't around at that time and conversely most of the people who were around are now gone as far as I can tell. Maybe kbrosnan remembers something? The best I've found is some bits and pieces in bug 816318, especially bug 816318 comment 0. I.e. mainly that the DownloadManager wasn't available on all platforms we supported at that time, plus the UX wasn't uniformly better - at least then DownloadManager downloads couldn't be paused, tapping the notification didn't do anything useful and error handling was worse in some regards. There's also the fact that while the DownloadManager might be a bit better at gracefully handling dropped network connections than we currently do, not using our own network stack imposes restrictions on other features (probably alluded to in bug 816318 comment 48): From what I can come up with given my limited knowledge in that regard, this entails that downloads wouldn't be using our proxy settings (or TOR integration in Orfox/the coming Tor Browser!), would use a different certificate store, etc. One-time downloads (where the link expires after one use) might also pose a fundamental problem, because the initial connection done by Gecko when we attempt navigating to the download URL (and then discover that we cannot handle the received data ourselves and need to download it in fact) already counts towards that limit, so it would be impossible to hand off that kind of downloads.

Jan hit all the major points as to why we used our own downloader. The notification from the download manager was a lot simpler. No way to do a progress bar & pausing in the notification and the need to support ancient Android versions at the time. Making TOR integration a bit harder and the proxy bits worry me a little bit.

Comment on attachment 9009375 [details] Bug 1280184 - Turn off downloads integration on Android M and later. r?snorp James Willcox (:snorp) (jwillcox@mozilla.com) has approved the revision.

Pushed by mozilla@buttercookie.de: https://hg.mozilla.org/integration/autoland/rev/a02ef614a87b Turn off downloads integration on Android M and later. r=snorp

This might be worth a mention in the release notes, but I'm not sure how best to formulate it. One thing to note is that this only applies to downloads downloaded after this change, i.e. we cannot retroactively prevent previous downloads from getting deleted once Firefox is uninstalled.

Added to 64beta release notes: Downloaded files are no longer deleted when uninstalling Firefox (note that this only applies to files downloaded after this update)

Hi, Verified as fixed on Beta 64.0b11, using Sebastian`s STR from Comment 14. Device: Google Pixel (Android 9.0) Thanks!