1070086 - files deleted from Firefox download manager are still listed in Android download manager

Status: VERIFIED FIXED

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

Description

[Christian Ascheberg]

STR:
- download file in Firefox
- remove finished download in Firefox download manager
- open Android downloads app

Result:
- download is still listed there, clicking it says file was not found

Expected:
- file should not be listed in Android download app anymore

Comment 2

[Christian Ascheberg]

I'd think this is the counterpart of bug 747351.
Bug 747351: delete in 3rd party app, check in Firefox
This bug: delete in Firefox, check in Android download app

Comment 3

[Aaron Train [:aaronmt]]

Is this something we have control over?

Comment 4

[Wesley Johnston (:wesj)]

There is a remove method on the DownloadManager. We'd have to maintain a list of ids (or maybe query for the download manager first and then remove it). I don't think we can do the opposite very well (yet).

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

Moving this code into its own module now.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This adds the removal code. We try to compare the info we have about the download with Android's data as best we can, but there isn't a ton to go on. We could start storing the source in Android's DB or something like that, but I intentionally avoided that to avoid data leakage.

RemoveAll also travels through this code path. It will wind up sending one message per download. That's not ideal, but not worth optimizing yet IMO (and this gives someone a chance to make a big huff in IRC about how its unoptimized someday). Working around it will require rewriting removeAll :(

Comment 7

[Wesley Johnston (:wesj)]

Looking at this code a bit more, maybe I should be using download-manager-remove-download-guid notifications as a better guide for when to remove things. Likewise, we can use the Downloads.jsm onDownloadRemoved callbacks for it:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm/DownloadList#addView%28%29

Will remake the second patch here...

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8497698 [details] [diff] [review]
Patch 1/2

Review of attachment 8497698 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/DownloadsIntegration.java
@@ +24,5 @@
> +
> +public class DownloadsIntegration
> +{
> +    private static final String LOGTAG = "GeckoDownloadsIntegration";
> +    @WrapElementForJNI

Nit: newline above

@@ +49,5 @@
> +            Context context = GeckoAppShell.getContext();
> +            GeckoMediaScannerClient.startScan(context, aFile, aMimeType);
> +        }
> +    }
> +    private static final class GeckoMediaScannerClient implements MediaScannerConnectionClient {

Nit: newline above

@@ +52,5 @@
> +    }
> +    private static final class GeckoMediaScannerClient implements MediaScannerConnectionClient {
> +        private final String mFile;
> +        private final String mMimeType;
> +        private MediaScannerConnection mScanner;

I guess this counts as new code now, so you could get rid of the m prefixes.

@@ +54,5 @@
> +        private final String mFile;
> +        private final String mMimeType;
> +        private MediaScannerConnection mScanner;
> +
> +        public static void startScan(Context context, String file, String mimeType) {

What's the point of this static method? Why not have callers just use the constructor?

@@ +62,5 @@
> +        private GeckoMediaScannerClient(Context context, String file, String mimeType) {
> +            mFile = file;
> +            mMimeType = mimeType;
> +            mScanner = new MediaScannerConnection(context, this);
> +            mScanner.connect();

It seems like an anti-pattern for the constructor to be doing actions beyond initialization. Maybe break this line off into a separate connect() method? Consider XMLHttpRequest.send(), Toast.show(), etc.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8497703 [details] [diff] [review]
Patch 2/2

Cancelling review because of comment 7.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8497703 [details] [diff] [review]
Patch 2/2

Putting this back up I guess. The download removed notifications are actually fired AFTER the download was removed, so while we have a guid for it we can't query for any other information about the download. For now we'll have to do things this way. Downloads.jsm does give us a better API, so I'll use that when we transition over.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8497703 [details] [diff] [review]
Patch 2/2

Review of attachment 8497703 [details] [diff] [review]:
-----------------------------------------------------------------

r- just for some questions about the time and status checks.

::: mobile/android/base/DownloadsIntegration.java
@@ +49,5 @@
> +        final private static int UNKNOWN_ID = -1;
> +
> +        public Download(final String path) {
> +            file = new File(path);
> +            this.date = file.lastModified();

Nit: date -> time

@@ +88,5 @@
> +            return file.equals(download.file) &&
> +                   name.equals(download.name) &&
> +                   description.equals(download.description) &&
> +                   size == download.size &&
> +                   date - download.date < 100;

Can you explain this date comparison? I assume the 100ms is some kind of fudge factor, but I'm not sure I understand what it's fixing. If the Android DLM entry is created by us, shouldn't they be exactly equal?

Also, you said there isn't much to go on, but I'm wondering if this actually might be too much -- particularly the date check. It seems like we could end up with false negatives. Any reason "file.equals(download.file)" alone wouldn't be enough?

@@ +142,5 @@
> +        final DownloadManager dm = (DownloadManager) GeckoAppShell.getContext().getSystemService(Context.DOWNLOAD_SERVICE);
> +
> +        Cursor c = null;
> +        try {
> +            c = dm.query((new DownloadManager.Query()).setFilterByStatus(DownloadManager.STATUS_SUCCESSFUL));

Is it possible to end up here for downloads that weren't successful (failed, paused, etc.)?

@@ +152,5 @@
> +            do {
> +                final Download d = Download.fromCursor(c);
> +                // Try hard as we can to verify this download is the one we think it is
> +                if (download.equals(d)) {
> +                    dm.remove(d.id);

Return early here?

Comment 12

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> Can you explain this date comparison? I assume the 100ms is some kind of
> fudge factor, but I'm not sure I understand what it's fixing. If the Android
> DLM entry is created by us, shouldn't they be exactly equal?

I was making a little guess here. But I think you're right. Lets just do a file == file. If someone decides to overwrite test.txt, we'll delete it, but that seems reasonable and edge casey enough I don't think we should worry about it.

> Is it possible to end up here for downloads that weren't successful (failed,
> paused, etc.)?

No. We only added downloads here when we get a success message in Gecko.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated. This just does a file-to-file comparison.

Comment 14

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/4a19faedc32b
https://hg.mozilla.org/integration/fx-team/rev/3980391d39b8

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8497698 [details] [diff] [review]
Patch 1/2

Approval Request Comment
[Feature/regressing bug #]: 816318
[User impact if declined]: Files show up in the Android database after removed from Firefox
[Describe test coverage new/current, TBPL]: We can't test the Android database yet. Needs some local tests though...
[Risks and why]: Low risk. This patch just moves code. The second patch is informing java of removals. If the remove fails, the download just shows up in the system download manager. I have a feeling that most users only know or check one of these anyway, so they're unlikely to notice the difference. 
[String/UUID change made/needed]: none

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 8502770 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: 816318
[User impact if declined]: Files show up in the Android database after removed from Firefox
[Describe test coverage new/current, TBPL]: We can't test the Android database yet. Needs some local tests though...
[Risks and why]: Low risk. This patch just moves code. The second patch is informing java of removals. If the remove fails, the download just shows up in the system download manager. I have a feeling that most users only know or check one of these anyway, so they're unlikely to notice the difference. 
[String/UUID change made/needed]: none

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a19faedc32b
https://hg.mozilla.org/mozilla-central/rev/3980391d39b8

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0cc0e528de6c
https://hg.mozilla.org/releases/mozilla-aurora/rev/2a88773b9b67

Comment 19

[Richard Newman [:rnewman]]

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

Comment 20

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Builds:
Firefox for Android 36.0a1 (2014-10-23)
Firefox for Android 35.0a2 (2014-10-23)
Device: Nexus 4 (Android 4.4.4)