Eliminate race condition in MetricsService and re-enable WizardControllerTest

BUG=110544
TEST=WizardControllerFlowTest.ControlFlowMain, no error during 100 cycles


Review URL: http://codereview.chromium.org/9355058

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

3 files changed, 21 insertions, 17 deletions

diff --git a/chrome/browser/chromeos/login/wizard_controller_browsertest.cc b/chrome/browser/chromeos/login/wizard_controller_browsertest.cc
index 2566e2c..4a82d34 100644
--- a/chrome/browser/chromeos/login/wizard_controller_browsertest.cc
+++ b/chrome/browser/chromeos/login/wizard_controller_browsertest.cc
@@ -141,8 +141,7 @@ class WizardControllerFlowTest : public WizardControllerTest {
   DISALLOW_COPY_AND_ASSIGN(WizardControllerFlowTest);
 };
 
-// Seems to be flaky http://crbug.com/110544.
-IN_PROC_BROWSER_TEST_F(WizardControllerFlowTest, DISABLED_ControlFlowMain) {
+IN_PROC_BROWSER_TEST_F(WizardControllerFlowTest, ControlFlowMain) {
   EXPECT_TRUE(ExistingUserController::current_controller() == NULL);
   EXPECT_EQ(controller()->GetNetworkScreen(), controller()->current_screen());
   EXPECT_CALL(*mock_network_screen_, Hide()).Times(1);
@@ -166,9 +165,7 @@ IN_PROC_BROWSER_TEST_F(WizardControllerFlowTest, DISABLED_ControlFlowMain) {
   set_controller(NULL);
 }
 
-// Seems to be flaky http://crbug.com/110544.
-IN_PROC_BROWSER_TEST_F(WizardControllerFlowTest,
-                       DISABLED_ControlFlowErrorUpdate) {
+IN_PROC_BROWSER_TEST_F(WizardControllerFlowTest, ControlFlowErrorUpdate) {
   EXPECT_EQ(controller()->GetNetworkScreen(), controller()->current_screen());
   EXPECT_CALL(*mock_update_screen_, StartUpdate()).Times(0);
   EXPECT_CALL(*mock_eula_screen_, Show()).Times(1);
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index 634ef6a..1626a5c 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -369,14 +369,14 @@ MetricsService::MetricsService()
       io_thread_(NULL),
       idle_since_last_transmission_(false),
       next_window_id_(0),
-      ALLOW_THIS_IN_INITIALIZER_LIST(log_sender_factory_(this)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(self_ptr_factory_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(state_saver_factory_(this)),
       waiting_for_asynchronus_reporting_step_(false) {
   DCHECK(IsSingleThreaded());
   InitializeMetricsState();
 
   base::Closure callback = base::Bind(&MetricsService::StartScheduledUpload,
-                                      base::Unretained(this));
+                                      self_ptr_factory_.GetWeakPtr());
   scheduler_.reset(new MetricsReportingScheduler(callback));
   log_manager_.set_log_serializer(new MetricsLogSerializer());
   log_manager_.set_max_ongoing_log_store_size(kUploadLogAvoidRetransmitSize);
@@ -723,9 +723,10 @@ void MetricsService::InitializeMetricsState() {
   ScheduleNextStateSave();
 }
 
+// static
 void MetricsService::InitTaskGetHardwareClass(
+    base::WeakPtr<MetricsService> self,
     base::MessageLoopProxy* target_loop) {
-  DCHECK(state_ == INIT_TASK_SCHEDULED);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   std::string hardware_class;
@@ -736,23 +737,23 @@ void MetricsService::InitTaskGetHardwareClass(
 
   target_loop->PostTask(FROM_HERE,
       base::Bind(&MetricsService::OnInitTaskGotHardwareClass,
-          base::Unretained(this), hardware_class));
+          self, hardware_class));
 }
 
 void MetricsService::OnInitTaskGotHardwareClass(
     const std::string& hardware_class) {
-  DCHECK(state_ == INIT_TASK_SCHEDULED);
+  DCHECK_EQ(state_, INIT_TASK_SCHEDULED);
   hardware_class_ = hardware_class;
 
   // Start the next part of the init task: loading plugin information.
   PluginService::GetInstance()->GetPlugins(
       base::Bind(&MetricsService::OnInitTaskGotPluginInfo,
-          base::Unretained(this)));
+          self_ptr_factory_.GetWeakPtr()));
 }
 
 void MetricsService::OnInitTaskGotPluginInfo(
     const std::vector<webkit::WebPluginInfo>& plugins) {
-  DCHECK(state_ == INIT_TASK_SCHEDULED);
+  DCHECK_EQ(state_, INIT_TASK_SCHEDULED);
   plugins_ = plugins;
 
   io_thread_ = g_browser_process->io_thread();
@@ -810,7 +811,7 @@ void MetricsService::StartRecording() {
         BrowserThread::FILE,
         FROM_HERE,
         base::Bind(&MetricsService::InitTaskGetHardwareClass,
-            base::Unretained(this),
+            self_ptr_factory_.GetWeakPtr(),
             MessageLoop::current()->message_loop_proxy()),
         kInitializationDelaySeconds);
   }
@@ -887,7 +888,7 @@ void MetricsService::StartScheduledUpload() {
 
   base::Closure callback =
       base::Bind(&MetricsService::OnMemoryDetailCollectionDone,
-                 log_sender_factory_.GetWeakPtr());
+                 self_ptr_factory_.GetWeakPtr());
 
   scoped_refptr<MetricsMemoryDetails> details(
       new MetricsMemoryDetails(callback));
@@ -916,7 +917,7 @@ void MetricsService::OnMemoryDetailCollectionDone() {
   // Create a callback_task for OnHistogramSynchronizationDone.
   base::Closure callback = base::Bind(
       &MetricsService::OnHistogramSynchronizationDone,
-      log_sender_factory_.GetWeakPtr());
+      self_ptr_factory_.GetWeakPtr());
 
   base::StatisticsRecorder::CollectHistogramStats("Browser");
 
diff --git a/chrome/browser/metrics/metrics_service.h b/chrome/browser/metrics/metrics_service.h
index 4280b44..56f9776 100644
--- a/chrome/browser/metrics/metrics_service.h
+++ b/chrome/browser/metrics/metrics_service.h
@@ -151,7 +151,8 @@ class MetricsService : public content::NotificationObserver,
 
   // First part of the init task. Called on the FILE thread to load hardware
   // class information.
-  void InitTaskGetHardwareClass(base::MessageLoopProxy* target_loop);
+  static void InitTaskGetHardwareClass(base::WeakPtr<MetricsService> self,
+                                       base::MessageLoopProxy* target_loop);
 
   // Callback from InitTaskGetHardwareClass() that continues the init task by
   // loading plugin information.
@@ -374,7 +375,12 @@ class MetricsService : public content::NotificationObserver,
   struct ChildProcessStats;
   std::map<string16, ChildProcessStats> child_process_stats_buffer_;
 
-  base::WeakPtrFactory<MetricsService> log_sender_factory_;
+  // Weak pointers factory used to post task on different threads. All weak
+  // pointers managed by this factory have the same lifetime as MetricsService.
+  base::WeakPtrFactory<MetricsService> self_ptr_factory_;
+
+  // Weak pointers factory used for saving state. All weak pointers managed by
+  // this factory are invalidated in ScheduleNextStateSave.
   base::WeakPtrFactory<MetricsService> state_saver_factory_;
 
   // Dictionary containing all the profile specific metrics. This is set