Prevent GetDeviceId from invoking its callback multiple times in failure cases.

chrome/browser/prefs/pref_metrics_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/prefs/pref_metrics_service.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/metrics/histogram.h"
@@ skipping 62 matching lines @@
 
 }  // namespace
 
 PrefMetricsService::PrefMetricsService(Profile* profile)
     : profile_(profile),
       prefs_(profile_->GetPrefs()),
       local_state_(g_browser_process->local_state()),
       profile_name_(profile_->GetPath().AsUTF8Unsafe()),
       tracked_pref_paths_(kTrackedPrefs),
       tracked_pref_path_count_(arraysize(kTrackedPrefs)),
-      checked_tracked_prefs_(false),
       weak_factory_(this) {
   pref_hash_seed_ = ResourceBundle::GetSharedInstance().GetRawDataResource(
       IDR_PREF_HASH_SEED_BIN).as_string();
 
   RecordLaunchPrefs();
 
   PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
   synced_pref_change_registrar_.reset(new SyncedPrefChangeRegistrar(prefs));
 
   RegisterSyncedPrefObservers();
@@ skipping 17 matching lines @@
                                        const char** tracked_pref_paths,
                                        int tracked_pref_path_count)
     : profile_(profile),
       prefs_(profile->GetPrefs()),
       local_state_(local_state),
       profile_name_(profile_->GetPath().AsUTF8Unsafe()),
       pref_hash_seed_(kSHA256DigestSize, 0),
       device_id_(device_id),
       tracked_pref_paths_(tracked_pref_paths),
       tracked_pref_path_count_(tracked_pref_path_count),
-      checked_tracked_prefs_(false),
       weak_factory_(this) {
   CheckTrackedPreferences();
 }
 
 PrefMetricsService::~PrefMetricsService() {
 }
 
 void PrefMetricsService::RecordLaunchPrefs() {
   bool show_home_button = prefs_->GetBoolean(prefs::kShowHomeButton);
   bool home_page_is_ntp = prefs_->GetBoolean(prefs::kHomePageIsNewTabPage);
@@ skipping 121 matching lines @@
   base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
       histogram_name,
       1,
       boundary_value,
       boundary_value + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag);
   histogram->Add(integer_value);
 }
 
 void PrefMetricsService::GetDeviceIdCallback(const std::string& device_id) {
+#if defined(ENABLE_DCHECK)
+  // Make sure this callback is only called once.
+  static bool called = false;

[Jeffrey Yasskin, 2013/11/21 22:23:22]

This is dangerous because it's process-wide. How sure are you that the user will
never create a PrefMetricsService for more than one profile in the same process?

[gab, 2013/11/22 01:52:13]

Oops, indeed, this is currently only called for the first profile, but we need
to change this soon, so I'll do it correctly, thanks!

+  DCHECK(!called);
+#endif
+
+#if !defined(OS_WIN) || defined(ENABLE_RLZ)
+  // Always expect a non-empty device ID, except on Windows if RLZ is disabled.

[Jeffrey Yasskin, 2013/11/21 22:23:22]

This has too many negatives. Can you make it easier to understand?

[gab, 2013/11/22 01:52:13]

Done.

+  DCHECK(!device_id.empty());
+#endif
+
   device_id_ = device_id;
-  // On Aura, this seems to be called twice.
-  if (!checked_tracked_prefs_)
-    CheckTrackedPreferences();
+  CheckTrackedPreferences();
+
+#if defined(ENABLE_DCHECK)
+  called = true;

[Jeffrey Yasskin, 2013/11/21 22:23:22]

Move this just after the DCHECK(!called) to reduce the number of #ifs.

[gab, 2013/11/22 01:52:13]

Done.

+#endif
 }
 
 // To detect changes to Preferences that happen outside of Chrome, we hash
 // selected pref values and save them in local state. CheckTrackedPreferences
 // compares the saved values to the values observed in the profile's prefs. A
 // dictionary of dictionaries in local state holds the hashed values, grouped by
 // profile. To make the system more resistant to spoofing, pref values are
 // hashed with the pref path and the device id.
 void PrefMetricsService::CheckTrackedPreferences() {
-  DCHECK(!checked_tracked_prefs_);
-
   const base::DictionaryValue* pref_hash_dicts =
       local_state_->GetDictionary(prefs::kProfilePreferenceHashes);
   // Get the hashed prefs dictionary if it exists. If it doesn't, it will be
   // created if we set preference values below.
   const base::DictionaryValue* hashed_prefs = NULL;
   pref_hash_dicts->GetDictionaryWithoutPathExpansion(profile_name_,
                                                      &hashed_prefs);
   for (int i = 0; i < tracked_pref_path_count_; ++i) {
     // Skip prefs that haven't been registered.
     if (!prefs_->FindPreference(tracked_pref_paths_[i]))
@@ skipping 42 matching lines @@
       }
     } else {
       // Record that we haven't tracked this preference yet, or the hash in
       // local state was removed.
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
           i, tracked_pref_path_count_);
       UpdateTrackedPreference(tracked_pref_paths_[i]);
     }
   }
 
-  checked_tracked_prefs_ = true;
-
   // Now that we've checked the incoming preferences, register for change
   // notifications, unless this is test code.
   // TODO(bbudge) Fix failing browser_tests and so we can remove this test.
   // Several tests fail when they shutdown before they can write local state.
   if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType) &&
       !CommandLine::ForCurrentProcess()->HasSwitch(switches::kChromeFrame)) {
     InitializePrefObservers();
   }
 }
 
@@ skipping 103 matching lines @@
 }
 
 bool PrefMetricsService::Factory::ServiceIsNULLWhileTesting() const {
   return false;
 }
 
 content::BrowserContext* PrefMetricsService::Factory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   return chrome::GetBrowserContextRedirectedInIncognito(context);
 }