From 2f10e381d9d9cc5fec61dbc1c670d2745118326c Mon Sep 17 00:00:00 2001 From: "nkostylev@chromium.org" Date: Tue, 6 May 2014 14:04:45 +0000 Subject: [PATCH] Revert of Start session fail causes restart chrome (https://codereview.chromium.org/228703004/) Reason for revert: Seems to be breaking user sign in. See also http://crbug.com/369040 Original issue's description: > Start session fail causes restart chrome > > BUG=244628 > TEST=manual check on Acer C720 > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266791 NOTREECHECKS=true NOTRY=true Review URL: https://codereview.chromium.org/268293002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@268511 0039d316-1c4b-4281-b951-d872f2087c98 --- .../chromeos/login/crash_restore_browsertest.cc | 38 ++--------------- chrome/browser/chromeos/login/login_utils.cc | 49 +--------------------- .../settings/device_settings_test_helper.cc | 5 +-- .../settings/device_settings_test_helper.h | 3 +- chromeos/dbus/fake_session_manager_client.cc | 5 +-- chromeos/dbus/fake_session_manager_client.h | 3 +- chromeos/dbus/mock_session_manager_client.h | 3 +- chromeos/dbus/session_manager_client.cc | 27 ++++-------- chromeos/dbus/session_manager_client.h | 7 +--- 9 files changed, 19 insertions(+), 121 deletions(-) diff --git a/chrome/browser/chromeos/login/crash_restore_browsertest.cc b/chrome/browser/chromeos/login/crash_restore_browsertest.cc index e7470c92e966..8bcaaf5576c0 100644 --- a/chrome/browser/chromeos/login/crash_restore_browsertest.cc +++ b/chrome/browser/chromeos/login/crash_restore_browsertest.cc @@ -33,7 +33,7 @@ const char kUserId3[] = "user3@example.com"; class CrashRestoreSimpleTest : public InProcessBrowserTest { protected: - CrashRestoreSimpleTest() : session_started_count_(0) {} + CrashRestoreSimpleTest() {} virtual ~CrashRestoreSimpleTest() {} @@ -53,27 +53,10 @@ class CrashRestoreSimpleTest : public InProcessBrowserTest { dbus_thread_manager->SetSessionManagerClient( scoped_ptr(session_manager_client_)); DBusThreadManager::SetInstanceForTesting(dbus_thread_manager); - // We need to create MessageLoop here to process callback from - // session_manager - base::MessageLoop msg_loop; - session_manager_client_->StartSession( - kUserId1, - base::Bind(&CrashRestoreSimpleTest::OnSessionStarted, - base::Unretained(this), - kUserId1)); - base::RunLoop().RunUntilIdle(); - ASSERT_EQ(1, session_started_count_); + session_manager_client_->StartSession(kUserId1); } - public: - void OnSessionStarted(const std::string& user_email, bool success) { - ASSERT_TRUE(success); - ++session_started_count_; - } - - protected: FakeSessionManagerClient* session_manager_client_; - int session_started_count_; }; IN_PROC_BROWSER_TEST_F(CrashRestoreSimpleTest, RestoreSessionForOneUser) { @@ -135,21 +118,8 @@ class CrashRestoreComplexTest : public CrashRestoreSimpleTest { virtual void SetUpInProcessBrowserTestFixture() OVERRIDE { CrashRestoreSimpleTest::SetUpInProcessBrowserTestFixture(); - // We need to create MessageLoop here to process callback from - // session_manager - base::MessageLoop msg_loop; - session_manager_client_->StartSession( - kUserId2, - base::Bind(&CrashRestoreSimpleTest::OnSessionStarted, - base::Unretained(this), - kUserId2)); - session_manager_client_->StartSession( - kUserId3, - base::Bind(&CrashRestoreSimpleTest::OnSessionStarted, - base::Unretained(this), - kUserId3)); - base::RunLoop().RunUntilIdle(); - ASSERT_EQ(3, CrashRestoreSimpleTest::session_started_count_); + session_manager_client_->StartSession(kUserId2); + session_manager_client_->StartSession(kUserId3); } }; diff --git a/chrome/browser/chromeos/login/login_utils.cc b/chrome/browser/chromeos/login/login_utils.cc index 7801be1db386..792b107dbd83 100644 --- a/chrome/browser/chromeos/login/login_utils.cc +++ b/chrome/browser/chromeos/login/login_utils.cc @@ -200,19 +200,6 @@ class LoginUtilsImpl // the authentication profile. void CompleteProfileCreate(Profile* user_profile); - // Callback to resume profile preparing after start session. - void OnSessionStarted(const UserContext& user_context, - const std::string& display_email, - bool has_cookies, - LoginUtils::Delegate* delegate, - bool success); - - // Complete profile preparation with having active session. - void CompletePrepareProfileWithActiveSession(const UserContext& user_context, - const std::string& display_email, - bool has_cookies, - LoginUtils::Delegate* delegate); - // Finalized profile preparation. void FinalizePrepareProfile(Profile* user_profile); @@ -413,42 +400,10 @@ void LoginUtilsImpl::PrepareProfile( if (!has_active_session) { btl->AddLoginTimeMarker("StartSession-Start", false); DBusThreadManager::Get()->GetSessionManagerClient()->StartSession( - user_context.username, - base::Bind(&LoginUtilsImpl::OnSessionStarted, - AsWeakPtr(), - user_context, - display_email, - has_cookies, - delegate)); - return; - } - CompletePrepareProfileWithActiveSession( - user_context, display_email, has_cookies, delegate); -} - -void LoginUtilsImpl::OnSessionStarted(const UserContext& user_context, - const std::string& display_email, - bool has_cookies, - LoginUtils::Delegate* delegate, - bool success) { - BootTimesLoader* btl = BootTimesLoader::Get(); - btl->AddLoginTimeMarker("StartSession-End", false); - - if (!success) { - LOG(ERROR) << "StartSession failed"; - chrome::AttemptUserExit(); - return; + user_context.username); + btl->AddLoginTimeMarker("StartSession-End", false); } - CompletePrepareProfileWithActiveSession( - user_context, display_email, has_cookies, delegate); -} -void LoginUtilsImpl::CompletePrepareProfileWithActiveSession( - const UserContext& user_context, - const std::string& display_email, - bool has_cookies, - LoginUtils::Delegate* delegate) { - BootTimesLoader* btl = BootTimesLoader::Get(); btl->AddLoginTimeMarker("UserLoggedIn-Start", false); UserManager* user_manager = UserManager::Get(); user_manager->UserLoggedIn(user_context.username, diff --git a/chrome/browser/chromeos/settings/device_settings_test_helper.cc b/chrome/browser/chromeos/settings/device_settings_test_helper.cc index 922dc2963313..bb1e5b651380 100644 --- a/chrome/browser/chromeos/settings/device_settings_test_helper.cc +++ b/chrome/browser/chromeos/settings/device_settings_test_helper.cc @@ -116,10 +116,7 @@ void DeviceSettingsTestHelper::EmitLoginPromptVisible() {} void DeviceSettingsTestHelper::RestartJob(int pid, const std::string& command_line) {} -void DeviceSettingsTestHelper::StartSession( - const std::string& user_email, - const StartSessionCallback& callback) { -} +void DeviceSettingsTestHelper::StartSession(const std::string& user_email) {} void DeviceSettingsTestHelper::StopSession() {} diff --git a/chrome/browser/chromeos/settings/device_settings_test_helper.h b/chrome/browser/chromeos/settings/device_settings_test_helper.h index af683d013af6..71ae633afcec 100644 --- a/chrome/browser/chromeos/settings/device_settings_test_helper.h +++ b/chrome/browser/chromeos/settings/device_settings_test_helper.h @@ -88,8 +88,7 @@ class DeviceSettingsTestHelper : public SessionManagerClient { virtual bool HasObserver(Observer* observer) OVERRIDE; virtual void EmitLoginPromptVisible() OVERRIDE; virtual void RestartJob(int pid, const std::string& command_line) OVERRIDE; - virtual void StartSession(const std::string& user_email, - const StartSessionCallback& callback) OVERRIDE; + virtual void StartSession(const std::string& user_email) OVERRIDE; virtual void StopSession() OVERRIDE; virtual void StartDeviceWipe() OVERRIDE; virtual void RequestLockScreen() OVERRIDE; diff --git a/chromeos/dbus/fake_session_manager_client.cc b/chromeos/dbus/fake_session_manager_client.cc index e2f9404704f4..c5fe549335d4 100644 --- a/chromeos/dbus/fake_session_manager_client.cc +++ b/chromeos/dbus/fake_session_manager_client.cc @@ -46,14 +46,11 @@ void FakeSessionManagerClient::RestartJob(int pid, const std::string& command_line) { } -void FakeSessionManagerClient::StartSession( - const std::string& user_email, - const StartSessionCallback& callback) { +void FakeSessionManagerClient::StartSession(const std::string& user_email) { DCHECK_EQ(0UL, user_sessions_.count(user_email)); std::string user_id_hash = CryptohomeClient::GetStubSanitizedUsername(user_email); user_sessions_[user_email] = user_id_hash; - base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback, true)); } void FakeSessionManagerClient::StopSession() { diff --git a/chromeos/dbus/fake_session_manager_client.h b/chromeos/dbus/fake_session_manager_client.h index 1a6c782ce03c..2f4bb9a0955f 100644 --- a/chromeos/dbus/fake_session_manager_client.h +++ b/chromeos/dbus/fake_session_manager_client.h @@ -31,8 +31,7 @@ class FakeSessionManagerClient : public SessionManagerClient { virtual bool HasObserver(Observer* observer) OVERRIDE; virtual void EmitLoginPromptVisible() OVERRIDE; virtual void RestartJob(int pid, const std::string& command_line) OVERRIDE; - virtual void StartSession(const std::string& user_email, - const StartSessionCallback& callback) OVERRIDE; + virtual void StartSession(const std::string& user_email) OVERRIDE; virtual void StopSession() OVERRIDE; virtual void StartDeviceWipe() OVERRIDE; virtual void RequestLockScreen() OVERRIDE; diff --git a/chromeos/dbus/mock_session_manager_client.h b/chromeos/dbus/mock_session_manager_client.h index 6847ee072980..c0e45310c936 100644 --- a/chromeos/dbus/mock_session_manager_client.h +++ b/chromeos/dbus/mock_session_manager_client.h @@ -24,8 +24,7 @@ class MockSessionManagerClient : public SessionManagerClient { MOCK_METHOD1(HasObserver, bool(Observer*)); MOCK_METHOD0(EmitLoginPromptVisible, void(void)); MOCK_METHOD2(RestartJob, void(int, const std::string&)); - MOCK_METHOD2(StartSession, - void(const std::string&, const StartSessionCallback&)); + MOCK_METHOD1(StartSession, void(const std::string&)); MOCK_METHOD0(StopSession, void(void)); MOCK_METHOD0(StartDeviceWipe, void(void)); MOCK_METHOD0(RequestLockScreen, void(void)); diff --git a/chromeos/dbus/session_manager_client.cc b/chromeos/dbus/session_manager_client.cc index c5693a844d43..bfa9aa448dc2 100644 --- a/chromeos/dbus/session_manager_client.cc +++ b/chromeos/dbus/session_manager_client.cc @@ -107,8 +107,7 @@ class SessionManagerClientImpl : public SessionManagerClient { weak_ptr_factory_.GetWeakPtr())); } - virtual void StartSession(const std::string& user_email, - const StartSessionCallback& callback) OVERRIDE { + virtual void StartSession(const std::string& user_email) OVERRIDE { dbus::MethodCall method_call(login_manager::kSessionManagerInterface, login_manager::kSessionManagerStartSession); dbus::MessageWriter writer(&method_call); @@ -118,8 +117,7 @@ class SessionManagerClientImpl : public SessionManagerClient { &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, base::Bind(&SessionManagerClientImpl::OnStartSession, - weak_ptr_factory_.GetWeakPtr(), - callback)); + weak_ptr_factory_.GetWeakPtr())); } virtual void StopSession() OVERRIDE { @@ -383,18 +381,10 @@ class SessionManagerClientImpl : public SessionManagerClient { } // Called when kSessionManagerStartSession method is complete. - void OnStartSession(const StartSessionCallback& callback, - dbus::Response* response) { - bool success = false; - if (!response) { - LOG(ERROR) << "Failed to call " - << login_manager::kSessionManagerStartSession; - } else { - dbus::MessageReader reader(response); - if (!reader.PopBool(&success)) - LOG(ERROR) << "Invalid response: " << response->ToString(); - } - callback.Run(success); + void OnStartSession(dbus::Response* response) { + LOG_IF(ERROR, !response) + << "Failed to call " + << login_manager::kSessionManagerStartSession; } // Called when kSessionManagerStopSession method is complete. @@ -610,10 +600,7 @@ class SessionManagerClientStubImpl : public SessionManagerClient { } virtual void EmitLoginPromptVisible() OVERRIDE {} virtual void RestartJob(int pid, const std::string& command_line) OVERRIDE {} - virtual void StartSession(const std::string& user_email, - const StartSessionCallback& callback) OVERRIDE { - callback.Run(true); - } + virtual void StartSession(const std::string& user_email) OVERRIDE {} virtual void StopSession() OVERRIDE {} virtual void StartDeviceWipe() OVERRIDE {} virtual void RequestLockScreen() OVERRIDE { diff --git a/chromeos/dbus/session_manager_client.h b/chromeos/dbus/session_manager_client.h index a9ae670cc6dc..d4c07066e2de 100644 --- a/chromeos/dbus/session_manager_client.h +++ b/chromeos/dbus/session_manager_client.h @@ -71,13 +71,8 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient { // Restarts a job referenced by |pid| with the provided command line. virtual void RestartJob(int pid, const std::string& command_line) = 0; - // Used for StartSession. Takes a boolean indicating whether the - // operation was successful or not. - typedef base::Callback StartSessionCallback; - // Starts the session for the user. - virtual void StartSession(const std::string& user_email, - const StartSessionCallback& callback) = 0; + virtual void StartSession(const std::string& user_email) = 0; // Stops the current session. virtual void StopSession() = 0; -- 2.11.4.GIT