Download History doesn't clear on exit - follow up to 492

[LaurenWags]

Description

Found while testing #492

If you select only Download History to clear on exit, it is not cleared.

Steps to Reproduce

Taken from brave/brave-core#1213

1. Start Brave browser with a clean profile,
2. Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Download History" box. Select Save.
3. Navigate to brave://downloads and verify that it's empty
4. In a new tab, navigate to any site (e.g. https://example.com)
5. Wait for the site to load, right-click on the page and select Save As..., save the page and wait for the download to complete; then close the tab
6. Back in the brave://downloads tab, reload the page and verify that an entry for the site has appeared
7. Exit Brave browser
8. Start Brave browser again
9. Navigate to brave://downloads

Actual result:

brave://downloads is not empty

Expected result:

brave://downloads should be empty

Reproduces how often:

easily

Brave version (brave://version info)

Brave	0.63.31 Chromium: 73.0.3683.75 (Official Build) beta(64-bit)
Revision	909ee014fcea6828f9a610e6716145bc0b3ebf4a-refs/branch-heads/3683@{#803}
OS	Mac OS X

Reproducible on current release: n/a

• Does it reproduce on brave-browser dev/beta builds? yes

Brave	0.64.36 Chromium: 73.0.3683.75 (Official Build) dev(64-bit)
Revision	909ee014fcea6828f9a610e6716145bc0b3ebf4a-refs/branch-heads/3683@{#803}
OS	Mac OS X

Website problems only:

• Does the issue resolve itself when disabling Brave Shields? n/a
• Is the issue reproducible on the latest version of Chrome? n/a

Additional Information

A few other things I checked:

1. If you select both 'Browsing History' and 'Download History' to clear on exit, then both are cleared.
2. If you go to 'Advanced' instead of 'On Exit' and select only Download History, and then select 'Clear data' button, the download history is cleared.

[rebron]

I was able to reproduce on 0.65.16 Chromium: 74.0.3729.40 (Official Build) nightly (64-bit) on macOS. Selecting another item checkbox does clear Downloads but Downloads by itself does not clear on exit.

[rebron]

cc: @mkarolin

[srirambv]

On Linux, even though there is an item listed in download, clear data modal shows Download items as None

[btlechowski]

Good find @LaurenWags!

Reproduced on

Brave	0.63.31 Chromium: 73.0.3683.75 (Official Build) beta (64-bit)
Revision	909ee014fcea6828f9a610e6716145bc0b3ebf4a-refs/branch-heads/3683@{#803}
OS	Windows 10 OS Build 17134.523

[mkarolin]

Confirmed. The issue is that BrowsingDataRemoverImpl::RemoveImpl doesn't actually wait for the removal, but only schedules it.

Specifically, for the removal of download history BrowsingDataRemoverImpl::RemoveImpl calls DownloadManager::RemoveDownloadsByURLAndTime which locates matching downloads and then in a loop calls download::DownloadItemImpl::Remove for each download. That, in turn, notifies DownloadHistory via DownloadHistory::OnDownloadRemoved, which ScheduleRemoveDownload by posting a task back to the UI thread. While the task hasn't run, DownloadHistory keeps batching removal requests for the same task. Once the loop in DownloadManager::RemoveDownloadsByURLAndTime is done it doesn't wait for DownloadHistory's task to execute.

In our scenario, at this point we proceed with the shutdown so the task doesn't run.

In the scenario without the shutdown, once the task runs, DownloadHistory::RemoveDownloadsBatch calls to HistoryService::RemoveDownloads, which again only schedules a task to run HistoryBackend::RemoveDownloads on the back end.

We need to figure out how to wait for the back end to finish the job. DownloadHistory::RemoveDownloadsBatch does call observers with OnDownloadsRemoved but that only notifies that the HistoryService scheduled removal. HistoryBackendObserver doesn't have a callback for downloads removal.

[tneo]

Given that Brave is not removing history properly I installed this. As that works like a charm. How do they handle the removal of history?

[fmarier]

The extension that @tneo found clears this info on startup instead of shutdown, which is a good approach to tackle this race condition. Let's follow up in #3673.