Commit e3966b15 authored by Maksim Moskvitin's avatar Maksim Moskvitin Committed by Commit Bot

Fix crash in ProfileSyncService::GetDisableReasons()

PeopleHandler somehow gets destroyed after ProfileSyncService shutdown,
so |auth_manager_| is nullptr during its destruction. This caused the
crash in GetDisableReasons().

Fix is to simply check |auth_manager_| before dereferencing. Avoiding
PSS usages after shutdown would be cleaner fix, but it's not clear why
they happen atm.

Bug: 1043642, 906995
Change-Id: Id6d54a2edee1210a579e36b2b771d360c8c40474
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2011220
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733582}
parent 1ba73ef0
......@@ -694,7 +694,6 @@ SyncService::DisableReasonSet ProfileSyncService::GetDisableReasons() const {
// If Sync is disabled via command line flag, then ProfileSyncService
// shouldn't even be instantiated.
DCHECK(switches::IsSyncAllowedByFlag());
DisableReasonSet result;
if (!user_settings_->IsSyncAllowedByPlatform()) {
result.Put(DISABLE_REASON_PLATFORM_OVERRIDE);
......@@ -715,7 +714,11 @@ SyncService::DisableReasonSet ProfileSyncService::GetDisableReasons() const {
result.Put(DISABLE_REASON_UNRECOVERABLE_ERROR);
}
if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
if (auth_manager_->IsSyncPaused()) {
// Some crashes on Chrome OS (crbug.com/1043642) suggest that
// ProfileSyncService gets called after its shutdown. It's not clear why
// this actually happens. To avoid crashes check that |auth_manager_| isn't
// null.
if (auth_manager_ && auth_manager_->IsSyncPaused()) {
result.Put(DISABLE_REASON_PAUSED);
}
}
......
......@@ -323,6 +323,19 @@ class ProfileSyncServiceTest : public ::testing::Test {
std::unique_ptr<ProfileSyncService> service_;
};
class ProfileSyncServiceTestWithStopSyncInPausedState
: public ProfileSyncServiceTest {
public:
ProfileSyncServiceTestWithStopSyncInPausedState() {
override_features_.InitAndEnableFeature(switches::kStopSyncInPausedState);
}
~ProfileSyncServiceTestWithStopSyncInPausedState() override = default;
private:
base::test::ScopedFeatureList override_features_;
};
// Gets the now time used for testing user demographics.
base::Time GetNowTime() {
base::Time now;
......@@ -1525,5 +1538,16 @@ TEST_F(ProfileSyncServiceTest, GetExperimentalAuthenticationKeyLocalSync) {
EXPECT_FALSE(service()->GetExperimentalAuthenticationKey());
}
// Regression test for crbug.com/1043642, can be removed once
// ProfileSyncService usages after shutdown are addressed.
TEST_F(ProfileSyncServiceTestWithStopSyncInPausedState,
ShouldProvideDisableReasonsAfterShutdown) {
CreateService(ProfileSyncService::AUTO_START);
InitializeForFirstSync();
SignIn();
service()->Shutdown();
EXPECT_FALSE(service()->GetDisableReasons().Empty());
}
} // namespace
} // namespace syncer
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