Closed Bug 1070086 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: Download Manager, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- verified
firefox36 --- verified
fennec 35+ ---

People

(Reporter: c.ascheberg, Assigned: wesj)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 747351
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
Is this something we have control over?
Flags: needinfo?(wjohnston)
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).
Flags: needinfo?(wjohnston)
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
tracking-fennec: --- → ?
tracking-fennec: ? → 35+
Assignee: nobody → wjohnston
Attached patch Patch 1/2Splinter Review
Moving this code into its own module now.
Attachment #8497698 - Flags: review?(bnicholson)
Attached patch Patch 2/2 (obsolete) — Splinter Review
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 :(
Attachment #8497703 - Flags: review?(bnicholson)
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 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.
Attachment #8497698 - Flags: review?(bnicholson) → review+
Comment on attachment 8497703 [details] [diff] [review]
Patch 2/2

Cancelling review because of comment 7.
Attachment #8497703 - Flags: review?(bnicholson)
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.
Attachment #8497703 - Flags: review?(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?
Attachment #8497703 - Flags: review?(bnicholson) → review-
(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.
Attached patch PatchSplinter Review
Updated. This just does a file-to-file comparison.
Attachment #8497703 - Attachment is obsolete: true
Attachment #8502770 - Flags: review?(bnicholson)
Attachment #8502770 - Flags: review?(bnicholson) → review+
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
Attachment #8497698 - Flags: approval-mozilla-aurora?
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
Attachment #8502770 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4a19faedc32b
https://hg.mozilla.org/mozilla-central/rev/3980391d39b8
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Attachment #8502770 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8497698 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.