[Sync] fix about:sync crash with unrecoverable error

If the client had encountered an unrecoverable error typing about:sync would crash because the client code would try to dereference the backendhost for the purpose of printing the model group information(by this time backendhost would have been released). The fix is to bail out of printing the model information if the client had encountered an unrecoverable error. however we would still print the client had encountered an urecoverable error and the line number etc.

In the process moved a bunch of code related to collecting about information for sync, from browser_about_handler.cc to sync_ui_util.cc. The refactoing helps us in unit testing by passing in a mock service object.
BUG=55503
TEST=Wrote a new unit test for it. ConstructAboutInformationWithUnrecoverableErrorTest. Also tested by pretending one of the datatypes generated an unrecoverable error in the debugger.

Original patch by lipalani@google.com
Original review: http://codereview.chromium.org/3715002

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

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

6 files changed, 196 insertions, 98 deletions

diff --git a/chrome/browser/browser_about_handler.cc b/chrome/browser/browser_about_handler.cc
index a93dd91c..38f27a6 100644
--- a/chrome/browser/browser_about_handler.cc
+++ b/chrome/browser/browser_about_handler.cc
@@ -42,6 +42,7 @@
 #include "chrome/browser/renderer_host/render_process_host.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/sync_ui_util.h"
 #include "chrome/common/about_handler.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_version_info.h"
@@ -675,38 +676,7 @@ std::string AboutVersion(DictionaryValue* localized_strings) {
       version_html, localized_strings, "t" /* template root node id */);
 }
 
-static void AddBoolSyncDetail(ListValue* details, const std::string& stat_name,
-                              bool stat_value) {
-  DictionaryValue* val = new DictionaryValue;
-  val->SetString("stat_name", stat_name);
-  val->SetBoolean("stat_value", stat_value);
-  details->Append(val);
-}
-
-static void AddIntSyncDetail(ListValue* details, const std::string& stat_name,
-                             int64 stat_value) {
-  DictionaryValue* val = new DictionaryValue;
-  val->SetString("stat_name", stat_name);
-  val->SetString("stat_value", base::FormatNumber(stat_value));
-  details->Append(val);
-}
 
-static std::string MakeSyncAuthErrorText(
-    const GoogleServiceAuthError::State& state) {
-  switch (state) {
-    case GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS:
-    case GoogleServiceAuthError::ACCOUNT_DELETED:
-    case GoogleServiceAuthError::ACCOUNT_DISABLED:
-    case GoogleServiceAuthError::SERVICE_UNAVAILABLE:
-      return "INVALID_GAIA_CREDENTIALS";
-    case GoogleServiceAuthError::USER_NOT_SIGNED_UP:
-      return "USER_NOT_SIGNED_UP";
-    case GoogleServiceAuthError::CONNECTION_FAILED:
-      return "CONNECTION_FAILED";
-    default:
-      return std::string();
-  }
-}
 
 std::string AboutSync() {
   FilePath user_data_dir;
@@ -717,71 +687,10 @@ std::string AboutSync() {
   ProfileSyncService* service = profile->GetProfileSyncService();
 
   DictionaryValue strings;
-  if (!service || !service->HasSyncSetupCompleted()) {
+  if (!service) {
     strings.SetString("summary", "SYNC DISABLED");
   } else {
-    SyncManager::Status full_status(service->QueryDetailedSyncStatus());
-
-    strings.SetString("service_url", service->sync_service_url().spec());
-    strings.SetString("summary",
-        ProfileSyncService::BuildSyncStatusSummaryText(
-            full_status.summary));
-
-    strings.Set("authenticated",
-        new FundamentalValue(full_status.authenticated));
-    strings.SetString("auth_problem",
-        MakeSyncAuthErrorText(service->GetAuthError().state()));
-
-    strings.SetString("time_since_sync", service->GetLastSyncedTimeString());
-
-    ListValue* details = new ListValue();
-    strings.Set("details", details);
-    AddBoolSyncDetail(details, "Server Up", full_status.server_up);
-    AddBoolSyncDetail(details, "Server Reachable",
-                      full_status.server_reachable);
-    AddBoolSyncDetail(details, "Server Broken", full_status.server_broken);
-    AddBoolSyncDetail(details, "Notifications Enabled",
-                      full_status.notifications_enabled);
-    AddIntSyncDetail(details, "Notifications Received",
-                     full_status.notifications_received);
-    AddIntSyncDetail(details, "Notifications Sent",
-                     full_status.notifications_sent);
-    AddIntSyncDetail(details, "Unsynced Count", full_status.unsynced_count);
-    AddIntSyncDetail(details, "Conflicting Count",
-                     full_status.conflicting_count);
-    AddBoolSyncDetail(details, "Syncing", full_status.syncing);
-    AddBoolSyncDetail(details, "Initial Sync Ended",
-                      full_status.initial_sync_ended);
-    AddBoolSyncDetail(details, "Syncer Stuck", full_status.syncer_stuck);
-    AddIntSyncDetail(details, "Updates Available",
-                     full_status.updates_available);
-    AddIntSyncDetail(details, "Updates Received", full_status.updates_received);
-    AddBoolSyncDetail(details, "Disk Full", full_status.disk_full);
-    AddBoolSyncDetail(details, "Invalid Store", full_status.invalid_store);
-    AddIntSyncDetail(details, "Max Consecutive Errors",
-                     full_status.max_consecutive_errors);
-
-    if (service->unrecoverable_error_detected()) {
-      strings.Set("unrecoverable_error_detected", new FundamentalValue(true));
-      strings.SetString("unrecoverable_error_message",
-                        service->unrecoverable_error_message());
-      tracked_objects::Location loc(service->unrecoverable_error_location());
-      std::string location_str;
-      loc.Write(true, true, &location_str);
-      strings.SetString("unrecoverable_error_location", location_str);
-    }
-
-    browser_sync::ModelSafeRoutingInfo routes;
-    service->backend()->GetModelSafeRoutingInfo(&routes);
-    ListValue* routing_info = new ListValue();
-    strings.Set("routing_info", routing_info);
-    browser_sync::ModelSafeRoutingInfo::const_iterator it = routes.begin();
-    for (; it != routes.end(); ++it) {
-      DictionaryValue* val = new DictionaryValue;
-      val->SetString("model_type", ModelTypeToString(it->first));
-      val->SetString("group", ModelSafeGroupToString(it->second));
-      routing_info->Append(val);
-    }
+    sync_ui_util::ConstructAboutInformation(service, &strings);
   }
 
   static const base::StringPiece sync_html(
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index d5514040..01b4f5b 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -205,7 +205,7 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
 
   // Get various information for displaying in the user interface.
   browser_sync::SyncBackendHost::StatusSummary QuerySyncStatusSummary();
-  browser_sync::SyncBackendHost::Status QueryDetailedSyncStatus();
+  virtual browser_sync::SyncBackendHost::Status QueryDetailedSyncStatus();
 
   const GoogleServiceAuthError& GetAuthError() const {
     return last_auth_error_;
@@ -233,7 +233,7 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   // TODO(timsteele): What happens if the bookmark model is loaded, a change
   // takes place, and the backend isn't initialized yet?
   bool sync_initialized() const { return backend_initialized_; }
-  bool unrecoverable_error_detected() const {
+  virtual bool unrecoverable_error_detected() const {
     return unrecoverable_error_detected_;
   }
   const std::string& unrecoverable_error_message() {
@@ -256,7 +256,7 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   const base::Time& last_synced_time() const { return last_synced_time_; }
 
   // Returns a user-friendly string form of last synced time (in minutes).
-  string16 GetLastSyncedTimeString() const;
+  virtual string16 GetLastSyncedTimeString() const;
 
   // Returns the authenticated username of the sync user, or empty if none
   // exists. It will only exist if the authentication service provider (e.g
diff --git a/chrome/browser/sync/profile_sync_service_mock.h b/chrome/browser/sync/profile_sync_service_mock.h
index 3142baf..dea5482 100644
--- a/chrome/browser/sync/profile_sync_service_mock.h
+++ b/chrome/browser/sync/profile_sync_service_mock.h
@@ -51,6 +51,11 @@ class ProfileSyncServiceMock : public ProfileSyncService {
                      void(syncable::ModelTypeSet* preferred_types));
   MOCK_CONST_METHOD1(GetRegisteredDataTypes,
                      void(syncable::ModelTypeSet* registered_types));
+
+  MOCK_METHOD0(QueryDetailedSyncStatus,
+               browser_sync::SyncBackendHost::Status());
+  MOCK_CONST_METHOD0(GetLastSyncedTimeString, string16());
+  MOCK_CONST_METHOD0(unrecoverable_error_detected, bool());
 };
 
 #endif  // CHROME_BROWSER_SYNC_PROFILE_SYNC_SERVICE_MOCK_H_
diff --git a/chrome/browser/sync/sync_ui_util.cc b/chrome/browser/sync/sync_ui_util.cc
index 914f918..684db84 100644
--- a/chrome/browser/sync/sync_ui_util.cc
+++ b/chrome/browser/sync/sync_ui_util.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/sync/sync_ui_util.h"
 
 #include "app/l10n_util.h"
+#include "base/i18n/number_formatting.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -196,4 +197,133 @@ void OpenSyncMyBookmarksDialog(
   }
 }
 
+void AddBoolSyncDetail(ListValue* details,
+                       const std::string& stat_name,
+                       bool stat_value) {
+  DictionaryValue* val = new DictionaryValue;
+  val->SetString("stat_name", stat_name);
+  val->SetBoolean("stat_value", stat_value);
+  details->Append(val);
+}
+
+void AddIntSyncDetail(ListValue* details, const std::string& stat_name,
+                      int64 stat_value) {
+  DictionaryValue* val = new DictionaryValue;
+  val->SetString("stat_name", stat_name);
+  val->SetString("stat_value", base::FormatNumber(stat_value));
+  details->Append(val);
+}
+
+std::string MakeSyncAuthErrorText(
+    const GoogleServiceAuthError::State& state) {
+  switch (state) {
+    case GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS:
+    case GoogleServiceAuthError::ACCOUNT_DELETED:
+    case GoogleServiceAuthError::ACCOUNT_DISABLED:
+    case GoogleServiceAuthError::SERVICE_UNAVAILABLE:
+      return "INVALID_GAIA_CREDENTIALS";
+    case GoogleServiceAuthError::USER_NOT_SIGNED_UP:
+      return "USER_NOT_SIGNED_UP";
+    case GoogleServiceAuthError::CONNECTION_FAILED:
+      return "CONNECTION_FAILED";
+    default:
+      return std::string();
+  }
+}
+
+void ConstructAboutInformation(ProfileSyncService* service,
+                               DictionaryValue* strings) {
+  CHECK(strings != NULL);
+  if (!service->HasSyncSetupCompleted()) {
+    strings->SetString("summary", "SYNC DISABLED");
+  } else {
+    sync_api::SyncManager::Status full_status(
+        service->QueryDetailedSyncStatus());
+
+    strings->SetString("service_url", service->sync_service_url().spec());
+    strings->SetString("summary",
+                       ProfileSyncService::BuildSyncStatusSummaryText(
+                       full_status.summary));
+
+    strings->Set("authenticated",
+                 new FundamentalValue(full_status.authenticated));
+    strings->SetString("auth_problem",
+                       sync_ui_util::MakeSyncAuthErrorText(
+                       service->GetAuthError().state()));
+
+    strings->SetString("time_since_sync", service->GetLastSyncedTimeString());
+
+    ListValue* details = new ListValue();
+    strings->Set("details", details);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Server Up",
+                                    full_status.server_up);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Server Reachable",
+                                    full_status.server_reachable);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Server Broken",
+                                    full_status.server_broken);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Notifications Enabled",
+                                    full_status.notifications_enabled);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Notifications Received",
+                                   full_status.notifications_received);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Notifications Sent",
+                                   full_status.notifications_sent);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Unsynced Count",
+                                   full_status.unsynced_count);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Conflicting Count",
+                                   full_status.conflicting_count);
+    sync_ui_util::AddBoolSyncDetail(details, "Syncing", full_status.syncing);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Initial Sync Ended",
+                                    full_status.initial_sync_ended);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Syncer Stuck",
+                                    full_status.syncer_stuck);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Updates Available",
+                                   full_status.updates_available);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Updates Received",
+                                   full_status.updates_received);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Disk Full",
+                                    full_status.disk_full);
+    sync_ui_util::AddBoolSyncDetail(details,
+                                    "Invalid Store",
+                                    full_status.invalid_store);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Max Consecutive Errors",
+                                   full_status.max_consecutive_errors);
+
+    if (service->unrecoverable_error_detected()) {
+      strings->Set("unrecoverable_error_detected", new FundamentalValue(true));
+      strings->SetString("unrecoverable_error_message",
+                         service->unrecoverable_error_message());
+      tracked_objects::Location loc(service->unrecoverable_error_location());
+      std::string location_str;
+      loc.Write(true, true, &location_str);
+      strings->SetString("unrecoverable_error_location", location_str);
+    } else {
+      browser_sync::ModelSafeRoutingInfo routes;
+      service->backend()->GetModelSafeRoutingInfo(&routes);
+      ListValue* routing_info = new ListValue();
+      strings->Set("routing_info", routing_info);
+      browser_sync::ModelSafeRoutingInfo::const_iterator it = routes.begin();
+      for (; it != routes.end(); ++it) {
+        DictionaryValue* val = new DictionaryValue;
+        val->SetString("model_type", ModelTypeToString(it->first));
+        val->SetString("group", ModelSafeGroupToString(it->second));
+        routing_info->Append(val);
+      }
+    }
+  }
+}
+
 }  // namespace sync_ui_util
diff --git a/chrome/browser/sync/sync_ui_util.h b/chrome/browser/sync/sync_ui_util.h
index ce370af..8bcd85a 100644
--- a/chrome/browser/sync/sync_ui_util.h
+++ b/chrome/browser/sync/sync_ui_util.h
@@ -6,10 +6,15 @@
 #define CHROME_BROWSER_SYNC_SYNC_UI_UTIL_H_
 #pragma once
 
+#include <string>
+
 #include "base/string16.h"
+#include "base/values.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 
 class Profile;
+class ListValue;
+class DictionaryValue;
 
 // Utility functions to gather current sync status information from the sync
 // service and constructs messages suitable for showing in UI.
@@ -42,7 +47,17 @@ string16 GetSyncMenuLabel(ProfileSyncService* service);
 // incognito).  |code| should be one of the START_FROM_* codes.
 void OpenSyncMyBookmarksDialog(
     Profile* profile, ProfileSyncService::SyncEventCodes code);
-}  // namespace sync_ui_util
 
+void AddBoolSyncDetail(ListValue* details,
+                       const std::string& stat_name,
+                       bool stat_value);
+
+void ConstructAboutInformation(ProfileSyncService* service,
+                               DictionaryValue* strings);
+
+void AddIntSyncDetail(ListValue* details,
+                      const std::string& stat_name,
+                      int64 stat_value);
+}  // namespace sync_ui_util
 #endif  // CHROME_BROWSER_SYNC_SYNC_UI_UTIL_H_
 
diff --git a/chrome/browser/sync/sync_ui_util_unittest.cc b/chrome/browser/sync/sync_ui_util_unittest.cc
new file mode 100644
index 0000000..7122170
--- /dev/null
+++ b/chrome/browser/sync/sync_ui_util_unittest.cc
@@ -0,0 +1,39 @@
+// Copyright (c) 2010 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 "base/basictypes.h"
+#include "chrome/browser/sync/sync_ui_util.h"
+#include "chrome/browser/sync/profile_sync_service_mock.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gmock/include/gmock/gmock-actions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::Return;
+using ::testing::NiceMock;
+TEST(SyncUIUtilTest, ConstructAboutInformationWithUnrecoverableErrorTest) {
+  NiceMock<ProfileSyncServiceMock> service;
+  DictionaryValue strings;
+
+  // Will be released when the dictionary is destroyed
+  string16 str(ASCIIToUTF16("none"));
+
+  browser_sync::SyncBackendHost::Status status =
+      { browser_sync::SyncBackendHost::Status::OFFLINE_UNUSABLE };
+
+  EXPECT_CALL(service, HasSyncSetupCompleted())
+              .WillOnce(Return(true));
+  EXPECT_CALL(service, QueryDetailedSyncStatus())
+              .WillOnce(Return(status));
+
+  EXPECT_CALL(service, unrecoverable_error_detected())
+             .WillOnce(Return(true));
+
+  EXPECT_CALL(service, GetLastSyncedTimeString())
+             .WillOnce(Return(str));
+
+  sync_ui_util::ConstructAboutInformation(&service, &strings);
+
+  EXPECT_TRUE(strings.HasKey("unrecoverable_error_detected"));
+}
+