Skip to content

GitLab

T
tangled
Commit 7cb1f9cb authored by Evan Stade's avatar Evan Stade Committed by Commit Bot
Browse files
Options

Remove NOTIFICATION_PROFILE_DESTROYED from IndependentOTRProfileManager 

Bug: 268984
Change-Id: Id59283a4bde7a87a88345672b10d9db9bf9b9c17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869829Reviewed-by: default avatarBrandon Tolsch <btolsch@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708037}
parent 704e8e0c
Hide whitespace changes
Inline Side-by-side
Showing with 59 additions and 108 deletions
chrome/browser/media/router/presentation/independent_otr_profile_manager.cc
View file @ 7cb1f9cb
......@@ -43,17 +43,15 @@ IndependentOTRProfileManager* IndependentOTRProfileManager::GetInstance() {
std::unique_ptr<IndependentOTRProfileManager::OTRProfileRegistration>
IndependentOTRProfileManager::CreateFromOriginalProfile(
Profile* profile,
Profile* original_profile,
ProfileDestroyedCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(profile);
DCHECK(!profile->IsOffTheRecord());
DCHECK(original_profile);
DCHECK(!original_profile->IsOffTheRecord());
DCHECK(!callback.is_null());
if (!HasDependentProfiles(profile)) {
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
content::Source<Profile>(profile));
}
auto* otr_profile = profile->CreateOffTheRecordProfile();
if (!HasDependentProfiles(original_profile))
observed_original_profiles_.Add(original_profile);
auto* otr_profile = original_profile->CreateOffTheRecordProfile();
auto entry = refcounts_map_.emplace(otr_profile, 1);
auto callback_entry =
callbacks_map_.emplace(otr_profile, std::move(callback));
......@@ -97,10 +95,8 @@ void IndependentOTRProfileManager::OnBrowserRemoved(Browser* browser) {
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, entry->first);
auto* original_profile = entry->first->GetOriginalProfile();
refcounts_map_.erase(entry);
if (!HasDependentProfiles(original_profile)) {
registrar_.Remove(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
content::Source<Profile>(original_profile));
}
if (!HasDependentProfiles(original_profile))
observed_original_profiles_.Remove(original_profile);
}
}
......@@ -138,28 +134,22 @@ void IndependentOTRProfileManager::UnregisterProfile(Profile* profile) {
auto* original_profile = profile->GetOriginalProfile();
ProfileDestroyer::DestroyProfileWhenAppropriate(entry->first);
refcounts_map_.erase(entry);
if (!HasDependentProfiles(original_profile)) {
registrar_.Remove(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
content::Source<Profile>(original_profile));
}
if (!HasDependentProfiles(original_profile))
observed_original_profiles_.Remove(original_profile);
}
}
void IndependentOTRProfileManager::Observe(
int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) {
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
void IndependentOTRProfileManager::OnProfileWillBeDestroyed(Profile* profile) {
for (auto it = callbacks_map_.begin(); it != callbacks_map_.end();) {
if (source != content::Source<Profile>(it->first->GetOriginalProfile())) {
if (profile != it->first->GetOriginalProfile()) {
++it;
continue;
}
// If the registration is destroyed from within this callback, we don't want
// to double-erase. If it isn't, we still need to erase the callback entry.
auto* profile = it->first;
auto* otr_profile = it->first;
auto callback = std::move(it->second);
it = callbacks_map_.erase(it);
std::move(callback).Run(profile);
std::move(callback).Run(otr_profile);
}
}
chrome/browser/media/router/presentation/independent_otr_profile_manager.h
View file @ 7cb1f9cb
......@@ -12,9 +12,10 @@
#include "base/containers/flat_map.h"
#include "base/macros.h"
#include "base/memory/singleton.h"
#include "base/scoped_observer.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_observer.h"
#include "chrome/browser/ui/browser_list_observer.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
class Profile;
......@@ -42,9 +43,8 @@ class Profile;
// original profile is being destroyed.
//
// All methods must be called on the UI thread.
class IndependentOTRProfileManager final
: public BrowserListObserver,
public content::NotificationObserver {
class IndependentOTRProfileManager final : public BrowserListObserver,
public ProfileObserver {
public:
class OTRProfileRegistration {
public:
......@@ -84,21 +84,19 @@ class IndependentOTRProfileManager final
bool HasDependentProfiles(Profile* profile) const;
void UnregisterProfile(Profile* profile);
// chrome::BrowserListObserver overrides.
// chrome::BrowserListObserver:
void OnBrowserAdded(Browser* browser) final;
void OnBrowserRemoved(Browser* browser) final;
// content::NotificationObserver overrides.
void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) final;
// ProfileObserver:
void OnProfileWillBeDestroyed(Profile* profile) final;
// Counts the number of Browser instances referencing an independent OTR
// profile plus 1 for the OTRProfileRegistration object that created it.
base::flat_map<Profile*, int32_t> refcounts_map_;
base::flat_map<Profile*, ProfileDestroyedCallback> callbacks_map_;
content::NotificationRegistrar registrar_;
ScopedObserver<Profile, ProfileObserver> observed_original_profiles_{this};
DISALLOW_COPY_AND_ASSIGN(IndependentOTRProfileManager);
};
......
chrome/browser/media/router/presentation/independent_otr_profile_manager_browsertest.cc
View file @ 7cb1f9cb
......@@ -7,6 +7,7 @@
#include "base/bind.h"
#include "base/files/scoped_temp_dir.h"
#include "base/run_loop.h"
#include "base/scoped_observer.h"
#include "base/stl_util.h"
#include "base/threading/thread_restrictions.h"
#include "build/build_config.h"
......@@ -14,6 +15,7 @@
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/media/router/presentation/independent_otr_profile_manager.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_observer.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/browser_list_observer.h"
......@@ -21,53 +23,33 @@
#include "chrome/browser/ui/browser_navigator_params.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/notification_source.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/test_utils.h"
#include "ui/base/page_transition_types.h"
#include "ui/base/window_open_disposition.h"
namespace content {
class NotificationDetails;
} // namespace content
namespace {
class ProfileDestructionWatcher final : public content::NotificationObserver {
class ProfileDestructionWatcher : public ProfileObserver {
public:
using DestructionCallback = base::OnceClosure;
ProfileDestructionWatcher() = default;
~ProfileDestructionWatcher() override = default;
// Watches for the destruction of |profile| and sets |destroyed| to true when
// that happens. |profile| can be any Profile object, but most tests below
// use it to watch for the cleanup of an OTR profile.
void Watch(Profile* profile, bool* destroyed) {
ASSERT_FALSE(*destroyed);
profile_ = profile;
destroyed_ = destroyed;
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
content::Source<Profile>(profile_));
}
void Watch(Profile* profile) { observed_profiles_.Add(profile); }
// content::NotificationObserver overrides.
void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) override {
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
DCHECK(content::Source<Profile>(profile_) == source);
*destroyed_ = true;
registrar_.Remove(this, chrome::NOTIFICATION_PROFILE_DESTROYED, source);
// ProfileObserver:
void OnProfileWillBeDestroyed(Profile* profile) override {
destroyed_ = true;
observed_profiles_.Remove(profile);
}
bool destroyed() const { return destroyed_; }
private:
Profile* profile_;
bool* destroyed_;
content::NotificationRegistrar registrar_;
bool destroyed_ = false;
ScopedObserver<Profile, ProfileObserver> observed_profiles_{this};
DISALLOW_COPY_AND_ASSIGN(ProfileDestructionWatcher);
};
// Waits for |browser| to be removed from BrowserList and then calls |callback|.
......@@ -140,7 +122,6 @@ class IndependentOTRProfileManagerTest : public InProcessBrowserTest {
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest, CreateAndDestroy) {
ProfileDestructionWatcher watcher;
bool destroyed = false;
{
auto profile_registration = manager_->CreateFromOriginalProfile(
browser()->profile(), base::BindOnce(&OriginalProfileNeverDestroyed));
......@@ -150,11 +131,11 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest, CreateAndDestroy) {
EXPECT_NE(browser()->profile()->GetOffTheRecordProfile(), otr_profile);
EXPECT_TRUE(otr_profile->IsOffTheRecord());
watcher.Watch(otr_profile, &destroyed);
watcher.Watch(otr_profile);
}
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(destroyed);
EXPECT_TRUE(watcher.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
......@@ -181,21 +162,19 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
ASSERT_FALSE(base::Contains(*BrowserList::GetInstance(), otr_browser1));
ASSERT_TRUE(base::Contains(*BrowserList::GetInstance(), otr_browser2));
bool destroyed = false;
watcher.Watch(otr_profile, &destroyed);
watcher.Watch(otr_profile);
base::RunLoop run_loop2;
BrowserRemovedWaiter removed_waiter2(otr_browser2,
run_loop2.QuitWhenIdleClosure());
otr_browser2->window()->Close();
run_loop2.Run();
ASSERT_FALSE(base::Contains(*BrowserList::GetInstance(), otr_browser2));
EXPECT_TRUE(destroyed);
EXPECT_TRUE(watcher.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
DeleteImmediatelyWhenBrowsersAlreadyClosed) {
ProfileDestructionWatcher watcher;
bool destroyed = false;
{
auto profile_registration = manager_->CreateFromOriginalProfile(
browser()->profile(), base::BindOnce(&OriginalProfileNeverDestroyed));
......@@ -218,19 +197,17 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
ASSERT_FALSE(base::Contains(*BrowserList::GetInstance(), otr_browser1));
ASSERT_FALSE(base::Contains(*BrowserList::GetInstance(), otr_browser2));
watcher.Watch(otr_profile, &destroyed);
watcher.Watch(otr_profile);
}
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(destroyed);
EXPECT_TRUE(watcher.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
CreateTwoFromSameProfile) {
ProfileDestructionWatcher watcher1;
ProfileDestructionWatcher watcher2;
bool destroyed1 = false;
bool destroyed2 = false;
{
auto profile_registration1 = manager_->CreateFromOriginalProfile(
browser()->profile(), base::BindOnce(&OriginalProfileNeverDestroyed));
......@@ -242,13 +219,13 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
ASSERT_NE(otr_profile1, otr_profile2);
watcher1.Watch(otr_profile1, &destroyed1);
watcher2.Watch(otr_profile2, &destroyed2);
watcher1.Watch(otr_profile1);
watcher2.Watch(otr_profile2);
}
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(destroyed1);
EXPECT_TRUE(destroyed2);
EXPECT_TRUE(watcher1.destroyed());
EXPECT_TRUE(watcher2.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
......@@ -270,8 +247,7 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
ASSERT_NE(original_profile.get(), otr_profile);
EXPECT_NE(original_profile->GetOffTheRecordProfile(), otr_profile);
bool destroyed = false;
watcher.Watch(otr_profile, &destroyed);
watcher.Watch(otr_profile);
// Run tasks to ensure that Mojo connections are created before the profile is
// destroyed.
base::RunLoop().RunUntilIdle();
......@@ -279,7 +255,7 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
// |original_profile| being destroyed should trigger the dependent OTR
// profile to be destroyed.
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(destroyed);
EXPECT_TRUE(watcher.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
......@@ -302,10 +278,8 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
auto profile_owner2 = RegistrationOwner(manager_, original_profile.get());
auto* otr_profile2 = profile_owner2.profile();
bool destroyed1 = false;
bool destroyed2 = false;
watcher1.Watch(otr_profile1, &destroyed1);
watcher2.Watch(otr_profile2, &destroyed2);
watcher1.Watch(otr_profile1);
watcher2.Watch(otr_profile2);
// Run tasks to ensure that Mojo connections are created before the profile is
// destroyed.
base::RunLoop().RunUntilIdle();
......@@ -313,16 +287,14 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
// |original_profile| being destroyed should trigger the dependent OTR
// profiles to be destroyed.
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(destroyed1);
EXPECT_TRUE(destroyed2);
EXPECT_TRUE(watcher1.destroyed());
EXPECT_TRUE(watcher2.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
BrowserClosingDoesntRemoveProfileObserver) {
ProfileDestructionWatcher watcher1;
ProfileDestructionWatcher watcher2;
bool destroyed1 = false;
bool destroyed2 = false;
base::ScopedAllowBlockingForTesting allow_blocking;
base::ScopedTempDir temp_dir;
......@@ -341,7 +313,7 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
auto* otr_profile2 = profile_owner2.profile();
otr_browser = CreateBrowser(otr_profile2);
watcher2.Watch(otr_profile2, &destroyed2);
watcher2.Watch(otr_profile2);
}
base::RunLoop run_loop;
BrowserRemovedWaiter removed_waiter(otr_browser,
......@@ -349,12 +321,12 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
otr_browser->window()->Close();
run_loop.Run();
ASSERT_FALSE(base::Contains(*BrowserList::GetInstance(), otr_browser));
EXPECT_TRUE(destroyed2);
EXPECT_TRUE(watcher2.destroyed());
watcher1.Watch(otr_profile1, &destroyed1);
watcher1.Watch(otr_profile1);
original_profile.reset();
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(destroyed1);
EXPECT_TRUE(watcher1.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
......@@ -369,15 +341,14 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest,
otr_browser = CreateBrowser(otr_profile);
}
bool destroyed = false;
watcher.Watch(otr_profile, &destroyed);
watcher.Watch(otr_profile);
base::RunLoop run_loop;
BrowserRemovedWaiter removed_waiter(otr_browser,
run_loop.QuitWhenIdleClosure());
otr_browser->window()->Close();
run_loop.Run();
ASSERT_FALSE(base::Contains(*BrowserList::GetInstance(), otr_browser));
EXPECT_TRUE(destroyed);
EXPECT_TRUE(watcher.destroyed());
}
IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest, Notifications) {
......@@ -398,12 +369,4 @@ IN_PROC_BROWSER_TEST_F(IndependentOTRProfileManagerTest, Notifications) {
EXPECT_FALSE(profile->HasOffTheRecordProfile());
EXPECT_TRUE(otr_profile->IsOffTheRecord());
EXPECT_TRUE(otr_profile->IsIndependentOffTheRecordProfile());
// Destroy the OTR profile.
content::WindowedNotificationObserver profile_destroyed_observer(
chrome::NOTIFICATION_PROFILE_DESTROYED,
content::Source<Profile>(otr_profile));
profile_registration.reset();
profile_destroyed_observer.Wait();
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment