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
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
// 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 4280b44f..56f9776 100644
--- a/chrome/browser/metrics/metrics_service.h
+++ b/chrome/browser/metrics/metrics_service.h
@@ -151,7 +151,8 @@
// 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 @@
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
