1141550 - Downloads are not cleared from about:downloads when "Clear on exit" is used

Status: VERIFIED FIXED

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

Description

[Flaviu Cos]

Environment: 
Device: Motorola Razr (Android 4.1.2);
Build: Nightly 39.0a1 (2015-03-08);

Steps to reproduce:
1. Launch Fennec;
2. Download some files (e.g.: 1.usa.gov/deeXKM);
3. Go to Settings -> Privacy -> Clear on exit;
4. Check "Downloads" and tap Set;
5. Tap Options and select "Quit" from the menu;
6. Relaunch Fennec and go to about: downloads.

Expected result:
The downloaded files are deleted from device and from about:downloads.

Actual result:
The files are deleted from device but they still appear in about:downloads.

Comment 1

[:Margaret Leibovic]

Sounds like a regression from bug 901360.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

We already have a likely regressor. Spending time on a regression range for this is not needed.

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking this privacy feature related regression.

Comment 4

[:Margaret Leibovic]

Investigating this a bit, our Sanitizer code is definitely called:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm?force=1#183

Also, it appears the file is properly being removed, because I'm seeing an error when I try to open it after re-opening the browser.

When I clear private data directly from settings, the download does disappear from about:downloads, so I'm wondering if something just isn't being flushed properly on shutdown.

Paolo, is there anything we can call after removing a download from the list to make sure that change persists?

Comment 5

[:Paolo Amadini]

(In reply to :Margaret Leibovic from comment #4)
> Paolo, is there anything we can call after removing a download from the list
> to make sure that change persists?

Hm, so I think what happens is that changes to the download store aren't persisted by the DownloadAutoSaveView if they happen late just before shutdown:

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

We could have the DownloadAutoSaveView register an AsyncShutdown blocker like the most recent LoginStore does:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginStore.jsm#116

I believe DeferredTask.finalize should take care of everything for us in that case. If this works, we should also tweak the comment here:

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

Comment 6

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1141550/margaret

/r/5491 - Bug 1141550 - Register an AsyncShutdown blocker to persist download changes. r=paolo

Pull down this commit:

hg pull review -r 8b93c5a0b693240e07a630d5f45ab40a5df0016a

Comment 7

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > Paolo, is there anything we can call after removing a download from the list
> > to make sure that change persists?
> 
> Hm, so I think what happens is that changes to the download store aren't
> persisted by the DownloadAutoSaveView if they happen late just before
> shutdown:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/
> src/DownloadIntegration.jsm#1206
> 
> We could have the DownloadAutoSaveView register an AsyncShutdown blocker
> like the most recent LoginStore does:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/
> LoginStore.jsm#116

Thanks for the suggestion! I verified this works.

> I believe DeferredTask.finalize should take care of everything for us in
> that case. If this works, we should also tweak the comment here:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/
> src/DownloadIntegration.jsm#89

I'm not confident that I updated this correctly, so please let me know how I should update my change if necessary :)

Comment 8

[:Paolo Amadini]

I would just reduce the comment like this, since most of the concerns go away:

/**
 * Indicates the delay between a change to the downloads data and the related
 * save operation.
 *
 * For best efficiency, this value should be high enough that the input/output
 * for opening or closing the target file does not overlap with the one for
 * saving the list of downloads.
 */

Comment 9

[:Paolo Amadini]

Comment on attachment 8578275 [details]
MozReview Request: bz://1141550/margaret

https://reviewboard.mozilla.org/r/5489/#review4431

Ship It!

Comment 10

[:Margaret Leibovic]

Comment on attachment 8578275 [details]
MozReview Request: bz://1141550/margaret

/r/5491 - Bug 1141550 - Register an AsyncShutdown blocker to persist download changes. r=paolo

Pull down this commit:

hg pull review -r 0380ee9fc10d75e11c1f29db74f85a24708c36ce

Comment 11

[:Margaret Leibovic]

Comment on attachment 8578275 [details]
MozReview Request: bz://1141550/margaret

Landed with the updated comment:

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

Comment 12

[:Margaret Leibovic]

Comment on attachment 8578275 [details]
MozReview Request: bz://1141550/margaret

Approval Request Comment
[Feature/regressing bug #]: bug 901360

[User impact if declined]: Downloads won't be properly cleared on shutdown (if that's what the user has set as a preference).

[Describe test coverage new/current, TreeHerder]: Tested locally, no automated tests for this particular use case.

[Risks and why]: Low-risk, adds a small piece of logic to ensure we save downloads changes on shutdown. Note: this is shared desktop/mobile code, so we should verify this doesn't cause any issues on desktop.

[String/UUID change made/needed]: None.

Comment 13

[:Margaret Leibovic]

Oops! Missing import:
https://hg.mozilla.org/integration/fx-team/rev/b9b9452968e3

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0743512ea4cc
https://hg.mozilla.org/mozilla-central/rev/b9b9452968e3

Comment 15

[Flaviu Cos]

Verified as fixed in build 39.0a1 2015-03-17;
Device: Sony Xperia Z2 (Android 4.4.4);

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Now that the fix has been verified, can you please request uplift for Beta and Aurora?

Comment 17

[:Margaret Leibovic]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #16)
> Now that the fix has been verified, can you please request uplift for Beta
> and Aurora?

I already requested uplift and it was granted... so I think we can just uplift these.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4aa3cc799504

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3eeb35bbafd2

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to :Margaret Leibovic from comment #17)
> I already requested uplift and it was granted... so I think we can just
> uplift these.

So you did. Not sure how I missed that.

Comment 21

[Flaviu Cos]

Verified as fixed in builds:
- 37 Beta 8;
- 38.0a2 (2015-03-24)
Device: Asus Transformer Tab (Android 4.2.1).

Comment 23

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1141550 - Register an AsyncShutdown blocker to persist download changes. r=paolo