Now that uploading to breakpad works on all threads add it to all places that we could return an unrecoverable error. Those places include:

1. Failed model association resulting in unrecoverable error being called on DTC.
2. When disabling a single type due to a perdatatype error.
3. Failing to download requested types resulting in unrecoverable error.


BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125220

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

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

13 files changed, 63 insertions, 44 deletions

diff --git a/chrome/browser/sync/glue/data_type_controller.cc b/chrome/browser/sync/glue/data_type_controller.cc
new file mode 100644
index 0000000..a647854
--- /dev/null
+++ b/chrome/browser/sync/glue/data_type_controller.cc
@@ -0,0 +1,28 @@
+// Copyright (c) 2012 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/sync/glue/chrome_report_unrecoverable_error.h"
+
+#include "chrome/browser/sync/glue/data_type_controller.h"
+#include "chrome/browser/sync/util/data_type_histogram.h"
+
+namespace browser_sync {
+
+bool DataTypeController::IsUnrecoverableResult(StartResult result) {
+  return (result == ASSOCIATION_FAILED || result == UNRECOVERABLE_ERROR);
+}
+
+void DataTypeController::RecordUnrecoverableError(
+    const tracked_objects::Location& from_here,
+    const std::string& message) {
+  DVLOG(1) << "Datatype Controller failed for type "
+           << ModelTypeToString(type()) << "  "
+           << message << " at location "
+           << from_here.ToString();
+  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures", type(),
+                            syncable::MODEL_TYPE_COUNT);
+  ChromeReportUnrecoverableError();
+}
+
+}
diff --git a/chrome/browser/sync/glue/data_type_controller.h b/chrome/browser/sync/glue/data_type_controller.h
index 3473e09..d39bdcd 100644
--- a/chrome/browser/sync/glue/data_type_controller.h
+++ b/chrome/browser/sync/glue/data_type_controller.h
@@ -65,6 +65,10 @@ class DataTypeController
                    scoped_refptr<DataTypeController> > TypeMap;
   typedef std::map<syncable::ModelType, DataTypeController::State> StateMap;
 
+  // Returns true if the start result should trigger an unrecoverable error.
+  // Public so unit tests can use this function as well.
+  static bool IsUnrecoverableResult(StartResult result);
+
   // Begins asynchronous start up of this data type.  Start up will
   // wait for all other dependent services to be available, then
   // proceed with model association and then change processor
@@ -93,12 +97,14 @@ class DataTypeController
   // Current state of the data type controller.
   virtual State state() const = 0;
 
-  virtual void OnSingleDatatypeUnrecoverableError(
+ protected:
+  // Handles the reporting of unrecoverable error. It records stuff in
+  // UMA and reports to breakpad.
+  // Virtual for testing purpose.
+  virtual void RecordUnrecoverableError(
       const tracked_objects::Location& from_here,
-      const std::string& message) = 0;
+      const std::string& message);
 
-
- protected:
   friend struct content::BrowserThread::DeleteOnThread<
       content::BrowserThread::UI>;
   friend class base::DeleteHelper<DataTypeController>;
diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc
index 7a3a7d5..390a09c 100644
--- a/chrome/browser/sync/glue/data_type_manager_impl.cc
+++ b/chrome/browser/sync/glue/data_type_manager_impl.cc
@@ -16,6 +16,7 @@
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
+#include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h"
 #include "chrome/browser/sync/glue/data_type_controller.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/browser_thread.h"
@@ -300,6 +301,7 @@ void DataTypeManagerImpl::DownloadReady(
   }
 
   if (!failed_configuration_types.Empty()) {
+    ChromeReportUnrecoverableError();
     std::string error_msg =
         "Configuration failed for types " +
         syncable::ModelTypeSetToString(failed_configuration_types);
diff --git a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc
index 90ac5ea..a136e48 100644
--- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc
+++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc
@@ -169,6 +169,11 @@ class FakeDataTypeController : public DataTypeController {
     ADD_FAILURE() << message;
  }
 
+  virtual void RecordUnrecoverableError(
+      const tracked_objects::Location& from_here,
+      const std::string& message) {
+    ADD_FAILURE() << message;
+ }
 
  private:
   State state_;
diff --git a/chrome/browser/sync/glue/frontend_data_type_controller.cc b/chrome/browser/sync/glue/frontend_data_type_controller.cc
index 7143384..69a0fac 100644
--- a/chrome/browser/sync/glue/frontend_data_type_controller.cc
+++ b/chrome/browser/sync/glue/frontend_data_type_controller.cc
@@ -8,6 +8,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/api/sync_error.h"
 #include "chrome/browser/sync/glue/change_processor.h"
+#include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h"
 #include "chrome/browser/sync/glue/model_associator.h"
 #include "chrome/browser/sync/profile_sync_components_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -121,6 +122,9 @@ bool FrontendDataTypeController::Associate() {
 void FrontendDataTypeController::StartFailed(StartResult result,
                                              const SyncError& error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (IsUnrecoverableResult(result))
+    RecordUnrecoverableError(FROM_HERE, "StartFailed");
   CleanUpState();
   set_model_associator(NULL);
   change_processor_.reset();
@@ -215,14 +219,6 @@ void FrontendDataTypeController::OnSingleDatatypeUnrecoverableError(
   sync_service_->OnDisableDatatype(type(), from_here, message);
 }
 
-void FrontendDataTypeController::RecordUnrecoverableError(
-    const tracked_objects::Location& from_here,
-    const std::string& message) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures", type(),
-                            syncable::MODEL_TYPE_COUNT);
-}
-
 void FrontendDataTypeController::RecordAssociationTime(base::TimeDelta time) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 #define PER_DATA_TYPE_MACRO(type_str) \
diff --git a/chrome/browser/sync/glue/frontend_data_type_controller.h b/chrome/browser/sync/glue/frontend_data_type_controller.h
index bec9aeb..8b46978 100644
--- a/chrome/browser/sync/glue/frontend_data_type_controller.h
+++ b/chrome/browser/sync/glue/frontend_data_type_controller.h
@@ -88,11 +88,6 @@ class FrontendDataTypeController : public DataTypeController {
   virtual void StartFailed(StartResult result, const SyncError& error);
   virtual void FinishStart(StartResult result);
 
-  // DataType specific histogram methods.
-  // Record unrecoverable errors.
-  virtual void RecordUnrecoverableError(
-      const tracked_objects::Location& from_here,
-      const std::string& message);
   // Record association time.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure.
diff --git a/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc
index d791bb0..7706b40 100644
--- a/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc
@@ -126,6 +126,8 @@ class SyncFrontendDataTypeControllerTest : public testing::Test {
   }
 
   void SetStartFailExpectations(DataTypeController::StartResult result) {
+    if (DataTypeController::IsUnrecoverableResult(result))
+      EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _));
     EXPECT_CALL(*dtc_mock_, CleanUpState());
     EXPECT_CALL(*dtc_mock_, RecordStartFailure(result));
     EXPECT_CALL(start_callback_, Run(result,_));
diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc
index a143c90..01db139 100644
--- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc
@@ -224,6 +224,8 @@ class SyncNewNonFrontendDataTypeControllerTest : public testing::Test {
   void SetStartFailExpectations(DataTypeController::StartResult result) {
     EXPECT_CALL(syncable_service_, StopSyncing(_));
     EXPECT_CALL(*dtc_mock_, StopModels()).Times(AtLeast(1));
+    if (DataTypeController::IsUnrecoverableResult(result))
+      EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _));
     EXPECT_CALL(*dtc_mock_, RecordStartFailure(result));
     EXPECT_CALL(start_callback_, Run(result,_));
   }
@@ -381,6 +383,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, AbortDuringAssociation) {
           Return(true)));
   EXPECT_CALL(*change_processor_, GetSyncData(_)).
       WillOnce(Return(SyncError(FROM_HERE, "Disconnected.", AUTOFILL_PROFILE)));
+  EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _));
   EXPECT_CALL(*change_processor_, Disconnect()).
       WillOnce(DoAll(SignalEvent(&pause_db_thread), Return(true)));
   EXPECT_CALL(service_, DeactivateDataType(_));
@@ -420,6 +423,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartAfterSyncShutdown) {
   EXPECT_CALL(*change_processor_, Connect(_,_,_,_)).
       WillOnce(Return(base::WeakPtr<SyncableService>()));
   new_non_frontend_dtc_->UnblockBackendTasks();
+  EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _));
   WaitForDTC();
 }
 
diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc
index cc72147..afd9e98 100644
--- a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc
+++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc
@@ -10,6 +10,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/api/sync_error.h"
 #include "chrome/browser/sync/glue/change_processor.h"
+#include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h"
 #include "chrome/browser/sync/glue/model_associator.h"
 #include "chrome/browser/sync/profile_sync_components_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -128,6 +129,9 @@ void NonFrontendDataTypeController::StartAssociation() {
 void NonFrontendDataTypeController::StartFailed(StartResult result,
                                                 const SyncError& error) {
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (IsUnrecoverableResult(result))
+    RecordUnrecoverableError(FROM_HERE, "StartFailed");
   StopAssociation();
   StartDone(result,
             result == ASSOCIATION_FAILED ? DISABLED : NOT_RUNNING,
@@ -316,14 +320,6 @@ void NonFrontendDataTypeController::DisableImpl(
   profile_sync_service_->OnDisableDatatype(type(), from_here, message);
 }
 
-void NonFrontendDataTypeController::RecordUnrecoverableError(
-    const tracked_objects::Location& from_here,
-    const std::string& message) {
-  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
-  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures", type(),
-                            syncable::MODEL_TYPE_COUNT);
-}
-
 void NonFrontendDataTypeController::RecordAssociationTime(
     base::TimeDelta time) {
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.h b/chrome/browser/sync/glue/non_frontend_data_type_controller.h
index 28acfd1..392e275 100644
--- a/chrome/browser/sync/glue/non_frontend_data_type_controller.h
+++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.h
@@ -116,12 +116,6 @@ class NonFrontendDataTypeController : public DataTypeController {
   virtual void DisableImpl(const tracked_objects::Location& from_here,
                            const std::string& message);
 
-  // DataType specific histogram methods.
-  // Important: calling them on other threads can lead to memory corruption!
-  // Record unrecoverable errors. Called on Datatype's thread.
-  virtual void RecordUnrecoverableError(
-      const tracked_objects::Location& from_here,
-      const std::string& message);
   // Record association time. Called on Datatype's thread.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure. Called on UI thread.
diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc
index da5410a..e60a4bc 100644
--- a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc
@@ -162,6 +162,8 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test {
 
   void SetStartFailExpectations(DataTypeController::StartResult result) {
     EXPECT_CALL(*dtc_mock_, StopModels());
+    if (DataTypeController::IsUnrecoverableResult(result))
+      EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _));
     EXPECT_CALL(*dtc_mock_, RecordStartFailure(result));
     EXPECT_CALL(start_callback_, Run(result,_));
   }
diff --git a/chrome/browser/sync/glue/ui_data_type_controller.cc b/chrome/browser/sync/glue/ui_data_type_controller.cc
index de5ef9e..d48fdda 100644
--- a/chrome/browser/sync/glue/ui_data_type_controller.cc
+++ b/chrome/browser/sync/glue/ui_data_type_controller.cc
@@ -143,6 +143,8 @@ void UIDataTypeController::Associate() {
 void UIDataTypeController::StartFailed(StartResult result,
                                        const SyncError& error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (IsUnrecoverableResult(result))
+    RecordUnrecoverableError(FROM_HERE, "StartFailed");
   StopModels();
   if (result == ASSOCIATION_FAILED) {
     state_ = DISABLED;
@@ -237,14 +239,6 @@ void UIDataTypeController::OnSingleDatatypeUnrecoverableError(
   sync_service_->OnDisableDatatype(type(), from_here, message);
 }
 
-void UIDataTypeController::RecordUnrecoverableError(
-    const tracked_objects::Location& from_here,
-    const std::string& message) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures", type(),
-                            syncable::MODEL_TYPE_COUNT);
-}
-
 void UIDataTypeController::RecordAssociationTime(base::TimeDelta time) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 #define PER_DATA_TYPE_MACRO(type_str) \
diff --git a/chrome/browser/sync/glue/ui_data_type_controller.h b/chrome/browser/sync/glue/ui_data_type_controller.h
index cb60cfe..857e50f 100644
--- a/chrome/browser/sync/glue/ui_data_type_controller.h
+++ b/chrome/browser/sync/glue/ui_data_type_controller.h
@@ -75,11 +75,6 @@ class UIDataTypeController : public DataTypeController {
   virtual void StartFailed(StartResult result, const SyncError& error);
   virtual void StartDone(StartResult result);
 
-  // Datatype specific histogram methods.
-  // Record unrecoverable errors.
-  virtual void RecordUnrecoverableError(
-      const tracked_objects::Location& from_here,
-      const std::string& message);
   // Record association time.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure.