Clear browsing data on exit. #1213

mkarolin · 2019-01-04T17:39:07Z

Adds a new "On exit" tab to chrome://settings/clearBrowserData dialog
with the same options as on the "Advanced" tab and a "Save" button. By
default all "On exit" options are turned off. When an option is modified
from the current state the Save button becomes enabled and allows the
user to save the "on exit" options. The options are saved to profile's
preferences: new *OnExit preferences have been added for each option.
Clear browsing data on exit functionality is triggered from
BraveBrowserMainParts::PreShutdown overrides. For each loaded profile
the code checks if user selected any on exit options and if so then a
BrowsingDataRemover is called.
Adds BraveClearDataOnExitTest and BraveClearDataOnExitTwoBrowsersTest
browser tests that verify that:

the removal of the browsing data is triggered only when an "on exit"
preference is set;
the removal flags are constrcuted correctly;
the removal is called only when the browser is exiting (tests with
OTR/Guest/multiple profiles).

Submitter Checklist:

Test Plan:

Automated tests:
npm run test -- brave_browser_tests --filter=BraveClearDataOnExitTest.*
npm run test -- brave_browser_tests --filter=BraveClearDataOnExitTwoBrowsersTest.*

Manual tests:

1. Verify browsing history removal.

Start Brave browser with a clean profile,
Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Browsing History" box,
Navigate to brave://history/ and verify that it's empty,
In a new tab, navigate to any site (e.g. https://example.com),
Wait for the site to load, then close the tab,
Back in the brave://history/ tab, reload the page and verify that an entry for the site has appeared,
Exit Brave browser,
Start Brave browser again,
Navigate to brave://history/ and verify that it's empty.

2. Verify downloads history removal.

Start Brave browser with a clean profile,
Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Download History" box,
Navigate to brave://downloads/ and verify that it's empty,
In a new tab, navigate to any site (e.g. https://example.com),
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,
Back in the brave://downloads/ tab, reload the page and verify that an entry for the site has appeared,
Exit Brave browser,
Start Brave browser again,
Navigate to brave://downloads/ and verify that it's empty.

3. Verify cookies removal.

Start Brave browser with a clean profile,
Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Cookies and other site data" box,
Navigate to brave://settings/siteData and verify that it's empty,
In a new tab, navigate to any site that uses cookies (e.g. https://google.com),
Wait for the site to load, then close the tab,
Back in the brave://settings/siteData tab, reload the page and verify that entries for the site have appeared (cookies and possibly local storage),
Exit Brave browser,
Start Brave browser again,
Navigate to brave://settings/siteData and verify that it's empty. Note: on Linux, while cookies should be gone, the local storage will remain - this is not specific to clearing data on exit and behaves the same way when clearing data from the Advanced tab of Clear Browsing Data dialog.

4. Verify cache removal.

Start Brave browser with a clean profile,
Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Cached images and files" box,
Open cache location in the file system (for example, on Windows in C:\Users_yourusername_\AppData\Local\BraveSoftware\Brave-Browser[-Dev|Beta]\User Data\Default\Cache)
In Brave, in a new tab, navigate to any site (e.g. https://example.com),
Wait for the site to load, then close the tab,
Verify that new cache file(s) is create in the cache file system folder (typically starts with f_),
Exit Brave browser,
Verify that cache file(s) created in the cache file system folder in the above step have been removed.

Alternatively, on Windows you can use NirSoft's ChromeCacheView and point it to Brave's cache, verify the cache content before exiting the browser and after.

5. Verify passwords removal.

Start Brave browser with a clean profile,
Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Passwords and other sign-in data" box,
Navigate to brave://settings/passwords and verify that the Saved Passwords list is empty,
In a new tab, navigate to any site that requires signing-in (e.g. https://hotmail.com),
Sign in and when a popup shows offering to save the site password agree to do so,
Back in the brave://settings/passwords tab, reload the page and verify that an entry for the site has appeared in the Saved Passwords list,
Exit Brave browser,
Start Brave browser again,
Navigate to brave://settings/passwords and verify that the Saved Passwords list is empty.

6. Verify autofill form data removal.

Start Brave browser with a clean profile,
Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Autofill form data" box,
Navigate to brave://settings/payments and verify that the Payment methods list is empty,
Add a payment method and verify that it shows in the list,
Exit Brave browser,
Start Brave browser again,
Navigate to brave://settings/payments and verify that the Payment methods list is empty.

7. Verify content settings removal.

Start Brave browser with a clean profile,
Navigate to brave://settings/clearBrowserData, select "On exit" tab, check "Content settings" box,
Navigate to one or more of the content setting pages, for example, brave://settings/content/javascript and verify that the Block list is empty,
Add a site to the Block list and verify that it shows in the list,
Exit Brave browser,
Start Brave browser again,
Navigate to brave://settings/content/javascript and verify that the Block list is empty.

Note: this will also clear out per site Brave Shields settings.

8. Test that clearing data on exit and manually produce the same results (suggested by @fmarier, thx!)

Create a new profile and start the browser.
Wait 5 minutes and close the browser.
Make a copy ("original") of the profile directory.
Open the browser again, navigate to a site where you can login and save the password.
Add a payment method.
Go to a site to trigger a download.
Add a site to the block list.
Set "clear everything on exit".
Close the browser.
Diff the new profile directory with the original one (e.g. diff -Nurd original/ autocleared/ on Linux).
Repeat Steps 1-10 but instead of using "clear on exit", do a manual "clear everything".
Close the browser and diff that third profile with the other two.

This will allow us to:

see what residues are left after auto-clearing, and
confirm that auto-clearing is just as effective as manually clearing everything.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

...-browser-resources-settings-clear_browsing_data_dialog-clear_browsing_data_dialog.html.patch

...me-browser-resources-settings-clear_browsing_data_dialog-clear_browsing_data_dialog.js.patch

patches/chrome-browser-ui-browser.cc.patch

bridiver · 2019-01-29T23:01:47Z

browser/BUILD.gn

@@ -11,8 +11,18 @@ source_set("browser_process") {
    "bookmarks/brave_bookmark_client.h",
    "brave_browser_main_extra_parts.cc",
    "brave_browser_main_extra_parts.h",
+    "brave_browser_main_parts.cc",
+    "brave_browser_main_parts.h",
+    "brave_browser_main_parts_android.cc",


we don't need all of these brave_browser_main_parts_os files because none of them implement PreShutdown

ChromeBrowserMainParts inherit from ChromeBrowserMainParts (except Linux and Mac which both inherit from Posix). If I just subclass ChromeBrowserMainParts I would still have to change ChromeBrowserMainParts<Win,Android,Posix> to inherit from the new subclass (see instantiation in https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_client.cc?l=1150).
Also, while none of them override PreShutdown now I am not sure we can depend on it staying this way or, if they do override, that they'd call the base class implementation.

bridiver · 2019-01-29T23:07:25Z

browser/browsing_data/brave_clear_browsing_data.cc

+
+  int num_profiles_to_clear_ = 0;
+  base::RunLoop run_loop_;
+  ScopedObserver<content::BrowsingDataRemover,


I'm a little confused. The is a subclass of content::BrowsingDataRemover::Observer, but also has an internal ScopedObserver for content::BrowsingDataRemover::Observer?

I am following the patter suggested in the comment here: https://cs.chromium.org/chromium/src/content/public/browser/browsing_data_remover.h?l=41 but delegating keeping track of content::BrowsingDataRemover for each profile to ScopedObserver.

diracdeltas · 2019-02-05T21:06:19Z

Please open a security review issue for this when ready, thanks.

browser/brave_browser_main_parts_android.cc

...ces/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_dialog_behavior.html

petemill · 2019-02-28T01:54:54Z

The checkboxes in the dialog don't seem to be remembering state for me. But the save button knows that the state (with the boxes unchecked) is different to what is saved. i.e.:

Open the dialog to the 'On exit' tab, tick "Passwords and other sign-in data"
Click Save
Close tab and open settings again
Open the dialog again to the 'On exit' tab.
Problem:

"Passwords and other sign-in data is not ticked"

petemill · 2019-02-28T01:55:59Z

Looks like it works on Debug builds but not Release build. Confirming...

mkarolin · 2019-02-28T03:09:44Z

Just built Release, seems to be working for me.

petemill · 2019-02-28T03:31:31Z

sorry @mkarolin false alarm on both fronts it seems. I can't replicate the issue I had with the checkboxes either 🤷‍♂️

petemill

UI changes in settings are great. Thanks for looking at the minimized patching. After discussing again with @bridiver I have some improvements to make this and some other settings modifications 0-patch, but I'll do that in a follow-up.

bsclifton

Works great 😄

fmarier · 2019-02-28T19:07:19Z

browser/browsing_data/brave_clear_browsing_data.cc

+// static
+void BraveClearBrowsingData::ClearOnExit() {
+  TRACE_EVENT0("browser", "BraveClearBrowsingData::ClearOnExit");
+  // Do not clear browsing data when the OS is ending session (logout/reboot/


Ideally, it would be nice to document this somewhere that's user-visible. If it's too much information for the UI, maybe a page on support.brave.com?

@bsclifton, @rebron ^^

@fmarier support.brave.com might not be the right audience for that- but we can definitely document custom trace events in our wiki. @mkarolin I created a stub page, if you wanted to add that (and describe why you'd use it):
https://github.com/brave/brave-browser/wiki/Custom-tracing-events

@bsclifton, I understood it that @fmarier was referring to the comment that we are not clearing browsing data on logout/reboot/shutdown.

@mkarolin ah- LOL; that makes a lot more sense 😂

cc'ing @Brave-Matt on that one - would be great to document this feature in general 😄 @Brave-Matt: this would be something we're planning for 0.63.x

Sorry, what I was referring to was not the trace events, but the limitation that if you shutdown your computer, it won't clear your history.

That is a very bad limitation, which would probably surprise a lot of people who use the feature. IMO, we should back this out and fix that problem.

cc: @rebron for the above comment by @tomlowenthal ^^

We may want to compare with Muon- it's possible Muon had that same limitation also. If the current experience is WORSE than Muon (ex: Muon works as expected), then I definitely agree that we should consider hiding this new UI until that problem is resolved

@tomlowenthal should brave/brave-browser#3549 cover this use-case also?

Yeah, that issue is intended to be the new partial fix for this… someday.

fmarier · 2019-02-28T19:10:20Z

browser/browsing_data/brave_clear_browsing_data_browsertest.cc

+  }
+
+  int GetRemoveMaskAll() {
+    return ChromeBrowsingDataRemoverDelegate::DATA_TYPE_HISTORY |


Should "Hosted App Data" be included here?

"Hosted App Data" is defined as a combination of mask = ChromeBrowsingDataRemoverDelegate::DATA_TYPE_SITE_DATA and origin = content::BrowsingDataRemover::ORIGIN_TYPE_PROTECTED_WEB, so it should be already accounted for.

fmarier · 2019-02-28T19:15:27Z

I noticed that "Media licences" is missing from the list of things that can be cleared on shutdown, even though it's part of the Advanced tab. Is that intentional?

mkarolin · 2019-02-28T19:36:22Z

I noticed that "Media licences" is missing from the list of things that can be cleared on shutdown, even though it's part of the Advanced tab. Is that intentional?

@fmarier, "Media licenses" is no longer a part of the Advanced tab in Chromium:

commit 3dece3d74d88a717a3a1e77eea8916e48847d8e8
Author: John Rummell
Date:   Thu Jan 10 00:34:57 2019 +0000
Remove "Media Licenses" from Clear Browsing Data dialog
Media licenses will now be deleted when "Cookies and site data" is selected.
BUG=901993

bridiver · 2019-03-01T03:19:07Z

browser/browsing_data/brave_clear_browsing_data.cc

+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/common/pref_names.h"
+#include "components/browsing_data/core/pref_names.h"


you need to a dep for this here https://github.com/brave/brave-core/pull/1213/files#diff-84f8700538180eec90bb2f4c9dff4da4R79

Fixes brave/brave-browser#492 1. Adds a new "On exit" tab to chrome://settings/clearBrowserData dialog with the same options as on the "Advanced" tab and a "Save" button. By default all "On exit" options are turned off. When an option is modified from the current state the Save button becomes enabled and allows the user to save the "on exit" options. The options are saved to profile's preferences: new *OnExit preferences have been added for each option. 2. Clear browsing data on exit functionality is triggered from BraveBrowserMainParts::PreShutdown override. For each loaded profile the code checks if user selected any on exit options and if so then a BrowsingDataRemover is called. 3. Adds BraveClearDataOnExitTest and BraveClearDataOnExitTwoBrowsersTest browser tests that verify that: - the removal of the browsing data is triggered only when an "on exit" preference is set; - the removal flags are constrcuted correctly; - the removal is called only when the browser is exiting (tests with OTR/Guest/multiple profiles).

mkarolin self-assigned this Jan 4, 2019

mkarolin force-pushed the maxk-clear-browsing-data-on-exit branch 4 times, most recently from 250044f to edd9a56 Compare January 7, 2019 20:31

mkarolin changed the title ~~WIP: Clear browsing data on exit.~~ Clear browsing data on exit. Jan 7, 2019

mkarolin requested review from yrliou, bridiver, bsclifton and petemill January 7, 2019 20:42

bridiver reviewed Jan 9, 2019

View reviewed changes

...-browser-resources-settings-clear_browsing_data_dialog-clear_browsing_data_dialog.html.patch Outdated Show resolved Hide resolved

bridiver reviewed Jan 9, 2019

View reviewed changes

...me-browser-resources-settings-clear_browsing_data_dialog-clear_browsing_data_dialog.js.patch Show resolved Hide resolved

bridiver reviewed Jan 9, 2019

View reviewed changes

patches/chrome-browser-ui-browser.cc.patch Outdated Show resolved Hide resolved

mkarolin changed the title ~~Clear browsing data on exit.~~ WIP: Clear browsing data on exit. Jan 15, 2019

mkarolin added the pr/work-in-progress label Jan 15, 2019

mkarolin force-pushed the maxk-clear-browsing-data-on-exit branch 4 times, most recently from dc1098f to a74e5cd Compare January 24, 2019 23:37

bridiver reviewed Jan 29, 2019

View reviewed changes

mkarolin force-pushed the maxk-clear-browsing-data-on-exit branch 2 times, most recently from a55cd20 to 3fc55a9 Compare February 2, 2019 00:58

mkarolin changed the title ~~WIP: Clear browsing data on exit.~~ Clear browsing data on exit. Feb 5, 2019

mkarolin removed the pr/work-in-progress label Feb 5, 2019

mkarolin added the PR/Pending Review label Feb 5, 2019

bsclifton added this to the 0.62.x - Nightly milestone Feb 7, 2019

srirambv mentioned this pull request Feb 7, 2019

Option to clear browsing data on exit brave/brave-browser#492

Closed

bsclifton reviewed Feb 7, 2019

View reviewed changes

browser/brave_browser_main_parts_android.cc Outdated Show resolved Hide resolved

mkarolin modified the milestones: 0.62.x - Dev, 0.63.x - Nightly Feb 26, 2019

petemill requested changes Feb 27, 2019

View reviewed changes

...ces/settings/brave_clear_browsing_data_dialog/brave_clear_browsing_data_dialog_behavior.html Outdated Show resolved Hide resolved

mkarolin force-pushed the maxk-clear-browsing-data-on-exit branch from 5982a91 to bf99490 Compare February 28, 2019 00:53

petemill previously approved these changes Feb 28, 2019

View reviewed changes

bsclifton previously approved these changes Feb 28, 2019

View reviewed changes

fmarier reviewed Feb 28, 2019

View reviewed changes

bridiver reviewed Mar 1, 2019

View reviewed changes

mkarolin dismissed stale reviews from bsclifton and petemill via 02e63a9 March 1, 2019 03:43

mkarolin force-pushed the maxk-clear-browsing-data-on-exit branch from bf99490 to 02e63a9 Compare March 1, 2019 03:43

bridiver approved these changes Mar 1, 2019

View reviewed changes

mkarolin merged commit 96a8582 into master Mar 1, 2019

mkarolin removed the PR/Pending Review label Mar 1, 2019

This was referenced Mar 8, 2019

Reduce patching for webui polymer component behavior injection brave/brave-browser#3631

Closed

WebUI: Hook in to Polymer in order to add behaviors to components. #1802

Merged

fmarier mentioned this pull request Mar 12, 2019

WIP: Rewards data can now be cleared #1187

Closed

18 tasks

LaurenWags mentioned this pull request Apr 4, 2019

Download History doesn't clear on exit - follow up to 492 brave/brave-browser#4004

Closed

mihaiplesa deleted the maxk-clear-browsing-data-on-exit branch May 13, 2019 10:55

This was referenced Jan 17, 2022

PC shutdown causes Brave application to exit before data can be cleared brave/brave-browser#16889

Open

Brave doesn't clear browsing data on exit until all user profiles are closed brave/brave-browser#15205

Open

btlechowski mentioned this pull request Jul 29, 2022

Fixes state-level ad targeting setting is not shown #14381

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear browsing data on exit. #1213

Clear browsing data on exit. #1213

mkarolin commented Jan 4, 2019 •

edited by bridiver

Loading

bridiver Jan 29, 2019 •

edited

Loading

mkarolin Jan 30, 2019

bridiver Jan 29, 2019

mkarolin Jan 30, 2019

diracdeltas commented Feb 5, 2019

petemill commented Feb 28, 2019

petemill commented Feb 28, 2019

mkarolin commented Feb 28, 2019

petemill commented Feb 28, 2019

petemill left a comment

bsclifton left a comment

fmarier Feb 28, 2019

mkarolin Feb 28, 2019

bsclifton Feb 28, 2019 •

edited

Loading

mkarolin Feb 28, 2019

bsclifton Feb 28, 2019 •

edited

Loading

fmarier Feb 28, 2019

tildelowengrimm Mar 1, 2019

bsclifton Mar 4, 2019

bsclifton Mar 4, 2019

tildelowengrimm Mar 7, 2019

fmarier Feb 28, 2019

mkarolin Feb 28, 2019

fmarier commented Feb 28, 2019

mkarolin commented Feb 28, 2019 •

edited

Loading

bridiver Mar 1, 2019

mkarolin Mar 1, 2019

Clear browsing data on exit. #1213

Clear browsing data on exit. #1213

Conversation

mkarolin commented Jan 4, 2019 • edited by bridiver Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

bridiver Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Feb 5, 2019

petemill commented Feb 28, 2019

petemill commented Feb 28, 2019

mkarolin commented Feb 28, 2019

petemill commented Feb 28, 2019

petemill left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarier commented Feb 28, 2019

mkarolin commented Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin commented Jan 4, 2019 •

edited by bridiver

Loading

bridiver Jan 29, 2019 •

edited

Loading

bsclifton Feb 28, 2019 •

edited

Loading

bsclifton Feb 28, 2019 •

edited

Loading

mkarolin commented Feb 28, 2019 •

edited

Loading