Inform GetEntropySource of whether or not metrics reporting will be enabled.

This resolves an issue where we were calling GetEntropySource too early before MetricsService::Start was called. Instead of checking the reporting_active member in MetricsService, we instead just check if the criteria for metrics reporting is allowed or not, ahead of time.

BUG=141628


Review URL: https://chromiumcodereview.appspot.com/10830241

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@150934 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chrome_browser_main.cc

@@ -542,10 +542,12 @@ void ChromeBrowserMainParts::SetupMetricsAndFieldTrials() {
   // Initialize FieldTrialList to support FieldTrials that use one-time
   // randomization.
   MetricsService* metrics = browser_process_->metrics_service();
-  if (IsMetricsReportingEnabled())
+  bool metrics_reporting_enabled = IsMetricsReportingEnabled();
+  if (metrics_reporting_enabled)
     metrics->ForceClientIdCreation();  // Needed below.
   field_trial_list_.reset(
-      new base::FieldTrialList(metrics->GetEntropySource()));
+      new base::FieldTrialList(
+          metrics->GetEntropySource(metrics_reporting_enabled)));
 
   // Ensure any field trials specified on the command line are initialized.
   // Also stop the metrics service so that we don't pollute UMA.
@@ -595,11 +597,14 @@ void ChromeBrowserMainParts::StartMetricsRecording() {
 bool ChromeBrowserMainParts::IsMetricsReportingEnabled() {
   // If the user permits metrics reporting with the checkbox in the
   // prefs, we turn on recording.  We disable metrics completely for
-  // non-official builds.
+  // non-official builds.  This can be forced with a flag.
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kEnableMetricsReportingForTesting))
+    return true;
+
   bool enabled = false;
 #ifndef NDEBUG
   // The debug build doesn't send UMA logs when FieldTrials are forced.
-  const CommandLine* command_line = CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kForceFieldTrials))
     return false;
 #endif  // #ifndef NDEBUG

chrome/browser/metrics/metrics_service.cc

@@ -502,7 +502,8 @@ MetricsService::MetricsService()
       next_window_id_(0),
       ALLOW_THIS_IN_INITIALIZER_LIST(self_ptr_factory_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(state_saver_factory_(this)),
-      waiting_for_asynchronous_reporting_step_(false) {
+      waiting_for_asynchronous_reporting_step_(false),
+      entropy_source_returned_(LAST_ENTROPY_NONE) {
   DCHECK(IsSingleThreaded());
   InitializeMetricsState();
 
@@ -538,7 +539,7 @@ std::string MetricsService::GetClientId() {
   return client_id_;
 }
 
-std::string MetricsService::GetEntropySource() {
+std::string MetricsService::GetEntropySource(bool reporting_will_be_enabled) {
   // For metrics reporting-enabled users, we combine the client ID and low
   // entropy source to get the final entropy source. Otherwise, only use the low
   // entropy source.
@@ -547,8 +548,11 @@ std::string MetricsService::GetEntropySource() {
   //     know the low entropy source.
   //  2) It makes the final entropy source resettable.
   std::string low_entropy_source = base::IntToString(GetLowEntropySource());
-  if (reporting_active())
+  if (reporting_will_be_enabled) {
+    entropy_source_returned_ = LAST_ENTROPY_HIGH;
     return client_id_ + low_entropy_source;
+  }
+  entropy_source_returned_ = LAST_ENTROPY_LOW;
   return low_entropy_source;
 }
 

chrome/browser/metrics/metrics_service.h

@@ -87,10 +87,15 @@ class MetricsService
   std::string GetClientId();
 
   // Returns the preferred entropy source used to seed persistent activities
-  // based on whether or not metrics reporting is permitted on this client. If
-  // it is permitted, this returns the client ID concatenated with the low
-  // entropy source. Otherwise, this just returns the low entropy source.
-  std::string GetEntropySource();
+  // based on whether or not metrics reporting will permitted on this client.
+  // The caller must determine if metrics reporting will be enabled for this
+  // client and pass that state in as |reporting_will_be_enabled|. If
+  // |reporting_will_be_enabled| is true, this method returns the client ID
+  // concatenated with the low entropy source. Otherwise, this method just
+  // returns the low entropy source. Note that this reporting state can not be
+  // checked by reporting_active() because this method may need to be called
+  // before the MetricsService needs to be started.
+  std::string GetEntropySource(bool reporting_will_be_enabled);
 
   // Force the client ID to be generated. This is useful in case it's needed
   // before recording.
@@ -177,6 +182,15 @@ class MetricsService
     NEED_TO_SHUTDOWN = ~CLEANLY_SHUTDOWN
   };
 
+  // Designates which entropy source was returned from this MetricsService.
+  // This is used for testing to validate that we return the correct source
+  // depending on the state of the service.
+  enum EntropySourceReturned {
+    LAST_ENTROPY_NONE,
+    LAST_ENTROPY_LOW,
+    LAST_ENTROPY_HIGH,
+  };
+
   // First part of the init task. Called on the FILE thread to load hardware
   // class information.
   static void InitTaskGetHardwareClass(base::WeakPtr<MetricsService> self,
@@ -213,6 +227,13 @@ class MetricsService
   // generate the entropy source value if it has not been called before.
   int GetLowEntropySource();
 
+  // Returns the last entropy source that was returned by this service since
+  // start up, or NONE if neither was returned yet. This is exposed for testing
+  // only.
+  EntropySourceReturned entropy_source_returned() const {
+    return entropy_source_returned_;
+  }
+
   // When we start a new version of Chromium (different from our last run), we
   // need to discard the old crash stats so that we don't attribute crashes etc.
   // in the old version to the current version (via current logs).
@@ -462,12 +483,18 @@ class MetricsService
   scoped_refptr<chromeos::ExternalMetrics> external_metrics_;
 #endif
 
+  // The last entropy source returned by this service, used for testing.
+  EntropySourceReturned entropy_source_returned_;
+
   // Reduntant marker to check that we completed our shutdown, and set the
   // exited-cleanly bit in the prefs.
   static ShutdownCleanliness clean_shutdown_status_;
 
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ClientIdCorrectlyFormatted);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, IsPluginProcess);
+  FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, CheckLowEntropySourceUsed);
+  FRIEND_TEST_ALL_PREFIXES(MetricsServiceReportingTest,
+                           CheckHighEntropySourceUsed);
 
   DISALLOW_COPY_AND_ASSIGN(MetricsService);
 };

chrome/browser/metrics/metrics_service_browsertest.cc

@@ -11,6 +11,7 @@
 #include "base/file_path.h"
 #include "base/path_service.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -56,6 +57,14 @@ class MetricsServiceTest : public InProcessBrowserTest {
   }
 };
 
+class MetricsServiceReportingTest : public InProcessBrowserTest {
+ public:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    // Enable the metrics service for testing (in the full mode).
+    command_line->AppendSwitch(switches::kEnableMetricsReportingForTesting);
+  }
+};
+
 IN_PROC_BROWSER_TEST_F(MetricsServiceTest, CloseRenderersNormally) {
   OpenTabs();
 
@@ -104,3 +113,21 @@ IN_PROC_BROWSER_TEST_F(MetricsServiceTest, MAYBE_CrashRenderers) {
   // is set to true, but this preference isn't set until the browser
   // exits... it's not clear to me how to test that.
 }
+
+IN_PROC_BROWSER_TEST_F(MetricsServiceTest, CheckLowEntropySourceUsed) {
+  // Since MetricsService is only in recording mode, and is not reporting,
+  // check that the low entropy source is returned at some point.
+  ASSERT_TRUE(g_browser_process->metrics_service());
+  EXPECT_EQ(MetricsService::LAST_ENTROPY_LOW,
+            g_browser_process->metrics_service()->entropy_source_returned());
+}
+
+IN_PROC_BROWSER_TEST_F(MetricsServiceReportingTest,
+                       CheckHighEntropySourceUsed) {
+  // Since the full metrics service runs in this test, we expect that
+  // MetricsService returns the full entropy source at some point during
+  // BrowserMain startup.
+  ASSERT_TRUE(g_browser_process->metrics_service());
+  EXPECT_EQ(MetricsService::LAST_ENTROPY_HIGH,
+            g_browser_process->metrics_service()->entropy_source_returned());
+}

chrome/common/chrome_switches.cc

@@ -569,6 +569,12 @@ const char kEnableMediaGalleryUI[]          = "enable-media-gallery-ui";
 // Allows reporting memory info (JS heap size) to page.
 const char kEnableMemoryInfo[]              = "enable-memory-info";
 
+// Enables metrics recording and reporting in the browser startup sequence, as
+// if this was an official Chrome build where the user allowed metrics
+// reporting. This is used for testing only.
+const char kEnableMetricsReportingForTesting[] =
+    "enable-metrics-reporting-for-testing";
+
 // Runs the Native Client inside the renderer process and enables GPU plugin
 // (internally adds lEnableGpuPlugin to the command line).
 const char kEnableNaCl[]                    = "enable-nacl";

chrome/common/chrome_switches.h

@@ -160,6 +160,7 @@ extern const char kEnableIPPooling[];
 extern const char kEnableManagedStorage[];
 extern const char kEnableMediaGalleryUI[];
 extern const char kEnableMemoryInfo[];
+extern const char kEnableMetricsReportingForTesting[];
 extern const char kEnableNaCl[];
 extern const char kEnableNaClDebug[];
 extern const char kEnableNaClExceptionHandling[];