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

VERIFIED FIXED in Firefox 37

Status

()

defect
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: cos_flaviu, Assigned: Margaret)

Tracking

Trunk
Firefox 39
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 unaffected, firefox37+ verified, firefox38+ verified, firefox39+ verified, fennec37+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Sounds like a regression from bug 901360.
Blocks: 901360
tracking-fennec: --- → ?
Flags: needinfo?(flaviu.cos)
We already have a likely regressor. Spending time on a regression range for this is not needed.
Flags: needinfo?(flaviu.cos)
Tracking this privacy feature related regression.
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 37+
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?
Flags: needinfo?(paolo.mozmail)
(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
Flags: needinfo?(paolo.mozmail)
/r/5491 - Bug 1141550 - Register an AsyncShutdown blocker to persist download changes. r=paolo

Pull down this commit:

hg pull review -r 8b93c5a0b693240e07a630d5f45ab40a5df0016a
Attachment #8578275 - Flags: review?(paolo.mozmail)
(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 :)
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.
 */
Attachment #8578275 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8578275 [details]
MozReview Request: bz://1141550/margaret

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

Ship It!
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
Attachment #8578275 - Flags: review+ → review?(paolo.mozmail)
Comment on attachment 8578275 [details]
MozReview Request: bz://1141550/margaret

Landed with the updated comment:

https://hg.mozilla.org/integration/fx-team/rev/0743512ea4cc
Attachment #8578275 - Flags: review?(paolo.mozmail)
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.
Attachment #8578275 - Flags: approval-mozilla-beta?
Attachment #8578275 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0743512ea4cc
https://hg.mozilla.org/mozilla-central/rev/b9b9452968e3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Attachment #8578275 - Flags: approval-mozilla-beta?
Attachment #8578275 - Flags: approval-mozilla-beta+
Attachment #8578275 - Flags: approval-mozilla-aurora?
Attachment #8578275 - Flags: approval-mozilla-aurora+
Verified as fixed in build 39.0a1 2015-03-17;
Device: Sony Xperia Z2 (Android 4.4.4);
Now that the fix has been verified, can you please request uplift for Beta and Aurora?
Flags: needinfo?(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.
Flags: needinfo?(margaret.leibovic)
(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.
Verified as fixed in builds:
- 37 Beta 8;
- 38.0a2 (2015-03-24)
Device: Asus Transformer Tab (Android 4.2.1).
Status: RESOLVED → VERIFIED
Depends on: 1163937
Attachment #8578275 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.