From fc5704ee80d576aaedd75530dee88f80aaf1c28f Mon Sep 17 00:00:00 2001 From: tech-zilla Date: Sat, 6 Jul 2024 15:40:13 +0300 Subject: [PATCH 1/7] When opening the browser in incognito mode from command line, we should prevent deleting the origins marked with "Forget me when I close this site". The reason for this is that the incognito window does not re-create the lifetime of TLD from the main profile and as a result, they get scheduled for deletion. --- components/ephemeral_storage/ephemeral_storage_service.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/components/ephemeral_storage/ephemeral_storage_service.cc b/components/ephemeral_storage/ephemeral_storage_service.cc index 5711652feba1..b9151c4d8df6 100644 --- a/components/ephemeral_storage/ephemeral_storage_service.cc +++ b/components/ephemeral_storage/ephemeral_storage_service.cc @@ -327,6 +327,13 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { DCHECK(!context_->IsOffTheRecord()); + + Profile* profile = Profile::FromBrowserContext(context_); + if (profile->HasAnyOffTheRecordProfile()) { + first_party_storage_areas_to_cleanup_on_startup_.clear(); + return; + } + ScopedListPrefUpdate pref_update(prefs_, kFirstPartyStorageOriginsToCleanup); for (const auto& url_to_cleanup : first_party_storage_areas_to_cleanup_on_startup_) { From 3e0beac8861c932224ebbe39b1823aede7b92a79 Mon Sep 17 00:00:00 2001 From: tech-zilla Date: Sat, 13 Jul 2024 16:55:40 +0300 Subject: [PATCH 2/7] Address review remarks --- .../ephemeral_storage_service.cc | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/components/ephemeral_storage/ephemeral_storage_service.cc b/components/ephemeral_storage/ephemeral_storage_service.cc index b9151c4d8df6..089658e89cde 100644 --- a/components/ephemeral_storage/ephemeral_storage_service.cc +++ b/components/ephemeral_storage/ephemeral_storage_service.cc @@ -15,6 +15,10 @@ #include "base/timer/timer.h" #include "brave/components/ephemeral_storage/ephemeral_storage_pref_names.h" #include "brave/components/ephemeral_storage/url_storage_checker.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_finder.h" +#include "chrome/browser/ui/browser_window.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/prefs/scoped_user_pref_update.h" #include "components/user_prefs/user_prefs.h" @@ -35,6 +39,14 @@ GURL GetFirstPartyStorageURL(const std::string& ephemeral_domain) { return GURL(base::StrCat({url::kHttpsScheme, "://", ephemeral_domain})); } +bool DoesProfileHaveAnyBrowserWindow(Profile* profile) { + for (Browser* browser : chrome::FindAllBrowsersWithProfile(profile)) { + if (browser->window()) + return true; + } + return false; +} + } // namespace EphemeralStorageService::EphemeralStorageService( @@ -327,13 +339,11 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { DCHECK(!context_->IsOffTheRecord()); - Profile* profile = Profile::FromBrowserContext(context_); - if (profile->HasAnyOffTheRecordProfile()) { + if (!DoesProfileHaveAnyBrowserWindow(profile)) { first_party_storage_areas_to_cleanup_on_startup_.clear(); return; } - ScopedListPrefUpdate pref_update(prefs_, kFirstPartyStorageOriginsToCleanup); for (const auto& url_to_cleanup : first_party_storage_areas_to_cleanup_on_startup_) { From ce5b6a820a1005acc597e2e88b6a576054be2b5f Mon Sep 17 00:00:00 2001 From: tech-zilla Date: Thu, 25 Jul 2024 12:31:50 +0300 Subject: [PATCH 3/7] Add browser tests --- .../ephemeral_storage_browsertest.cc | 10 ++-- .../ephemeral_storage_browsertest.h | 2 +- ...l_storage_forget_by_default_browsertest.cc | 56 ++++++++++++++++++- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/browser/ephemeral_storage/ephemeral_storage_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_browsertest.cc index 2a0e6e6615a3..99230aef1d3b 100644 --- a/browser/ephemeral_storage/ephemeral_storage_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_browsertest.cc @@ -309,12 +309,12 @@ content::EvalJsResult EphemeralStorageBrowserTest::GetCookiesInFrame( return content::EvalJs(host, "document.cookie"); } -size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Browser* b) { - if (!b) { - b = browser(); +size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Profile* profile) { + if (!profile) { + profile = browser()->profile(); } const size_t fired_cnt = EphemeralStorageServiceFactory::GetInstance() - ->GetForContext(b->profile()) + ->GetForContext(profile) ->FireCleanupTimersForTesting(); // NetworkService closes existing connections when a data removal action @@ -323,7 +323,7 @@ size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Browser* b) { // sure the queued Ephemeral Storage cleanup was complete. BrowsingDataRemoverObserver data_remover_observer; content::BrowsingDataRemover* remover = - b->profile()->GetBrowsingDataRemover(); + profile->GetBrowsingDataRemover(); remover->AddObserver(&data_remover_observer); remover->RemoveAndReply(base::Time(), base::Time::Max(), 0, 0, &data_remover_observer); diff --git a/browser/ephemeral_storage/ephemeral_storage_browsertest.h b/browser/ephemeral_storage/ephemeral_storage_browsertest.h index 4f54fb626bdc..c6f99437495e 100644 --- a/browser/ephemeral_storage/ephemeral_storage_browsertest.h +++ b/browser/ephemeral_storage/ephemeral_storage_browsertest.h @@ -88,7 +88,7 @@ class EphemeralStorageBrowserTest : public InProcessBrowserTest { StorageType storage_type); void SetCookieInFrame(content::RenderFrameHost* host, std::string cookie); content::EvalJsResult GetCookiesInFrame(content::RenderFrameHost* host); - size_t WaitForCleanupAfterKeepAlive(Browser* browser = nullptr); + size_t WaitForCleanupAfterKeepAlive(Profile* profile = nullptr); void ExpectValuesFromFramesAreEmpty(const base::Location& location, const ValuesFromFrames& values); void ExpectValuesFromFrameAreEmpty(const base::Location& location, diff --git a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc index f8cc752d4f52..2157b8c345d3 100644 --- a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc @@ -10,6 +10,7 @@ #include "chrome/browser/content_settings/host_content_settings_map_factory.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" +#include "chrome/common/chrome_switches.h" #include "chrome/test/base/ui_test_utils.h" #include "components/content_settings/core/browser/cookie_settings.h" #include "components/content_settings/core/browser/host_content_settings_map.h" @@ -206,7 +207,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultBrowserTest, // After keepalive values should be cleared. ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser, b_site_ephemeral_storage_url_)); - WaitForCleanupAfterKeepAlive(incognito_browser); + WaitForCleanupAfterKeepAlive(incognito_browser->profile()); ASSERT_TRUE(ui_test_utils::NavigateToURL(incognito_browser, a_site_ephemeral_storage_url_)); @@ -570,4 +571,57 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultDisabledBrowserTest, EXPECT_EQ(1u, GetAllCookies().size()); } +class EphemeralStorageForgetByDefaultIncognitoBrowserTest + : public EphemeralStorageForgetByDefaultBrowserTest { + public: + EphemeralStorageForgetByDefaultIncognitoBrowserTest() { + scoped_feature_list_.InitAndEnableFeature( + net::features::kBraveForgetFirstPartyStorage); + } + ~EphemeralStorageForgetByDefaultIncognitoBrowserTest() override = default; + + void SetUpCommandLine(base::CommandLine* command_line) override { + EphemeralStorageForgetByDefaultBrowserTest::SetUpCommandLine(command_line); + if (IsPreTest()) { + command_line->AppendSwitch(switches::kIncognito); + } + } + + static bool IsPreTest() { + const testing::TestInfo* const test_info = + testing::UnitTest::GetInstance()->current_test_info(); + return base::StartsWith(test_info->name(), "PRE_DontForgetFirstParty"); + } + + private: + base::test::ScopedFeatureList scoped_feature_list_; +}; + +IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultIncognitoBrowserTest, + PRE_PRE_DontForgetFirstPartyIfNoBrowserWindowIsActive) { + const GURL a_site_set_cookie_url( + "https://a.com/set-cookie?name=acom;path=/" + ";SameSite=None;Secure;Max-Age=600"); + brave_shields::SetForgetFirstPartyStorageEnabled(content_settings(), true, + a_site_set_cookie_url); + + // Cookies should NOT exist for a.com. + EXPECT_EQ(0u, GetAllCookies().size()); + + EXPECT_TRUE(LoadURLInNewTab(a_site_set_cookie_url)); + + // Cookies SHOULD exist for a.com. + EXPECT_EQ(1u, GetAllCookies().size()); +} + +IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultIncognitoBrowserTest, + PRE_DontForgetFirstPartyIfNoBrowserWindowIsActive) { + EXPECT_TRUE(browser()->profile()->IsOffTheRecord()); + WaitForCleanupAfterKeepAlive(browser()->profile()->GetOriginalProfile()); +} + +IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultIncognitoBrowserTest, + DontForgetFirstPartyIfNoBrowserWindowIsActive) { + EXPECT_EQ(1u, GetAllCookies().size()); +} } // namespace ephemeral_storage From 1ab7e162c7b9cb39da774d3cbd56d7e984ca285e Mon Sep 17 00:00:00 2001 From: tech-zilla Date: Fri, 26 Jul 2024 10:10:26 +0300 Subject: [PATCH 4/7] Attempt to fix pre-submit checks --- .../brave_ephemeral_storage_service_delegate.cc | 14 ++++++++++++++ .../brave_ephemeral_storage_service_delegate.h | 3 +++ .../ephemeral_storage_browsertest.cc | 6 +++--- ...ral_storage_forget_by_default_browsertest.cc | 3 +-- .../ephemeral_storage_service.cc | 17 ++++------------- .../ephemeral_storage_service_delegate.h | 3 +++ 6 files changed, 28 insertions(+), 18 deletions(-) diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc index 00c6a0f83faa..b0d142ca39fb 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc @@ -9,6 +9,8 @@ #include "brave/components/brave_shields/content/browser/brave_shields_util.h" #include "chrome/browser/browsing_data/chrome_browsing_data_remover_constants.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser_finder.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browsing_data_filter_builder.h" @@ -111,4 +113,16 @@ void BraveEphemeralStorageServiceDelegate::CleanupFirstPartyStorageArea( origin_type, std::move(filter_builder)); } +void BraveEphemeralStorageServiceDelegate::DoesProfileHaveAnyBrowserWindow( + Profile* profile, + bool* have_window) { + for (Browser* browser : chrome::FindAllBrowsersWithProfile(profile)) { + if (browser->window()) { + *have_window = true; + return; + } + } + *have_window = false; +} + } // namespace ephemeral_storage diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h index d1024699350b..95a285b84e37 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h @@ -13,6 +13,7 @@ #include "brave/components/ephemeral_storage/ephemeral_storage_service_delegate.h" #include "components/content_settings/core/browser/cookie_settings.h" +class Profile; namespace content { class BrowserContext; } @@ -33,6 +34,8 @@ class BraveEphemeralStorageServiceDelegate void CleanupTLDEphemeralArea(const TLDEphemeralAreaKey& key) override; void CleanupFirstPartyStorageArea( const std::string& registerable_domain) override; + void DoesProfileHaveAnyBrowserWindow(Profile* profile, + bool* have_window) override; private: raw_ptr context_ = nullptr; diff --git a/browser/ephemeral_storage/ephemeral_storage_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_browsertest.cc index 99230aef1d3b..6bb2e0f29406 100644 --- a/browser/ephemeral_storage/ephemeral_storage_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_browsertest.cc @@ -309,7 +309,8 @@ content::EvalJsResult EphemeralStorageBrowserTest::GetCookiesInFrame( return content::EvalJs(host, "document.cookie"); } -size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Profile* profile) { +size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive( + Profile* profile) { if (!profile) { profile = browser()->profile(); } @@ -322,8 +323,7 @@ size_t EphemeralStorageBrowserTest::WaitForCleanupAfterKeepAlive(Profile* profil // failures when the timing is "just right". Do a no-op removal here to make // sure the queued Ephemeral Storage cleanup was complete. BrowsingDataRemoverObserver data_remover_observer; - content::BrowsingDataRemover* remover = - profile->GetBrowsingDataRemover(); + content::BrowsingDataRemover* remover = profile->GetBrowsingDataRemover(); remover->AddObserver(&data_remover_observer); remover->RemoveAndReply(base::Time(), base::Time::Max(), 0, 0, &data_remover_observer); diff --git a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc index 2157b8c345d3..3138ca21e6a4 100644 --- a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc @@ -4,7 +4,6 @@ * You can obtain one at https://mozilla.org/MPL/2.0/. */ #include "brave/browser/ephemeral_storage/ephemeral_storage_browsertest.h" - #include "brave/components/brave_shields/content/browser/brave_shields_util.h" #include "chrome/browser/content_settings/cookie_settings_factory.h" #include "chrome/browser/content_settings/host_content_settings_map_factory.h" @@ -609,7 +608,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageForgetByDefaultIncognitoBrowserTest, EXPECT_EQ(0u, GetAllCookies().size()); EXPECT_TRUE(LoadURLInNewTab(a_site_set_cookie_url)); - + // Cookies SHOULD exist for a.com. EXPECT_EQ(1u, GetAllCookies().size()); } diff --git a/components/ephemeral_storage/ephemeral_storage_service.cc b/components/ephemeral_storage/ephemeral_storage_service.cc index 089658e89cde..ed959b8b49b1 100644 --- a/components/ephemeral_storage/ephemeral_storage_service.cc +++ b/components/ephemeral_storage/ephemeral_storage_service.cc @@ -15,10 +15,6 @@ #include "base/timer/timer.h" #include "brave/components/ephemeral_storage/ephemeral_storage_pref_names.h" #include "brave/components/ephemeral_storage/url_storage_checker.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/ui/browser.h" -#include "chrome/browser/ui/browser_finder.h" -#include "chrome/browser/ui/browser_window.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/prefs/scoped_user_pref_update.h" #include "components/user_prefs/user_prefs.h" @@ -39,14 +35,6 @@ GURL GetFirstPartyStorageURL(const std::string& ephemeral_domain) { return GURL(base::StrCat({url::kHttpsScheme, "://", ephemeral_domain})); } -bool DoesProfileHaveAnyBrowserWindow(Profile* profile) { - for (Browser* browser : chrome::FindAllBrowsersWithProfile(profile)) { - if (browser->window()) - return true; - } - return false; -} - } // namespace EphemeralStorageService::EphemeralStorageService( @@ -340,7 +328,10 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { DCHECK(!context_->IsOffTheRecord()); Profile* profile = Profile::FromBrowserContext(context_); - if (!DoesProfileHaveAnyBrowserWindow(profile)) { + + bool have_window{false}; + delegate_->DoesProfileHaveAnyBrowserWindow(profile, &have_window); + if (!have_window) { first_party_storage_areas_to_cleanup_on_startup_.clear(); return; } diff --git a/components/ephemeral_storage/ephemeral_storage_service_delegate.h b/components/ephemeral_storage/ephemeral_storage_service_delegate.h index 399ae1291745..964023edb80f 100644 --- a/components/ephemeral_storage/ephemeral_storage_service_delegate.h +++ b/components/ephemeral_storage/ephemeral_storage_service_delegate.h @@ -12,6 +12,7 @@ #include "brave/components/ephemeral_storage/ephemeral_storage_types.h" #include "url/origin.h" +class Profile; namespace ephemeral_storage { // Delegate performs cleanup for all required parts (chrome, content, etc.). @@ -24,6 +25,8 @@ class EphemeralStorageServiceDelegate { // Cleanups non-ephemeral first party storage areas (cache, dom storage). virtual void CleanupFirstPartyStorageArea( const std::string& registerable_domain) {} + virtual void DoesProfileHaveAnyBrowserWindow(Profile* profile, + bool* have_window) {} }; } // namespace ephemeral_storage From 80768b970f400fe10e591ca85b028bcf09b101a2 Mon Sep 17 00:00:00 2001 From: tech-zilla Date: Sat, 27 Jul 2024 16:17:49 +0300 Subject: [PATCH 5/7] Use std::optional instead of raw pointer --- .../brave_ephemeral_storage_service_delegate.cc | 10 ++++++---- .../brave_ephemeral_storage_service_delegate.h | 9 ++------- .../ephemeral_storage/ephemeral_storage_service.cc | 7 +++---- .../ephemeral_storage_service_delegate.h | 6 +++--- 4 files changed, 14 insertions(+), 18 deletions(-) diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc index b0d142ca39fb..22d2a852a60c 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc @@ -10,7 +10,9 @@ #include "brave/components/brave_shields/content/browser/brave_shields_util.h" #include "chrome/browser/browsing_data/chrome_browsing_data_remover_constants.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" +#include "chrome/browser/ui/browser_window.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browsing_data_filter_builder.h" @@ -114,15 +116,15 @@ void BraveEphemeralStorageServiceDelegate::CleanupFirstPartyStorageArea( } void BraveEphemeralStorageServiceDelegate::DoesProfileHaveAnyBrowserWindow( - Profile* profile, - bool* have_window) { + std::optional& is_window_visible) { + Profile* profile = Profile::FromBrowserContext(context_); for (Browser* browser : chrome::FindAllBrowsersWithProfile(profile)) { if (browser->window()) { - *have_window = true; + is_window_visible = true; return; } } - *have_window = false; + is_window_visible = false; } } // namespace ephemeral_storage diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h index 95a285b84e37..7f54445eef2b 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h @@ -13,11 +13,6 @@ #include "brave/components/ephemeral_storage/ephemeral_storage_service_delegate.h" #include "components/content_settings/core/browser/cookie_settings.h" -class Profile; -namespace content { -class BrowserContext; -} - class HostContentSettingsMap; namespace ephemeral_storage { @@ -34,8 +29,8 @@ class BraveEphemeralStorageServiceDelegate void CleanupTLDEphemeralArea(const TLDEphemeralAreaKey& key) override; void CleanupFirstPartyStorageArea( const std::string& registerable_domain) override; - void DoesProfileHaveAnyBrowserWindow(Profile* profile, - bool* have_window) override; + void DoesProfileHaveAnyBrowserWindow( + std::optional& is_window_visible) override; private: raw_ptr context_ = nullptr; diff --git a/components/ephemeral_storage/ephemeral_storage_service.cc b/components/ephemeral_storage/ephemeral_storage_service.cc index ed959b8b49b1..ebe9b2fe0571 100644 --- a/components/ephemeral_storage/ephemeral_storage_service.cc +++ b/components/ephemeral_storage/ephemeral_storage_service.cc @@ -327,11 +327,10 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { DCHECK(!context_->IsOffTheRecord()); - Profile* profile = Profile::FromBrowserContext(context_); - bool have_window{false}; - delegate_->DoesProfileHaveAnyBrowserWindow(profile, &have_window); - if (!have_window) { + std::optional is_window_visible = std::nullopt; + delegate_->DoesProfileHaveAnyBrowserWindow(is_window_visible); + if (is_window_visible.has_value() && !is_window_visible.value()) { first_party_storage_areas_to_cleanup_on_startup_.clear(); return; } diff --git a/components/ephemeral_storage/ephemeral_storage_service_delegate.h b/components/ephemeral_storage/ephemeral_storage_service_delegate.h index 964023edb80f..2646a91fbc13 100644 --- a/components/ephemeral_storage/ephemeral_storage_service_delegate.h +++ b/components/ephemeral_storage/ephemeral_storage_service_delegate.h @@ -6,13 +6,13 @@ #ifndef BRAVE_COMPONENTS_EPHEMERAL_STORAGE_EPHEMERAL_STORAGE_SERVICE_DELEGATE_H_ #define BRAVE_COMPONENTS_EPHEMERAL_STORAGE_EPHEMERAL_STORAGE_SERVICE_DELEGATE_H_ +#include #include #include #include "brave/components/ephemeral_storage/ephemeral_storage_types.h" #include "url/origin.h" -class Profile; namespace ephemeral_storage { // Delegate performs cleanup for all required parts (chrome, content, etc.). @@ -25,8 +25,8 @@ class EphemeralStorageServiceDelegate { // Cleanups non-ephemeral first party storage areas (cache, dom storage). virtual void CleanupFirstPartyStorageArea( const std::string& registerable_domain) {} - virtual void DoesProfileHaveAnyBrowserWindow(Profile* profile, - bool* have_window) {} + virtual void DoesProfileHaveAnyBrowserWindow( + std::optional& is_window_visible) {} }; } // namespace ephemeral_storage From ecff2ff7eca77c055023603b0a54dcdd1ff84ec8 Mon Sep 17 00:00:00 2001 From: tech-zilla Date: Sat, 27 Jul 2024 16:52:30 +0300 Subject: [PATCH 6/7] We still need the browser context forward declaration --- .../brave_ephemeral_storage_service_delegate.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h index 7f54445eef2b..4e5a39364deb 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h @@ -13,6 +13,10 @@ #include "brave/components/ephemeral_storage/ephemeral_storage_service_delegate.h" #include "components/content_settings/core/browser/cookie_settings.h" +namespace content { +class BrowserContext; +} + class HostContentSettingsMap; namespace ephemeral_storage { From 92ccc3f7e5ade6828127888af40f81a19b271a66 Mon Sep 17 00:00:00 2001 From: tech-zilla Date: Mon, 29 Jul 2024 15:13:35 +0300 Subject: [PATCH 7/7] Review changes --- .../brave_ephemeral_storage_service_delegate.cc | 9 ++++----- .../brave_ephemeral_storage_service_delegate.h | 3 +-- ...emeral_storage_forget_by_default_browsertest.cc | 4 ++-- components/ephemeral_storage/BUILD.gn | 1 + .../ephemeral_storage/ephemeral_storage_service.cc | 4 +--- .../ephemeral_storage_service_delegate.cc | 14 ++++++++++++++ .../ephemeral_storage_service_delegate.h | 4 +--- 7 files changed, 24 insertions(+), 15 deletions(-) create mode 100644 components/ephemeral_storage/ephemeral_storage_service_delegate.cc diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc index 22d2a852a60c..d8256a791d1c 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.cc @@ -115,16 +115,15 @@ void BraveEphemeralStorageServiceDelegate::CleanupFirstPartyStorageArea( origin_type, std::move(filter_builder)); } -void BraveEphemeralStorageServiceDelegate::DoesProfileHaveAnyBrowserWindow( - std::optional& is_window_visible) { +bool BraveEphemeralStorageServiceDelegate::DoesProfileHaveAnyBrowserWindow() + const { Profile* profile = Profile::FromBrowserContext(context_); for (Browser* browser : chrome::FindAllBrowsersWithProfile(profile)) { if (browser->window()) { - is_window_visible = true; - return; + return true; } } - is_window_visible = false; + return false; } } // namespace ephemeral_storage diff --git a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h index 4e5a39364deb..ae30d3501c88 100644 --- a/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h +++ b/browser/ephemeral_storage/brave_ephemeral_storage_service_delegate.h @@ -33,8 +33,7 @@ class BraveEphemeralStorageServiceDelegate void CleanupTLDEphemeralArea(const TLDEphemeralAreaKey& key) override; void CleanupFirstPartyStorageArea( const std::string& registerable_domain) override; - void DoesProfileHaveAnyBrowserWindow( - std::optional& is_window_visible) override; + bool DoesProfileHaveAnyBrowserWindow() const override; private: raw_ptr context_ = nullptr; diff --git a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc index 3138ca21e6a4..0365d5b35e78 100644 --- a/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc +++ b/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc @@ -581,12 +581,12 @@ class EphemeralStorageForgetByDefaultIncognitoBrowserTest void SetUpCommandLine(base::CommandLine* command_line) override { EphemeralStorageForgetByDefaultBrowserTest::SetUpCommandLine(command_line); - if (IsPreTest()) { + if (IsPreTestToEnableIncognito()) { command_line->AppendSwitch(switches::kIncognito); } } - static bool IsPreTest() { + static bool IsPreTestToEnableIncognito() { const testing::TestInfo* const test_info = testing::UnitTest::GetInstance()->current_test_info(); return base::StartsWith(test_info->name(), "PRE_DontForgetFirstParty"); diff --git a/components/ephemeral_storage/BUILD.gn b/components/ephemeral_storage/BUILD.gn index 1749037d4fa0..ddd4e06e0aa0 100644 --- a/components/ephemeral_storage/BUILD.gn +++ b/components/ephemeral_storage/BUILD.gn @@ -8,6 +8,7 @@ static_library("ephemeral_storage") { "ephemeral_storage_pref_names.h", "ephemeral_storage_service.cc", "ephemeral_storage_service.h", + "ephemeral_storage_service_delegate.cc", "ephemeral_storage_service_delegate.h", "ephemeral_storage_service_observer.h", "ephemeral_storage_types.h", diff --git a/components/ephemeral_storage/ephemeral_storage_service.cc b/components/ephemeral_storage/ephemeral_storage_service.cc index ebe9b2fe0571..08adc7fbc40b 100644 --- a/components/ephemeral_storage/ephemeral_storage_service.cc +++ b/components/ephemeral_storage/ephemeral_storage_service.cc @@ -328,9 +328,7 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { DCHECK(!context_->IsOffTheRecord()); - std::optional is_window_visible = std::nullopt; - delegate_->DoesProfileHaveAnyBrowserWindow(is_window_visible); - if (is_window_visible.has_value() && !is_window_visible.value()) { + if (!delegate_->DoesProfileHaveAnyBrowserWindow()) { first_party_storage_areas_to_cleanup_on_startup_.clear(); return; } diff --git a/components/ephemeral_storage/ephemeral_storage_service_delegate.cc b/components/ephemeral_storage/ephemeral_storage_service_delegate.cc new file mode 100644 index 000000000000..f3f13f71993a --- /dev/null +++ b/components/ephemeral_storage/ephemeral_storage_service_delegate.cc @@ -0,0 +1,14 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/ephemeral_storage/ephemeral_storage_service_delegate.h" + +namespace ephemeral_storage { + +bool EphemeralStorageServiceDelegate::DoesProfileHaveAnyBrowserWindow() const { + return false; +} + +} // namespace ephemeral_storage diff --git a/components/ephemeral_storage/ephemeral_storage_service_delegate.h b/components/ephemeral_storage/ephemeral_storage_service_delegate.h index 2646a91fbc13..c2f4323a24be 100644 --- a/components/ephemeral_storage/ephemeral_storage_service_delegate.h +++ b/components/ephemeral_storage/ephemeral_storage_service_delegate.h @@ -6,7 +6,6 @@ #ifndef BRAVE_COMPONENTS_EPHEMERAL_STORAGE_EPHEMERAL_STORAGE_SERVICE_DELEGATE_H_ #define BRAVE_COMPONENTS_EPHEMERAL_STORAGE_EPHEMERAL_STORAGE_SERVICE_DELEGATE_H_ -#include #include #include @@ -25,8 +24,7 @@ class EphemeralStorageServiceDelegate { // Cleanups non-ephemeral first party storage areas (cache, dom storage). virtual void CleanupFirstPartyStorageArea( const std::string& registerable_domain) {} - virtual void DoesProfileHaveAnyBrowserWindow( - std::optional& is_window_visible) {} + virtual bool DoesProfileHaveAnyBrowserWindow() const; }; } // namespace ephemeral_storage