Skip to content

Commit

Permalink
Split profile deletion code out of ProfileManager
Browse files Browse the repository at this point in the history
This CL splits some logic related to profile deletion to separate
files.
This hopefully makes the code more readable, as the ProfileManager
was very large and poorly organized.

It should be a pure refactoring with no behavior change.
This is mostly a code move, although some other minor changes were
required, like moving a few functions from the unnamed namespace to
the `ProfileManager` API.

It also removes `ScheduleProfileForDeletion()` from the public API,
as it was only called by tests. The tests were updated to call
`MaybeScheduleProfileForDeletion()` instead.

In a follow-up change, the KeepAlive management in
`DeleteProfileHelper` will be improved to fix a bug where Lacros
exits immediately at startup while trying to delete a profile.

Bug: 1382815
Change-Id: I49b4ecee7a83dcce3a0d6826982780ba8dbd0be3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4055710
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1076189}
  • Loading branch information
David Roger authored and Chromium LUCI CQ committed Nov 28, 2022
1 parent 66a22da commit 2373065
Show file tree
Hide file tree
Showing 25 changed files with 876 additions and 649 deletions.
4 changes: 4 additions & 0 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -4072,10 +4072,14 @@ static_library("browser") {
"profile_resetter/triggered_profile_resetter.h",
"profile_resetter/triggered_profile_resetter_factory.cc",
"profile_resetter/triggered_profile_resetter_factory.h",
"profiles/delete_profile_helper.cc",
"profiles/delete_profile_helper.h",
"profiles/guest_mode_policy_handler.cc",
"profiles/guest_mode_policy_handler.h",
"profiles/guest_profile_creation_logger.cc",
"profiles/guest_profile_creation_logger.h",
"profiles/nuke_profile_directory_utils.cc",
"profiles/nuke_profile_directory_utils.h",
"profiles/profile_shortcut_manager.cc",
"profiles/profile_shortcut_manager.h",
"profiles/profile_theme_update_service.cc",
Expand Down
7 changes: 5 additions & 2 deletions chrome/browser/app_controller_mac_browsertest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,13 @@
#include "chrome/browser/lifetime/application_lifetime.h"
#include "chrome/browser/lifetime/application_lifetime_desktop.h"
#include "chrome/browser/prefs/incognito_mode_prefs.h"
#include "chrome/browser/profiles/delete_profile_helper.h"
#include "chrome/browser/profiles/keep_alive/profile_keep_alive_types.h"
#include "chrome/browser/profiles/profile_attributes_entry.h"
#include "chrome/browser/profiles/profile_attributes_init_params.h"
#include "chrome/browser/profiles/profile_attributes_storage.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/profiles/profile_metrics.h"
#include "chrome/browser/profiles/profile_observer.h"
#include "chrome/browser/profiles/profile_test_util.h"
#include "chrome/browser/signin/signin_util.h"
Expand Down Expand Up @@ -829,8 +831,9 @@ void SetUpCommandLine(base::CommandLine* command_line) override {
ServiceAccessType::EXPLICIT_ACCESS));

// Delete profile2.
profile_manager->ScheduleProfileForDeletion(profile2->GetPath(),
base::DoNothing());
profile_manager->GetDeleteProfileHelper().MaybeScheduleProfileForDeletion(
profile2->GetPath(), base::DoNothing(),
ProfileMetrics::DELETE_PROFILE_USER_MANAGER);
content::RunAllTasksUntilIdle();

// Verify the controller's history is back to profile1.
Expand Down
9 changes: 7 additions & 2 deletions chrome/browser/app_controller_mac_unittest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
#include "chrome/app/chrome_command_ids.h"
#import "chrome/browser/app_controller_mac.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/delete_profile_helper.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/profiles/profile_metrics.h"
#include "chrome/common/chrome_constants.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/testing_browser_process.h"
Expand Down Expand Up @@ -199,8 +201,11 @@ void TearDown() override {
base::scoped_nsobject<AppController> ac([[AppController alloc] init]);

// Delete the active profile.
profile_manager_.profile_manager()->ScheduleProfileForDeletion(
dest_path1, base::DoNothing());
profile_manager_.profile_manager()
->GetDeleteProfileHelper()
.MaybeScheduleProfileForDeletion(
dest_path1, base::DoNothing(),
ProfileMetrics::DELETE_PROFILE_USER_MANAGER);

base::RunLoop().RunUntilIdle();

Expand Down
9 changes: 7 additions & 2 deletions chrome/browser/chrome_browser_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@
#include "chrome/browser/ui/page_info/chrome_page_info_client.h"
#include "ui/base/resource/resource_bundle_android.h"
#else
#include "chrome/browser/profiles/delete_profile_helper.h"
#include "chrome/browser/resource_coordinator/tab_activity_watcher.h"
#include "chrome/browser/resource_coordinator/tab_manager.h"
#include "chrome/browser/resources_integrity.h"
Expand Down Expand Up @@ -1156,9 +1157,13 @@ void ChromeBrowserMainParts::PreProfileInit() {

#if !BUILDFLAG(IS_ANDROID)
// Ephemeral profiles may have been left behind if the browser crashed.
g_browser_process->profile_manager()->CleanUpEphemeralProfiles();
g_browser_process->profile_manager()
->GetDeleteProfileHelper()
.CleanUpEphemeralProfiles();
// Files of deleted profiles can also be left behind after a crash.
g_browser_process->profile_manager()->CleanUpDeletedProfiles();
g_browser_process->profile_manager()
->GetDeleteProfileHelper()
.CleanUpDeletedProfiles();
#endif // !BUILDFLAG(IS_ANDROID)

#if BUILDFLAG(ENABLE_EXTENSIONS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "base/ranges/algorithm.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/lacros/account_manager/add_account_helper.h"
#include "chrome/browser/profiles/delete_profile_helper.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_attributes_entry.h"
#include "chrome/browser/profiles/profile_attributes_storage.h"
Expand Down Expand Up @@ -510,9 +511,11 @@ AccountProfileMapper::RemoveStaleAccounts() {
// profile.
// TODO(https://crbug.com/1257610): ensure that the user cannot cancel the
// profile deletion.
g_browser_process->profile_manager()->MaybeScheduleProfileForDeletion(
entry->GetPath(), base::DoNothing(),
ProfileMetrics::DELETE_PROFILE_PRIMARY_ACCOUNT_REMOVED_LACROS);
g_browser_process->profile_manager()
->GetDeleteProfileHelper()
.MaybeScheduleProfileForDeletion(
entry->GetPath(), base::DoNothing(),
ProfileMetrics::DELETE_PROFILE_PRIMARY_ACCOUNT_REMOVED_LACROS);
}
}
return removed_ids;
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/lifetime/browser_shutdown.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
#include "chrome/browser/buildflags.h"
#include "chrome/browser/lifetime/application_lifetime_chromeos.h"
#include "chrome/browser/lifetime/switch_utils.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/profiles/nuke_profile_directory_utils.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/pref_names.h"
Expand Down Expand Up @@ -289,7 +289,7 @@ void ShutdownPostThreadsStop(RestartMode restart_mode) {

// crbug.com/95079 - This needs to happen after the browser process object
// goes away.
ProfileManager::NukeDeletedProfilesFromDisk();
NukeDeletedProfilesFromDisk();

#if BUILDFLAG(IS_CHROMEOS_ASH)
ash::BootTimesRecorder::Get()->AddLogoutTimeMarker("BrowserDeleted", true);
Expand Down

0 comments on commit 2373065

Please sign in to comment.