Fix NOTREACHED crash if ReadingListFactory isn't instantiated
The cleanup code for deprecated preferences can crash (NOTREACHED) if
the preference isn't registered. Before this patch, the registration
relied on the factories built-in support for preference registration,
i.e. RegisterBrowserStatePrefs()/RegisterProfilePrefs(), but this turns
out to be unreliable for the purpose of data cleanup, which is exercised
independently of factory creation.
The fix is to register the deprecated preference closer to the cleanup
logic, instead of relying on the factories.
Change-Id: Ia82dbb7dc308d29e878d648d230ab5fd3e7a646d
Fixed: 1431882
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4459277
Reviewed-by: Shakti Sahu <[email protected]>
Reviewed-by: Olivier Robin <[email protected]>
Reviewed-by: Dominic Battre <[email protected]>
Code-Coverage: Findit <[email protected]>
Commit-Queue: Mikel Astiz <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1134315}
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index 7db8872..d11c0f133 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -139,7 +139,6 @@
#include "components/prefs/pref_service.h"
#include "components/privacy_sandbox/privacy_sandbox_prefs.h"
#include "components/proxy_config/pref_proxy_config_tracker_impl.h"
-#include "components/reading_list/core/reading_list_pref_names.h"
#include "components/safe_browsing/content/common/file_type_policies_prefs.h"
#include "components/safe_browsing/core/common/safe_browsing_prefs.h"
#include "components/search_engines/template_url_prepopulate_data.h"
@@ -675,6 +674,8 @@
// Deprecated 12/2022.
const char kAutofillWalletImportStorageCheckboxState[] =
"autofill.wallet_import_storage_checkbox_state";
+const char kDeprecatedReadingListHasUnseenEntries[] =
+ "reading_list.has_unseen_entries";
// Deprecated 01/2023
const char kSendDownloadToCloudPref[] =
@@ -1027,6 +1028,7 @@
// Deprecated 12/2022.
registry->RegisterBooleanPref(kAutofillWalletImportStorageCheckboxState,
true);
+ registry->RegisterBooleanPref(kDeprecatedReadingListHasUnseenEntries, false);
// Deprecated 01/2023.
registry->RegisterBooleanPref(
@@ -2073,8 +2075,7 @@
profile_prefs->ClearPref(kAutofillAssistantTriggerScriptsIsFirstTimeUser);
// Added 12/2022.
- profile_prefs->ClearPref(
- reading_list::prefs::kDeprecatedReadingListHasUnseenEntries);
+ profile_prefs->ClearPref(kDeprecatedReadingListHasUnseenEntries);
// Added 12/2022.
profile_prefs->ClearPref(kAutofillWalletImportStorageCheckboxState);
diff --git a/chrome/browser/reading_list/reading_list_model_factory.cc b/chrome/browser/reading_list/reading_list_model_factory.cc
index 69ce95b..52b69a0 100644
--- a/chrome/browser/reading_list/reading_list_model_factory.cc
+++ b/chrome/browser/reading_list/reading_list_model_factory.cc
@@ -95,9 +95,6 @@
void ReadingListModelFactory::RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {
- registry->RegisterBooleanPref(
- reading_list::prefs::kDeprecatedReadingListHasUnseenEntries, false,
- PrefRegistry::NO_REGISTRATION_FLAGS);
#if !BUILDFLAG(IS_ANDROID)
registry->RegisterBooleanPref(
reading_list::prefs::kReadingListDesktopFirstUseExperienceShown, false,
diff --git a/components/reading_list/core/reading_list_pref_names.cc b/components/reading_list/core/reading_list_pref_names.cc
index 997e856..da7d6bf1 100644
--- a/components/reading_list/core/reading_list_pref_names.cc
+++ b/components/reading_list/core/reading_list_pref_names.cc
@@ -9,10 +9,6 @@
namespace reading_list {
namespace prefs {
-// No longer used, only around for cleanup logic.
-const char kDeprecatedReadingListHasUnseenEntries[] =
- "reading_list.has_unseen_entries";
-
#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)
// Boolean to track if the first-use experience has been shown on desktop.
const char kReadingListDesktopFirstUseExperienceShown[] =
diff --git a/components/reading_list/core/reading_list_pref_names.h b/components/reading_list/core/reading_list_pref_names.h
index 53989df1..c4e2937 100644
--- a/components/reading_list/core/reading_list_pref_names.h
+++ b/components/reading_list/core/reading_list_pref_names.h
@@ -12,8 +12,6 @@
namespace reading_list {
namespace prefs {
-extern const char kDeprecatedReadingListHasUnseenEntries[];
-
#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)
extern const char kReadingListDesktopFirstUseExperienceShown[];
#endif // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS)
diff --git a/ios/chrome/browser/prefs/browser_prefs.mm b/ios/chrome/browser/prefs/browser_prefs.mm
index 74a356f..aa7a28e 100644
--- a/ios/chrome/browser/prefs/browser_prefs.mm
+++ b/ios/chrome/browser/prefs/browser_prefs.mm
@@ -39,7 +39,6 @@
#import "components/pref_registry/pref_registry_syncable.h"
#import "components/prefs/pref_service.h"
#import "components/proxy_config/pref_proxy_config_tracker_impl.h"
-#import "components/reading_list/core/reading_list_pref_names.h"
#import "components/safe_browsing/core/common/safe_browsing_prefs.h"
#import "components/search_engines/template_url_prepopulate_data.h"
#import "components/segmentation_platform/embedder/default_model/device_switcher_result_dispatcher.h"
@@ -120,6 +119,10 @@
// Deprecated 11/2022.
const char kLocalConsentsDictionary[] = "local_consents";
+// Deprecated 12/2022.
+const char kDeprecatedReadingListHasUnseenEntries[] =
+ "reading_list.has_unseen_entries";
+
// Deprecated 01/2023.
const char* kTrialGroupMICeAndDefaultBrowserVersionPrefName =
"fre_refactoring_mice_and_default_browser.trial_version";
@@ -384,6 +387,8 @@
registry->RegisterBooleanPref(prefs::kBrowserLockdownModeEnabled, false);
ntp_snippets::prefs::RegisterProfilePrefsForMigrationApril2023(registry);
+
+ registry->RegisterBooleanPref(kDeprecatedReadingListHasUnseenEntries, false);
}
// This method should be periodically pruned of year+ old migrations.
@@ -456,7 +461,7 @@
}
// Added 12/2022.
- prefs->ClearPref(reading_list::prefs::kDeprecatedReadingListHasUnseenEntries);
+ prefs->ClearPref(kDeprecatedReadingListHasUnseenEntries);
// Added 04/2023.
ntp_snippets::prefs::MigrateObsoleteProfilePrefsApril2023(prefs);
diff --git a/ios/chrome/browser/reading_list/reading_list_model_factory.h b/ios/chrome/browser/reading_list/reading_list_model_factory.h
index fc292b9..ded308b 100644
--- a/ios/chrome/browser/reading_list/reading_list_model_factory.h
+++ b/ios/chrome/browser/reading_list/reading_list_model_factory.h
@@ -24,9 +24,6 @@
ReadingListModelFactory(const ReadingListModelFactory&) = delete;
ReadingListModelFactory& operator=(const ReadingListModelFactory&) = delete;
- void RegisterBrowserStatePrefs(
- user_prefs::PrefRegistrySyncable* registry) override;
-
private:
friend class base::NoDestructor<ReadingListModelFactory>;
diff --git a/ios/chrome/browser/reading_list/reading_list_model_factory.mm b/ios/chrome/browser/reading_list/reading_list_model_factory.mm
index 9fa4d2e..e373352 100644
--- a/ios/chrome/browser/reading_list/reading_list_model_factory.mm
+++ b/ios/chrome/browser/reading_list/reading_list_model_factory.mm
@@ -12,11 +12,9 @@
#import "base/no_destructor.h"
#import "base/time/default_clock.h"
#import "components/keyed_service/ios/browser_state_dependency_manager.h"
-#import "components/pref_registry/pref_registry_syncable.h"
#import "components/reading_list/core/dual_reading_list_model.h"
#import "components/reading_list/core/reading_list_model_impl.h"
#import "components/reading_list/core/reading_list_model_storage_impl.h"
-#import "components/reading_list/core/reading_list_pref_names.h"
#import "components/reading_list/features/reading_list_switches.h"
#import "components/sync/base/storage_type.h"
#import "components/sync/model/model_type_store_service.h"
@@ -52,13 +50,6 @@
ReadingListModelFactory::~ReadingListModelFactory() {}
-void ReadingListModelFactory::RegisterBrowserStatePrefs(
- user_prefs::PrefRegistrySyncable* registry) {
- registry->RegisterBooleanPref(
- reading_list::prefs::kDeprecatedReadingListHasUnseenEntries, false,
- PrefRegistry::NO_REGISTRATION_FLAGS);
-}
-
std::unique_ptr<KeyedService> ReadingListModelFactory::BuildServiceInstanceFor(
web::BrowserState* context) const {
ChromeBrowserState* chrome_browser_state =
