Add fallback behavior if the last used profile cannot initialize

chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc

Index: chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc b/chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9d9f7da3d5bbfa67911bb98fef5cc5f0f38efdcc
--- /dev/null
+++ b/chrome/browser/ui/startup/startup_browser_creator_corrupt_profiles_browsertest_win.cc
@@ -0,0 +1,416 @@
+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[Peter Kasting, 2016/07/19 21:56:07]

No (c) (see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
).

[WC Leung, 2016/07/27 18:00:07]

Thanks for noting. Where did I copy the notice???

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+#include <string>
+#include <vector>
+
+#include "base/base_switches.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "base/strings/string_util.h"
+#include "base/test/test_file_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/profiles/profile_window.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/user_manager.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_browser_process.h"
+
+namespace {
+
+const ProfileManager::CreateCallback kOnProfileSwitchDoNothing;

[Peter Kasting, 2016/07/19 21:56:07]

Nit: I'm not sure this buys much over just inlining
"ProfileManager::CreateCallback()" at the one usage below.

If you really think this is a readability win, prefer to declare locally just
above the use, in keeping with the suggestions in
http://google.github.io/styleguide/cppguide.html#Local_Variables .

[WC Leung, 2016/07/27 18:00:07]

I wanted to make it consistent with profile_manager_browsertest.cc. See
https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager_...

BTW, this is only used once in this file, so inlining sounds good.

+
+void UnblockOnProfileCreation(Profile::CreateStatus expected_final_status,
+                              const base::Closure& quit_closure,
+                              Profile* profile,
+                              Profile::CreateStatus status) {
+  // If the status is CREATE_STATUS_CREATED, then the function will be called
+  // again with CREATE_STATUS_INITIALIZED.
+  if (status == Profile::CREATE_STATUS_CREATED)
+    return;
+
+  EXPECT_EQ(expected_final_status, status);
+  quit_closure.Run();
+}
+
+void UnblockOnProfileInitialized(const base::Closure& quit_closure,
+                                 Profile* profile,
+                                 Profile::CreateStatus status) {
+  UnblockOnProfileCreation(Profile::CREATE_STATUS_INITIALIZED, quit_closure,
+                           profile, status);
+}
+
+void CloseBrowsersSuccessCallback(const base::Closure& quit_closure,

[Peter Kasting, 2016/07/19 21:56:08]

Nit: Maybe OnCloseAllBrowsersSucceeded()?

[WC Leung, 2016/07/27 18:00:07]

Sounds good. Done.

+                                  const base::FilePath& path) {
+  quit_closure.Run();
+}
+
+void CreateAndSwitchToProfile(const std::string& basepath) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  ASSERT_TRUE(profile_manager);
+
+  base::FilePath path = profile_manager->user_data_dir().AppendASCII(basepath);
+  base::RunLoop run_loop;
+  profile_manager->CreateProfileAsync(
+      path, base::Bind(&UnblockOnProfileInitialized, run_loop.QuitClosure()),
+      base::string16(), std::string(), std::string());
+  // Run the message loop to allow profile creation to take place; the loop is
+  // terminated by OnUnblockOnProfileCreation when the profile is created.

[Peter Kasting, 2016/07/19 21:56:08]

Nit: Extra On

[WC Leung, 2016/07/27 18:00:06]

Done.

+  run_loop.Run();
+
+  profiles::SwitchToProfile(path, false, kOnProfileSwitchDoNothing,
+                            ProfileMetrics::SWITCH_PROFILE_ICON);
+}
+
+void CheckBrowserWindows(const std::vector<std::string>& expected_basepaths) {
+  BrowserList* browser_list = BrowserList::GetInstance();

[Peter Kasting, 2016/07/19 21:56:08]

Nit: Just inline this below

[WC Leung, 2016/07/27 18:00:06]

Done.

+  std::vector<std::string> actual_basepaths;
+

[Peter Kasting, 2016/07/19 21:56:08]

Nit: Unnecessary blank line

[WC Leung, 2016/07/27 18:00:06]

Done.

+  for (const Browser* browser : *browser_list) {
+    actual_basepaths.push_back(
+        browser->profile()->GetPath().BaseName().AsUTF8Unsafe());
+  }
+
+  if (actual_basepaths.size() != expected_basepaths.size() ||
+      !std::is_permutation(actual_basepaths.cbegin(), actual_basepaths.cend(),
+                           expected_basepaths.cbegin())) {
+    ADD_FAILURE()
+        << "Expected profile paths are different from actual profile paths."
+           "\n  Actual profile paths: "
+        << base::JoinString(actual_basepaths, ", ")
+        << "\n  Expected profile paths: "
+        << base::JoinString(expected_basepaths, ", ");
+  }
+}
+
+// If the user manager is shown when the browser process die, a crash will

[Peter Kasting, 2016/07/19 21:56:08]

Nit: "Waits for the user manager to appear."

Then, in the test body, above the Hide() call: "If the user manager is still
showing when the browser process dies, ____ will crash.  So hide it before the
caller ends the test."  (I'm not sure what is actually crashing here, fill in
the blank.)

[WC Leung, 2016/07/27 18:00:06]

I have no idea what is crashing. But since UserManager is a singleton, object
lifetime will be likely messed up if UserManager::Hide() is not called.

BTW, I've removed the crash statement. Just said "We must hide the user manager
before the test ends."

FYI, see
https://cs.chromium.org/search/?q=UserManager::Hide&sq=package:chromium&type=cs
for the comments (if any) in other files.

Here's the stack trace anyway.
[7452:5488:0726/175008:FATAL:resource_dispatcher_host_impl.cc(1431)] Check
failed: ContainsKey(active_resource_contexts_, resource_context).
Backtrace:
        base::debug::StackTrace::StackTrace [0x68E0EA37+23]
(f:\chromium\src\base\debug\stack_trace_win.cc:215)
        logging::LogMessage::~LogMessage [0x68E303B7+55]
(f:\chromium\src\base\logging.cc:524)
        content::ResourceDispatcherHostImpl::BeginRequest [0x65DC6765+581]
(f:\chromium\src\content\browser\loader\resource_dispatcher_host_impl.cc:1435)
        content::ResourceDispatcherHostImpl::OnRequestResource [0x65DCC3BC+204]
(f:\chromium\src\content\browser\loader\resource_dispatcher_host_impl.cc:1242)
       
IPC::MessageT<ResourceHostMsg_RequestResource_Meta,std::tuple<int,int,content::ResourceRequest>,void>::Dispatch<content::ResourceDispatcherHostImpl,content::ResourceDispatcherHostImpl,void,void
(__thiscall content::ResourceDispatcherHostImpl::*)(int,int,c [0x65DBFA9E+286]
(f:\chromium\src\ipc\ipc_message_templates.h:121)
        content::ResourceDispatcherHostImpl::OnMessageReceived [0x65DCBE8D+125]
(f:\chromium\src\content\browser\loader\resource_dispatcher_host_impl.cc:1184)
        content::ResourceMessageFilter::OnMessageReceived [0x65DD5F13+19]
(f:\chromium\src\content\browser\loader\resource_message_filter.cc:50)
        content::BrowserMessageFilter::Internal::OnMessageReceived
[0x65A9863D+237]
(f:\chromium\src\content\public\browser\browser_message_filter.cc:70)
        IPC::`anonymous namespace'::TryFiltersImpl [0x656B04BB+43]
(f:\chromium\src\ipc\message_filter_router.cc:22)
        IPC::MessageFilterRouter::TryFilters [0x656B0474+68]
(f:\chromium\src\ipc\message_filter_router.cc:87)
        IPC::ChannelProxy::Context::TryFilters [0x6569DDFD+29]
(f:\chromium\src\ipc\ipc_channel_proxy.cc:80)
        IPC::ChannelProxy::Context::OnMessageReceived [0x6569D51E+14]
(f:\chromium\src\ipc\ipc_channel_proxy.cc:97)
        IPC::ChannelMojo::OnMessageReceived [0x62FB99C6+278]
(f:\chromium\src\ipc\mojo\ipc_channel_mojo.cc:405)
        IPC::internal::MessagePipeReader::Receive [0x62FBE3A2+306]
(f:\chromium\src\ipc\mojo\ipc_message_pipe_reader.cc:139)
        IPC::mojom::ChannelStub::Accept [0x62FB538D+397]
(f:\chromium\src\out\release\gen\ipc\mojo\ipc.mojom.cc:607)
        mojo::internal::InterfaceEndpointClient::HandleValidatedMessage
[0x62FC8ECC+476]
(f:\chromium\src\mojo\public\cpp\bindings\lib\interface_endpoint_client.cc:309)
        IPC::mojom::ChannelRequestValidator::Accept [0x62FB51D3+99]
(f:\chromium\src\out\release\gen\ipc\mojo\ipc.mojom.cc:646)
        mojo::internal::InterfaceEndpointClient::HandleIncomingMessage
[0x62FC8CD8+24]
(f:\chromium\src\mojo\public\cpp\bindings\lib\interface_endpoint_client.cc:260)
        mojo::internal::MultiplexRouter::ProcessIncomingMessage [0x62FC3DA5+325]
(f:\chromium\src\mojo\public\cpp\bindings\lib\multiplex_router.cc:776)
        mojo::internal::MultiplexRouter::Accept [0x62FC23CC+76]
(f:\chromium\src\mojo\public\cpp\bindings\lib\multiplex_router.cc:494)
        mojo::internal::MessageHeaderValidator::Accept [0x62FCCF11+81]
(f:\chromium\src\mojo\public\cpp\bindings\lib\message_header_validator.cc:73)
        mojo::internal::Connector::ReadAllAvailableMessages [0x62FCC8CA+154]
(f:\chromium\src\mojo\public\cpp\bindings\lib\connector.cc:297)
        mojo::internal::Connector::OnWatcherHandleReady [0x62FCC6FE+46]
(f:\chromium\src\mojo\public\cpp\bindings\lib\connector.cc:210)
       
base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void
(__thiscall mojo::internal::Connector::*)(unsigned int)>,void
__cdecl(mojo::internal::Connector *,unsigned
int),base::internal::UnretainedWrapper [0x62FCCB93+19]
(f:\chromium\src\base\bind_internal.h:368)
        mojo::Watcher::OnHandleReady [0x62FD1194+100]
(f:\chromium\src\mojo\public\cpp\system\watcher.cc:122)
        mojo::Watcher::CallOnHandleReady [0x62FD0FAE+46]
(f:\chromium\src\mojo\public\cpp\system\watcher.cc:142)
        mojo::edk::`anonymous namespace'::CallWatchCallback [0x64F2D2E7+23]
(f:\chromium\src\mojo\edk\system\core.cc:58)
       
base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
(__cdecl*)(void (__cdecl*)(unsigned int,unsigned
int,MojoHandleSignalsState,unsigned int),unsigned int,unsigned
int,mojo::edk::HandleSignalsSta [0x64F306BE+30]
(f:\chromium\src\base\bind_internal.h:364)
        mojo::edk::Watcher::MaybeInvokeCallback [0x64F4DDB5+53]
(f:\chromium\src\mojo\edk\system\watcher.cc:24)
        mojo::edk::RequestContext::~RequestContext [0x64F485E1+145]
(f:\chromium\src\mojo\edk\system\request_context.cc:50)
        mojo::edk::NodeChannel::OnChannelMessage [0x64F3B655+821]
(f:\chromium\src\mojo\edk\system\node_channel.cc:707)
        mojo::edk::Channel::OnReadComplete [0x64F2A438+360]
(f:\chromium\src\mojo\edk\system\channel.cc:554)
        mojo::edk::`anonymous namespace'::ChannelWin::OnReadDone [0x64F2B400+32]
(f:\chromium\src\mojo\edk\system\channel_win.cc:236)
        mojo::edk::`anonymous namespace'::ChannelWin::OnIOCompleted
[0x64F2B333+115] (f:\chromium\src\mojo\edk\system\channel_win.cc:225)
        base::MessagePumpForIO::WaitForWork [0x68E3FDF9+73]
(f:\chromium\src\base\message_loop\message_pump_win.cc:674)
        base::MessagePumpForIO::DoRunLoop [0x68E3EFAE+174]
(f:\chromium\src\base\message_loop\message_pump_win.cc:636)
        base::MessagePumpWin::Run [0x68E3F9CA+74]
(f:\chromium\src\base\message_loop\message_pump_win.cc:60)
        base::MessageLoop::RunHandler [0x68E3CBC1+17]
(f:\chromium\src\base\message_loop\message_loop.cc:439)
        base::MessageLoop::Run [0x68E3CB8D+29]
(f:\chromium\src\base\message_loop\message_loop.cc:295)
        base::Thread::Run [0x68E957AB+11]
(f:\chromium\src\base\threading\thread.cc:205)
        content::BrowserThreadImpl::IOThreadRun [0x65BAEDF0+32]
(f:\chromium\src\content\browser\browser_thread_impl.cc:216)
        content::BrowserThreadImpl::Run [0x65BAF85A+362]
(f:\chromium\src\content\browser\browser_thread_impl.cc:251)
        base::Thread::ThreadMain [0x68E95D6F+319]
(f:\chromium\src\base\threading\thread.cc:259)
        base::`anonymous namespace'::ThreadFunc [0x68E8CF82+130]
(f:\chromium\src\base\threading\platform_thread_win.cc:86)
        BaseThreadInitThunk [0x750538F4+36]
        RtlUnicodeStringToInteger [0x77895DE3+595]
        RtlUnicodeStringToInteger [0x77895DAE+542]

+// happen. If a user manager is expected to show, it is needed to hide the user
+// manager before ending the test.
+void WaitForAndHideUserManager() {

[Peter Kasting, 2016/07/19 21:56:08]

Nit: Maybe name this ExpectUserManagerShown() since from the caller perspective
that's the critical bit (hiding the user manager afterwards is more like an
implementation detail).

[WC Leung, 2016/07/27 18:00:07]

SGTM. I've renamed to ExpectUserManagerToShow. (It may be already shown, or will
show in the future.)

+  if (!UserManager::IsShowing()) {
+    base::RunLoop run_loop;
+    UserManager::AddOnUserManagerShownCallbackForTesting(
+        run_loop.QuitClosure());
+    run_loop.Run();
+  }
+  ASSERT_TRUE(UserManager::IsShowing());
+
+  UserManager::Hide();
+}
+
+}  // namespace
+
+class StartupBrowserCreatorCorruptProfileTest : public InProcessBrowserTest {
+ public:
+  StartupBrowserCreatorCorruptProfileTest()
+      : test_body_has_run_(false), expect_test_body_to_run_(true) {}
+
+  void SetExpectTestBodyToRun(bool expected_result) {
+    expect_test_body_to_run_ = expected_result;
+  }
+
+  bool DeleteProfileData(const std::string& basepath) {
+    base::FilePath user_data_dir;
+    if (!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
+      return false;
+
+    base::FilePath dir_to_delete = user_data_dir.AppendASCII(basepath);
+    return base::DirectoryExists(dir_to_delete) &&
+           base::DeleteFile(dir_to_delete, true);
+  }
+
+  bool RemoveCreateDirectoryPermissionForUserDataDirectory() {
+    base::FilePath user_data_dir;
+    return PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) &&
+           base::DenyFilePermission(user_data_dir, FILE_ADD_SUBDIRECTORY);
+  }
+
+ protected:
+  // For each test, create a bool SetUpUserDataDirectory[testname] function. Use
+  // these functions to do things related to the user data directory, most
+  // importantly remove directories before the browser process starts.

[Peter Kasting, 2016/07/19 21:56:07]

How come we can't do the steps in these functions inside the PRE_ functions
instead, and collapse each test from three parts to two?

[WC Leung, 2016/07/27 18:00:06]

When the browser process in running we cannot remove directories. That's why we
need to delete the directories in SetUpUserDataDirectory(), which is called
before the browser process starts.

+  bool SetUpUserDataDirectoryForLastOpenedProfileMissing();
+  bool SetUpUserDataDirectoryForLastUsedProfileFallbackToLastOpenedProfiles();
+  bool SetUpUserDataDirectoryForLastUsedProfileFallbackToUserManager();
+  bool SetUpUserDataDirectoryForCannotCreateSystemProfile();
+  bool SetUpUserDataDirectoryForLastUsedProfileFallbackToAnyProfile();
+  bool SetUpUserDataDirectoryForLastUsedProfileFallbackFail();
+  bool SetUpUserDataDirectoryForDoNotStartSignedInProfile();
+  bool SetUpUserDataDirectoryForNoFallbackForUserSelectedProfile();
+  bool SetUpUserDataDirectoryForDeletedProfileFallbackToUserManager();
+
+  void CloseBrowsersSynchronouslyForProfileBasePath(
+           const std::string& basepath) {
+    ProfileManager* profile_manager = g_browser_process->profile_manager();
+    ASSERT_TRUE(profile_manager);
+
+    Profile* profile =
+        profile_manager->GetProfileByPath(
+            profile_manager->user_data_dir().AppendASCII(basepath));
+    ASSERT_TRUE(profile);
+
+    base::RunLoop run_loop;
+    BrowserList::GetInstance()->CloseAllBrowsersWithProfile(
+        profile, base::Bind(&CloseBrowsersSuccessCallback,
+                            run_loop.QuitClosure()));
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kRestoreLastSession);
+    command_line->AppendSwitch(switches::kNoErrorDialogs);
+  }
+
+  // In this test fixture, SetUpUserDataDirectory must be handled for all
+  // non-PRE_ tests.
+  bool SetUpUserDataDirectory() override {
+#define SET_UP_USER_DATA_DIRECTORY_FOR(testname) \
+if (testing::UnitTest::GetInstance()->current_test_info()->name() == \
+    std::string(#testname)) {\
+  return this->SetUpUserDataDirectoryFor ## testname(); \
+}
+
+    SET_UP_USER_DATA_DIRECTORY_FOR(LastOpenedProfileMissing);
+    SET_UP_USER_DATA_DIRECTORY_FOR(LastUsedProfileFallbackToLastOpenedProfiles);
+    SET_UP_USER_DATA_DIRECTORY_FOR(LastUsedProfileFallbackToUserManager);
+    SET_UP_USER_DATA_DIRECTORY_FOR(CannotCreateSystemProfile);
+    SET_UP_USER_DATA_DIRECTORY_FOR(LastUsedProfileFallbackToAnyProfile);
+    SET_UP_USER_DATA_DIRECTORY_FOR(LastUsedProfileFallbackFail);
+    SET_UP_USER_DATA_DIRECTORY_FOR(DoNotStartSignedInProfile);
+    SET_UP_USER_DATA_DIRECTORY_FOR(NoFallbackForUserSelectedProfile);
+    SET_UP_USER_DATA_DIRECTORY_FOR(DeletedProfileFallbackToUserManager);
+
+#undef SET_UP_USER_DATA_DIRECTORY_FOR
+
+    // If control goes here, it means SetUpUserDataDirectory is not handled.
+    // This is okay for PRE_ tests, but not acceptable for main tests.
+    if (base::StartsWith(
+            testing::UnitTest::GetInstance()->current_test_info()->name(),
+             "PRE_", base::CompareCase::SENSITIVE)) {
+      return true;
+    }
+
+    ADD_FAILURE() << "SetUpUserDataDirectory is not handled by the test.";
+    return false;
+  }
+
+  void TearDownOnMainThread() override {
+    test_body_has_run_ = true;
+    InProcessBrowserTest::TearDownOnMainThread();
+  }
+
+  void TearDown() override {
+    EXPECT_EQ(expect_test_body_to_run_, test_body_has_run_);
+    InProcessBrowserTest::TearDown();
+  }
+
+ private:
+  bool test_body_has_run_;
+  bool expect_test_body_to_run_;
+  DISALLOW_COPY_AND_ASSIGN(StartupBrowserCreatorCorruptProfileTest);
+};
+
+// Most of the tests below has three sections:

[Peter Kasting, 2016/07/19 21:56:08]

Nit: has -> have

Somewhere in this comment you should probably also explain why this
functionality can't all be in one function for each test (like is commonly
done).

[WC Leung, 2016/07/27 18:00:06]

Updated the comment so it carries more details. I did not directly tell why it
is not feasible to do it in one function though.

+// (1) PRE_ test, which is used to create profiles;
+// (2) StartupBrowserCreatorCorruptProfileTest::SetUpUserDataDirectoryFor...
+//     which set up the user data directory for the main test;
+// (3) The main test, where the real test is taken.

[Peter Kasting, 2016/07/19 21:56:07]

Nit: Just "The test itself." is probably fine

[WC Leung, 2016/07/27 18:00:06]

Done.

+
+// LastOpenedProfileMissing : If any last opened profile is missing, that
+// profile is skipped, but other last opened profiles should be opened in the
+// browser.
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_LastOpenedProfileMissing) {
+  CreateAndSwitchToProfile("Profile 1");
+  CreateAndSwitchToProfile("Profile 2");
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+         SetUpUserDataDirectoryForLastOpenedProfileMissing() {
+  return DeleteProfileData("Profile 1") &&
+         RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       LastOpenedProfileMissing) {
+  CheckBrowserWindows({"Default", "Profile 2"});
+}
+
+// LastUsedProfileFallbackToLastOpenedProfiles : If the last used profile is
+// missing, it should fallback to any last opened profiles.

[Peter Kasting, 2016/07/19 21:56:08]

Nit: When using "fallback" as a verb, spell it "fall back".  "fallback" is a
noun, or adjective.  (Several places)

[WC Leung, 2016/07/27 18:00:06]

Done.

+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_LastUsedProfileFallbackToLastOpenedProfiles) {
+  CreateAndSwitchToProfile("Profile 1");
+  CreateAndSwitchToProfile("Profile 2");
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForLastUsedProfileFallbackToLastOpenedProfiles() {
+  return DeleteProfileData("Profile 2") &&
+         RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       LastUsedProfileFallbackToLastOpenedProfiles) {
+  CheckBrowserWindows({"Default", "Profile 1"});
+}
+
+// LastUsedProfileFallbackToUserManager : If all last used profile is

[Peter Kasting, 2016/07/19 21:56:07]

Nit: profile is -> profiles are

[WC Leung, 2016/07/27 18:00:07]

Done. Anyway, I should have referred to "last opened profiles" instead of "last
used profile".

+// missing, it should fallback to user manager. To open the user manager, both

[Peter Kasting, 2016/07/19 21:56:07]

Nit: it should fallback to -> fall back to the

[WC Leung, 2016/07/27 18:00:06]

Done.

+// the guesst profile and the system profile must be creatable.

[Peter Kasting, 2016/07/19 21:56:08]

Nit: guest

[WC Leung, 2016/07/27 18:00:06]

Done.

+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_LastUsedProfileFallbackToUserManager) {
+  CreateAndSwitchToProfile("Profile 1");
+  CloseBrowsersSynchronouslyForProfileBasePath("Profile 1");
+  CreateAndSwitchToProfile("Profile 2");
+
+  ASSERT_TRUE(base::CreateDirectory(ProfileManager::GetGuestProfilePath()));
+  ASSERT_TRUE(base::CreateDirectory(ProfileManager::GetSystemProfilePath()));
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForLastUsedProfileFallbackToUserManager() {
+  return DeleteProfileData("Default") &&
+         DeleteProfileData("Profile 2") &&
+         RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       LastUsedProfileFallbackToUserManager) {
+  CheckBrowserWindows({});
+  WaitForAndHideUserManager();
+}
+
+
+// CannotCreateSystemProfile : If the system profile cannot be created, the user
+// manager should not be shown. Fallback to any other profile.
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_CannotCreateSystemProfile) {
+  CreateAndSwitchToProfile("Profile 1");
+  CloseBrowsersSynchronouslyForProfileBasePath("Profile 1");
+  CreateAndSwitchToProfile("Profile 2");
+
+  // Create the guest profile path only. The system profile cannot be created.

[Peter Kasting, 2016/07/19 21:56:08]

Nit: "Create the guest profile path, but not the system profile one.  This will
make it impossible to create the system profile once the permissions are locked
down during setup."

[WC Leung, 2016/07/27 18:00:06]

Done.

+  ASSERT_TRUE(base::CreateDirectory(ProfileManager::GetGuestProfilePath()));
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForCannotCreateSystemProfile() {
+  return DeleteProfileData("Default") &&
+         DeleteProfileData("Profile 2") &&
+         RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       CannotCreateSystemProfile) {
+  CheckBrowserWindows({"Profile 1"});
+}
+
+// LastUsedProfileFallbackToAnyProfile : If all the last opened profiles and the
+// guest profile cannot be opened, fallback to any non-signin profile.

[Peter Kasting, 2016/07/19 21:56:08]

Nit: non-signin -> not-signed-in

[WC Leung, 2016/07/27 18:00:06]

Oh. Per Mike's comment before, I'd change this to "any profile that is not
locked".

+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_LastUsedProfileFallbackToAnyProfile) {
+  CreateAndSwitchToProfile("Profile 1");
+  CloseBrowsersSynchronouslyForProfileBasePath("Profile 1");
+  CreateAndSwitchToProfile("Profile 2");
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForLastUsedProfileFallbackToAnyProfile() {
+  return DeleteProfileData("Default") &&
+         DeleteProfileData("Profile 2") &&
+         RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       LastUsedProfileFallbackToAnyProfile) {
+  CheckBrowserWindows({"Profile 1"});
+}
+
+// LastUsedProfileFallbackFail : If all the options fail, quit the browser with
+// an appropriate exit code (0).

[Peter Kasting, 2016/07/19 21:56:07]

Nit: This doesn't check the browser exit code, so I wouldn't talk about it.

[WC Leung, 2016/07/27 18:00:06]

The browser exit code is checked somewhere else in InProcessBrowserTest. For
InProcessBrowserTest, somehow returning a zero means the test passed even if the
test body is not run.

+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForLastUsedProfileFallbackFail() {
+  SetExpectTestBodyToRun(false);
+  return RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       LastUsedProfileFallbackFail) {
+  ADD_FAILURE() << "Test body is not expected to run.";
+}
+
+// DoNotStartSignedInProfile : Profile that are locked are not expected to be
+// started in any stage of the fallback. Since there is no other profile to
+// fallback to in this test, chromium should not start.

[Peter Kasting, 2016/07/19 21:56:07]

Nit: "Profiles that are locked should never be initialized.  Since there are no
unlocked profiles, the browser should not start."

[WC Leung, 2016/07/27 18:00:07]

Done. Anyway the test is named incorrectly, so I renamed to
DoNotStartLockedProfile.

+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_DoNotStartSignedInProfile) {
+  // Sign out the default profile and wait for the user manager.

[Peter Kasting, 2016/07/19 21:56:07]

Why is the user manager appearing during the PRE_ test?  Isn't this just setup
before the browser starts?

[WC Leung, 2016/07/27 18:00:06]

When a profile is being locked the user manager shows, allowing the user to use
the browser with the same or another profile. This is just some implementation
detail, which has nothing to do with the idea of the test.

Anyway, added a short explanation in the comment.

+  profiles::LockProfile(browser()->profile());
+  WaitForAndHideUserManager();
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForDoNotStartSignedInProfile() {
+  SetExpectTestBodyToRun(false);
+  return RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       DoNotStartSignedInProfile) {
+  ADD_FAILURE() << "Test body is not expected to run.";
+}
+
+// NoFallbackForUserSelectedProfile : No fallback should be attempted if the
+// profile is selected by --profile-directory switch. Chromium should not start

[Peter Kasting, 2016/07/19 21:56:07]

Nit: by -> by the; Chromium -> The browser

[WC Leung, 2016/07/27 18:00:06]

Done.

+// if the specified profile could not initialize.

[Peter Kasting, 2016/07/19 21:56:07]

Nit: initialize -> be initialized

[WC Leung, 2016/07/27 18:00:06]

Done.

+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_NoFallbackForUserSelectedProfile) {
+  CreateAndSwitchToProfile("Profile 1");
+  CreateAndSwitchToProfile("Profile 2");
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForNoFallbackForUserSelectedProfile() {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  command_line->AppendSwitchASCII(switches::kProfileDirectory, "Profile 1");
+  SetExpectTestBodyToRun(false);
+  return DeleteProfileData("Profile 1") &&
+         RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       NoFallbackForUserSelectedProfile) {
+  ADD_FAILURE() << "Test body is not expected to run.";
+}
+
+// DeletedProfileFallbackToUserManager : If the profile is deleted, which is
+// known by having its entry missing in ProfileAttributesStorage, we do not
+// attempt to open the profile. Instead shows the user manager.

[Peter Kasting, 2016/07/19 21:56:07]

Nit: "If the profile's entry in ProfileAttributesStorage is missing, the profile
is deleted.  The browser should not attempt to open the profile, but should show
the user manager instead."

[WC Leung, 2016/07/27 18:00:06]

Done.

+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       PRE_DeletedProfileFallbackToUserManager) {
+  ASSERT_TRUE(base::CreateDirectory(ProfileManager::GetGuestProfilePath()));
+  ASSERT_TRUE(base::CreateDirectory(ProfileManager::GetSystemProfilePath()));
+}
+
+bool StartupBrowserCreatorCorruptProfileTest::
+    SetUpUserDataDirectoryForDeletedProfileFallbackToUserManager() {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  // Simulate a deleted profile by not creating the profile at the first place.
+  command_line->AppendSwitchASCII(switches::kProfileDirectory, "Not Found");
+  return RemoveCreateDirectoryPermissionForUserDataDirectory();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorCorruptProfileTest,
+                       DeletedProfileFallbackToUserManager) {
+  CheckBrowserWindows({});
+  WaitForAndHideUserManager();
+}