Refactor about_sync_util.cc to remove unsafe usage of raw pointers

The previous [Bool|Int|String]SyncStat classes relied on raw pointers
into base::ListValue, which is bad because they become invalid when
the actual std::vector backing the list gets resized. They're replaced
by new SectionList/Section/Stat classes that are less fragile.
While we're here, this also updates the code to use the new base::Value
APIs, i.e. using values instead of unique_ptrs.

Bug: 702230
Change-Id: I5da50857587acb50ffb8849a7fa2cf8dabce9e45
Reviewed-on: https://chromium-review.googlesource.com/955324
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543384}

components/sync/driver/about_sync_util.cc

@@ -6,12 +6,11 @@
 
 #include <string>
 #include <utility>
+#include <vector>
 
-#include "base/location.h"
 #include "base/logging.h"
-#include "base/memory/ptr_util.h"
-#include "base/strings/string16.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/strings/grit/components_strings.h"
@@ -24,9 +23,6 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/time_format.h"
 
-using base::DictionaryValue;
-using base::ListValue;
-
 namespace syncer {
 
 namespace sync_ui_util {
@@ -77,156 +73,112 @@ const char kUpdate[] = "update";
 
 namespace {
 
-// Creates a 'section' for display on about:sync, consisting of a title and a
-// list of fields.  Returns a pointer to the new section.  Note that
-// |parent_list|, not the caller, owns the newly added section.
-base::ListValue* AddSection(base::ListValue* parent_list,
-                            const std::string& title) {
-  auto section = std::make_unique<base::DictionaryValue>();
-  section->SetString("title", title);
-  base::ListValue* section_contents =
-      section->SetList("data", std::make_unique<base::ListValue>());
-  section->SetBoolean("is_sensitive", false);
-  // If the following |Append| results in a reallocation, pointers to the
-  // members of |parent_list| will be invalidated. This would result in
-  // use-after-free in |*SyncStat::SetValue|. This is why the following DCHECK
-  // is necessary to ensure no reallocation takes place.
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  DCHECK_LT(parent_list->GetSize(), parent_list->GetList().capacity());
-  parent_list->Append(std::move(section));
-  return section_contents;
-}
+const char kUninitialized[] = "Uninitialized";
 
-// Same as AddSection, but for data that should be elided when dumped into text
-// form and posted in a public forum (e.g. unique identifiers).
-base::ListValue* AddSensitiveSection(base::ListValue* parent_list,
-                                     const std::string& title) {
-  auto section = std::make_unique<base::DictionaryValue>();
-  section->SetString("title", title);
-  base::ListValue* section_contents =
-      section->SetList("data", std::make_unique<base::ListValue>());
-  section->SetBoolean("is_sensitive", true);
-  // If the following |Append| results in a reallocation, pointers to
-  // |parent_list| and its members will be invalidated. This would result in
-  // use-after-free in |*SyncStat::SetValue|. This is why the following DCHECK
-  // is necessary to ensure no reallocation takes place.
-  DCHECK_LT(parent_list->GetSize(), parent_list->GetList().capacity());
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  parent_list->Append(std::move(section));
-  return section_contents;
-}
+// This class represents one field in about:sync. It gets serialized into a
+// dictionary with entries for 'stat_name', 'stat_value' and 'is_valid'.
+class StatBase {
+ public:
+  base::Value ToValue() const {
+    base::Value result(base::Value::Type::DICTIONARY);
+    result.SetKey("stat_name", base::Value(key_));
+    result.SetKey("stat_value", value_.Clone());
+    result.SetKey("is_valid", base::Value(is_valid_));
+    return result;
+  }
 
-// The following helper classes help manage the about:sync fields which will be
-// populated in method in ConstructAboutInformation.
-//
-// Each instance of one of thse classes indicates a field in about:sync.  Each
-// field will be serialized to a DictionaryValue with entries for 'stat_name',
-// 'stat_value' and 'is_valid'.
+ protected:
+  StatBase(const std::string& key, base::Value default_value)
+      : key_(key), value_(std::move(default_value)) {}
 
-class StringSyncStat {
- public:
-  StringSyncStat(base::ListValue* section, const std::string& key);
-  void SetValue(const std::string& value);
-  void SetValue(const base::string16& value);
+  void SetFromValue(base::Value value) {
+    value_ = std::move(value);
+    is_valid_ = true;
+  }
 
  private:
-  // Owned by the |section| passed in during construction.
-  base::DictionaryValue* stat_;
+  std::string key_;
+  base::Value value_;
+  bool is_valid_ = false;
 };
 
-StringSyncStat::StringSyncStat(base::ListValue* section,
-                               const std::string& key) {
-  stat_ = new base::DictionaryValue();
-  stat_->SetString("stat_name", key);
-  stat_->SetString("stat_value", "Uninitialized");
-  stat_->SetBoolean("is_valid", false);
-  // |stat_| will be invalidated by |Append|, so it needs to be reset.
-  // Furthermore, if |Append| results in a reallocation, |stat_| members of
-  // other SyncStats will be invalidated. This is why the following dcheck is
-  // necessary, so that it is guaranteed that a reallocation will not happen.
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  DCHECK_LT(section->GetSize(), section->GetList().capacity());
-  section->Append(base::WrapUnique(stat_));
-  section->GetDictionary(section->GetSize() - 1, &stat_);
-}
-
-void StringSyncStat::SetValue(const std::string& value) {
-  stat_->SetString("stat_value", value);
-  stat_->SetBoolean("is_valid", true);
-}
+template <typename T>
+class Stat : public StatBase {
+ public:
+  Stat(const std::string& key, const T& default_value)
+      : StatBase(key, base::Value(default_value)) {}
 
-void StringSyncStat::SetValue(const base::string16& value) {
-  stat_->SetString("stat_value", value);
-  stat_->SetBoolean("is_valid", true);
-}
+  void Set(const T& value) { SetFromValue(base::Value(value)); }
+};
 
-class BoolSyncStat {
+// A section for display on about:sync, consisting of a title and a list of
+// fields.
+class Section {
  public:
-  BoolSyncStat(base::ListValue* section, const std::string& key);
-  void SetValue(bool value);
+  explicit Section(const std::string& title) : title_(title) {}
+
+  void MarkSensitive() { is_sensitive_ = true; }
+
+  Stat<bool>* AddBoolStat(const std::string& key) {
+    return AddStat(key, false);
+  }
+  Stat<int>* AddIntStat(const std::string& key) { return AddStat(key, 0); }
+  Stat<std::string>* AddStringStat(const std::string& key) {
+    return AddStat(key, std::string(kUninitialized));
+  }
+
+  base::Value ToValue() const {
+    base::Value result(base::Value::Type::DICTIONARY);
+    result.SetKey("title", base::Value(title_));
+    base::Value stats(base::Value::Type::LIST);
+    for (const std::unique_ptr<StatBase>& stat : stats_)
+      stats.GetList().push_back(stat->ToValue());
+    result.SetKey("data", std::move(stats));
+    result.SetKey("is_sensitive", base::Value(is_sensitive_));
+    return result;
+  }
 
  private:
-  // Owned by the |section| passed in during construction.
-  base::DictionaryValue* stat_;
+  template <typename T>
+  Stat<T>* AddStat(const std::string& key, const T& default_value) {
+    auto stat = std::make_unique<Stat<T>>(key, default_value);
+    Stat<T>* result = stat.get();
+    stats_.push_back(std::move(stat));
+    return result;
+  }
+
+  std::string title_;
+  std::vector<std::unique_ptr<StatBase>> stats_;
+  bool is_sensitive_ = false;
 };
 
-BoolSyncStat::BoolSyncStat(base::ListValue* section, const std::string& key) {
-  stat_ = new base::DictionaryValue();
-  stat_->SetString("stat_name", key);
-  stat_->SetBoolean("stat_value", false);
-  stat_->SetBoolean("is_valid", false);
-  // |stat_| will be invalidated by |Append|, so it needs to be reset.
-  // Furthermore, if |Append| results in a reallocation, |stat_| members of
-  // other SyncStats will be invalidated. This is why the following dcheck is
-  // necessary, so that it is guaranteed that a reallocation will not happen.
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  DCHECK_LT(section->GetSize(), section->GetList().capacity());
-  section->Append(base::WrapUnique(stat_));
-  section->GetDictionary(section->GetSize() - 1, &stat_);
-}
+class SectionList {
+ public:
+  SectionList() = default;
 
-void BoolSyncStat::SetValue(bool value) {
-  stat_->SetBoolean("stat_value", value);
-  stat_->SetBoolean("is_valid", true);
-}
+  Section* AddSection(const std::string& title) {
+    sections_.push_back(std::make_unique<Section>(title));
+    return sections_.back().get();
+  }
 
-class IntSyncStat {
- public:
-  IntSyncStat(base::ListValue* section, const std::string& key);
-  void SetValue(int value);
+  base::Value ToValue() const {
+    base::Value result(base::Value::Type::LIST);
+    for (const std::unique_ptr<Section>& section : sections_)
+      result.GetList().push_back(section->ToValue());
+    return result;
+  }
 
  private:
-  // Owned by the |section| passed in during construction.
-  base::DictionaryValue* stat_;
+  std::vector<std::unique_ptr<Section>> sections_;
 };
 
-IntSyncStat::IntSyncStat(base::ListValue* section, const std::string& key) {
-  stat_ = new base::DictionaryValue();
-  stat_->SetString("stat_name", key);
-  stat_->SetInteger("stat_value", 0);
-  stat_->SetBoolean("is_valid", false);
-  // |stat_| will be invalidated by |Append|, so it needs to be reset.
-  // Furthermore, if |Append| results in a reallocation, |stat_| members of
-  // other SyncStats will be invalidated. This is why the following dcheck is
-  // necessary, so that it is guaranteed that a reallocation will not happen.
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  DCHECK_LT(section->GetSize(), section->GetList().capacity());
-  section->Append(base::WrapUnique(stat_));
-  section->GetDictionary(section->GetSize() - 1, &stat_);
-}
-
-void IntSyncStat::SetValue(int value) {
-  stat_->SetInteger("stat_value", value);
-  stat_->SetBoolean("is_valid", true);
-}
-
 // Returns a string describing the chrome version environment. Version format:
 // <Build Info> <OS> <Version number> (<Last change>)<channel or "-devel">
 // If version information is unavailable, returns "invalid."
-// TODO(zea): this approximately matches MakeUserAgentForSyncApi in
-// sync_backend_host.cc. Unify the two if possible.
+// TODO(zea): this approximately matches syncer::MakeUserAgentForSync in
+// sync_util.h. Unify the two if possible.
 std::string GetVersionString(version_info::Channel channel) {
-  // Build a version string that matches MakeUserAgentForSyncApi with the
+  // Build a version string that matches syncer::MakeUserAgentForSync with the
   // addition of channel info and proper OS names.
   // chrome::GetChannelString() returns empty string for stable channel or
   // unofficial builds, the channel string otherwise. We want to have "-devel"
@@ -253,45 +205,39 @@ std::string GetTimeStr(base::Time time, const std::string& default_msg) {
   return time_str;
 }
 
-base::string16 GetLastSyncedTimeString(base::Time last_synced_time) {
+std::string GetLastSyncedTimeString(base::Time last_synced_time) {
   if (last_synced_time.is_null())
-    return l10n_util::GetStringUTF16(IDS_SYNC_TIME_NEVER);
+    return l10n_util::GetStringUTF8(IDS_SYNC_TIME_NEVER);
 
   base::TimeDelta time_since_last_sync = base::Time::Now() - last_synced_time;
 
   if (time_since_last_sync < base::TimeDelta::FromMinutes(1))
-    return l10n_util::GetStringUTF16(IDS_SYNC_TIME_JUST_NOW);
+    return l10n_util::GetStringUTF8(IDS_SYNC_TIME_JUST_NOW);
 
-  return ui::TimeFormat::Simple(ui::TimeFormat::FORMAT_ELAPSED,
-                                ui::TimeFormat::LENGTH_SHORT,
-                                time_since_last_sync);
+  return base::UTF16ToUTF8(ui::TimeFormat::Simple(
+      ui::TimeFormat::FORMAT_ELAPSED, ui::TimeFormat::LENGTH_SHORT,
+      time_since_last_sync));
 }
 
 std::string GetConnectionStatus(const SyncService::SyncTokenStatus& status) {
-  std::string message;
   switch (status.connection_status) {
     case CONNECTION_NOT_ATTEMPTED:
-      base::StringAppendF(&message, "not attempted");
-      break;
+      return "not attempted";
     case CONNECTION_OK:
-      base::StringAppendF(
-          &message, "OK since %s",
+      return base::StringPrintf(
+          "OK since %s",
           GetTimeStr(status.connection_status_update_time, "n/a").c_str());
-      break;
     case CONNECTION_AUTH_ERROR:
-      base::StringAppendF(
-          &message, "auth error since %s",
+      return base::StringPrintf(
+          "auth error since %s",
           GetTimeStr(status.connection_status_update_time, "n/a").c_str());
-      break;
     case CONNECTION_SERVER_ERROR:
-      base::StringAppendF(
-          &message, "server error since %s",
+      return base::StringPrintf(
+          "server error since %s",
           GetTimeStr(status.connection_status_update_time, "n/a").c_str());
-      break;
-    default:
-      NOTREACHED();
   }
-  return message;
+  NOTREACHED();
+  return std::string();
 }
 
 }  // namespace
@@ -316,261 +262,258 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
     version_info::Channel channel) {
   auto about_info = std::make_unique<base::DictionaryValue>();
 
-  // 'details': A list of sections.
-  auto stats_list = std::make_unique<base::ListValue>();
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  stats_list->Reserve(12);
-
-  // The following lines define the sections and their fields.  For each field,
-  // a class is instantiated, which allows us to reference the fields in
-  // 'setter' code later on in this function.
-  base::ListValue* section_summary = AddSection(stats_list.get(), "Summary");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_summary->Reserve(1);
-  StringSyncStat summary_string(section_summary, "Summary");
-
-  base::ListValue* section_version =
-      AddSection(stats_list.get(), "Version Info");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_version->Reserve(2);
-  StringSyncStat client_version(section_version, "Client Version");
-  StringSyncStat server_url(section_version, "Server URL");
-
-  base::ListValue* section_identity =
-      AddSensitiveSection(stats_list.get(), kIdentityTitle);
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_identity->Reserve(3);
-  StringSyncStat sync_id(section_identity, "Sync Client ID");
-  StringSyncStat invalidator_id(section_identity, "Invalidator Client ID");
-  StringSyncStat username(section_identity, "Username");
-
-  base::ListValue* section_credentials =
-      AddSection(stats_list.get(), "Credentials");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_credentials->Reserve(4);
-  StringSyncStat request_token_time(section_credentials, "Requested Token");
-  StringSyncStat receive_token_time(section_credentials, "Received Token");
-  StringSyncStat token_request_status(section_credentials,
-                                      "Token Request Status");
-  StringSyncStat next_token_request(section_credentials, "Next Token Request");
-
-  base::ListValue* section_local = AddSection(stats_list.get(), "Local State");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_local->Reserve(7);
-  StringSyncStat server_connection(section_local, "Server Connection");
-  StringSyncStat last_synced(section_local, "Last Synced");
-  BoolSyncStat is_setup_complete(section_local,
-                                 "Sync First-Time Setup Complete");
-  StringSyncStat backend_initialization(section_local,
-                                        "Sync Backend Initialization");
-  BoolSyncStat is_syncing(section_local, "Syncing");
-  BoolSyncStat is_local_sync_enabled(section_local,
-                                     "Local Sync Backend Enabled");
-  StringSyncStat local_backend_path(section_local, "Local Backend Path");
-
-  base::ListValue* section_network = AddSection(stats_list.get(), "Network");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_network->Reserve(3);
-  BoolSyncStat is_any_throttled_or_backoff(section_network,
-                                           "Throttled or Backoff");
-  StringSyncStat retry_time(section_network, "Retry Time");
-  BoolSyncStat are_notifications_enabled(section_network,
-                                         "Notifications Enabled");
-
-  base::ListValue* section_encryption =
-      AddSection(stats_list.get(), "Encryption");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_encryption->Reserve(9);
-  BoolSyncStat is_using_explicit_passphrase(section_encryption,
-                                            "Explicit Passphrase");
-  BoolSyncStat is_passphrase_required(section_encryption,
-                                      "Passphrase Required");
-  BoolSyncStat is_cryptographer_ready(section_encryption,
-                                      "Cryptographer Ready");
-  BoolSyncStat has_pending_keys(section_encryption,
-                                "Cryptographer Has Pending Keys");
-  StringSyncStat encrypted_types(section_encryption, "Encrypted Types");
-  BoolSyncStat has_keystore_key(section_encryption, "Has Keystore Key");
-  StringSyncStat keystore_migration_time(section_encryption,
-                                         "Keystore Migration Time");
-  StringSyncStat passphrase_type(section_encryption, "Passphrase Type");
-  StringSyncStat passphrase_time(section_encryption, "Passphrase Time");
-
-  base::ListValue* section_last_session =
-      AddSection(stats_list.get(), "Status from Last Completed Session");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_last_session->Reserve(4);
-  StringSyncStat session_source(section_last_session, "Sync Source");
-  StringSyncStat get_key_result(section_last_session, "GetKey Step Result");
-  StringSyncStat download_result(section_last_session, "Download Step Result");
-  StringSyncStat commit_result(section_last_session, "Commit Step Result");
-
-  base::ListValue* section_counters =
-      AddSection(stats_list.get(), "Running Totals");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_counters->Reserve(7);
-  IntSyncStat notifications_received(section_counters,
-                                     "Notifications Received");
-  IntSyncStat updates_received(section_counters, "Updates Downloaded");
-  IntSyncStat tombstone_updates(section_counters, "Tombstone Updates");
-  IntSyncStat reflected_updates(section_counters, "Reflected Updates");
-  IntSyncStat successful_commits(section_counters, "Successful Commits");
-  IntSyncStat conflicts_resolved_local_wins(section_counters,
-                                            "Conflicts Resolved: Client Wins");
-  IntSyncStat conflicts_resolved_server_wins(section_counters,
-                                             "Conflicts Resolved: Server Wins");
-
-  base::ListValue* section_this_cycle =
-      AddSection(stats_list.get(), "Transient Counters (this cycle)");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_this_cycle->Reserve(4);
-  IntSyncStat encryption_conflicts(section_this_cycle, "Encryption Conflicts");
-  IntSyncStat hierarchy_conflicts(section_this_cycle, "Hierarchy Conflicts");
-  IntSyncStat server_conflicts(section_this_cycle, "Server Conflicts");
-  IntSyncStat committed_items(section_this_cycle, "Committed Items");
-
-  base::ListValue* section_that_cycle =
-      AddSection(stats_list.get(),
-                 "Transient Counters (last cycle of last completed session)");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_that_cycle->Reserve(3);
-  IntSyncStat updates_downloaded(section_that_cycle, "Updates Downloaded");
-  IntSyncStat committed_count(section_that_cycle, "Committed Count");
-  IntSyncStat entries(section_that_cycle, "Entries");
-
-  base::ListValue* section_nudge_info =
-      AddSection(stats_list.get(), "Nudge Source Counters");
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  section_nudge_info->Reserve(3);
-  IntSyncStat nudge_source_notification(section_nudge_info,
-                                        "Server Invalidations");
-  IntSyncStat nudge_source_local(section_nudge_info, "Local Changes");
-  IntSyncStat nudge_source_local_refresh(section_nudge_info, "Local Refreshes");
-
-  // This list of sections belongs in the 'details' field of the returned
-  // message.
-  about_info->Set(kDetailsKey, std::move(stats_list));
+  SectionList section_list;
+
+  Section* section_summary = section_list.AddSection("Summary");
+  Stat<std::string>* summary_string = section_summary->AddStringStat("Summary");
+
+  Section* section_version = section_list.AddSection("Version Info");
+  Stat<std::string>* client_version =
+      section_version->AddStringStat("Client Version");
+  Stat<std::string>* server_url = section_version->AddStringStat("Server URL");
+
+  Section* section_identity = section_list.AddSection(kIdentityTitle);
+  section_identity->MarkSensitive();
+  Stat<std::string>* sync_id =
+      section_identity->AddStringStat("Sync Client ID");
+  Stat<std::string>* invalidator_id =
+      section_identity->AddStringStat("Invalidator Client ID");
+  Stat<std::string>* username = section_identity->AddStringStat("Username");
+
+  Section* section_credentials = section_list.AddSection("Credentials");
+  Stat<std::string>* request_token_time =
+      section_credentials->AddStringStat("Requested Token");
+  Stat<std::string>* receive_token_time =
+      section_credentials->AddStringStat("Received Token");
+  Stat<std::string>* token_request_status =
+      section_credentials->AddStringStat("Token Request Status");
+  Stat<std::string>* next_token_request =
+      section_credentials->AddStringStat("Next Token Request");
+
+  Section* section_local = section_list.AddSection("Local State");
+  Stat<std::string>* server_connection =
+      section_local->AddStringStat("Server Connection");
+  Stat<std::string>* last_synced = section_local->AddStringStat("Last Synced");
+  Stat<bool>* is_setup_complete =
+      section_local->AddBoolStat("Sync First-Time Setup Complete");
+  Stat<std::string>* backend_initialization =
+      section_local->AddStringStat("Sync Backend Initialization");
+  Stat<bool>* is_syncing = section_local->AddBoolStat("Syncing");
+  Stat<bool>* is_local_sync_enabled =
+      section_local->AddBoolStat("Local Sync Backend Enabled");
+  Stat<std::string>* local_backend_path =
+      section_local->AddStringStat("Local Backend Path");
+
+  Section* section_network = section_list.AddSection("Network");
+  Stat<bool>* is_any_throttled_or_backoff =
+      section_network->AddBoolStat("Throttled or Backoff");
+  Stat<std::string>* retry_time = section_network->AddStringStat("Retry Time");
+  Stat<bool>* are_notifications_enabled =
+      section_network->AddBoolStat("Notifications Enabled");
+
+  Section* section_encryption = section_list.AddSection("Encryption");
+  Stat<bool>* is_using_explicit_passphrase =
+      section_encryption->AddBoolStat("Explicit Passphrase");
+  Stat<bool>* is_passphrase_required =
+      section_encryption->AddBoolStat("Passphrase Required");
+  Stat<bool>* is_cryptographer_ready =
+      section_encryption->AddBoolStat("Cryptographer Ready");
+  Stat<bool>* has_pending_keys =
+      section_encryption->AddBoolStat("Cryptographer Has Pending Keys");
+  Stat<std::string>* encrypted_types =
+      section_encryption->AddStringStat("Encrypted Types");
+  Stat<bool>* has_keystore_key =
+      section_encryption->AddBoolStat("Has Keystore Key");
+  Stat<std::string>* keystore_migration_time =
+      section_encryption->AddStringStat("Keystore Migration Time");
+  Stat<std::string>* passphrase_type =
+      section_encryption->AddStringStat("Passphrase Type");
+  Stat<std::string>* passphrase_time =
+      section_encryption->AddStringStat("Passphrase Time");
+
+  Section* section_last_session =
+      section_list.AddSection("Status from Last Completed Session");
+  Stat<std::string>* session_source =
+      section_last_session->AddStringStat("Sync Source");
+  Stat<std::string>* get_key_result =
+      section_last_session->AddStringStat("GetKey Step Result");
+  Stat<std::string>* download_result =
+      section_last_session->AddStringStat("Download Step Result");
+  Stat<std::string>* commit_result =
+      section_last_session->AddStringStat("Commit Step Result");
+
+  Section* section_counters = section_list.AddSection("Running Totals");
+  Stat<int>* notifications_received =
+      section_counters->AddIntStat("Notifications Received");
+  Stat<int>* updates_received =
+      section_counters->AddIntStat("Updates Downloaded");
+  Stat<int>* tombstone_updates =
+      section_counters->AddIntStat("Tombstone Updates");
+  Stat<int>* reflected_updates =
+      section_counters->AddIntStat("Reflected Updates");
+  Stat<int>* successful_commits =
+      section_counters->AddIntStat("Successful Commits");
+  Stat<int>* conflicts_resolved_local_wins =
+      section_counters->AddIntStat("Conflicts Resolved: Client Wins");
+  Stat<int>* conflicts_resolved_server_wins =
+      section_counters->AddIntStat("Conflicts Resolved: Server Wins");
+
+  Section* section_this_cycle =
+      section_list.AddSection("Transient Counters (this cycle)");
+  Stat<int>* encryption_conflicts =
+      section_this_cycle->AddIntStat("Encryption Conflicts");
+  Stat<int>* hierarchy_conflicts =
+      section_this_cycle->AddIntStat("Hierarchy Conflicts");
+  Stat<int>* server_conflicts =
+      section_this_cycle->AddIntStat("Server Conflicts");
+  Stat<int>* committed_items =
+      section_this_cycle->AddIntStat("Committed Items");
+
+  Section* section_that_cycle = section_list.AddSection(
+      "Transient Counters (last cycle of last completed session)");
+  Stat<int>* updates_downloaded =
+      section_that_cycle->AddIntStat("Updates Downloaded");
+  Stat<int>* committed_count =
+      section_that_cycle->AddIntStat("Committed Count");
+  Stat<int>* entries = section_that_cycle->AddIntStat("Entries");
+
+  Section* section_nudge_info =
+      section_list.AddSection("Nudge Source Counters");
+  Stat<int>* nudge_source_notification =
+      section_nudge_info->AddIntStat("Server Invalidations");
+  Stat<int>* nudge_source_local =
+      section_nudge_info->AddIntStat("Local Changes");
+  Stat<int>* nudge_source_local_refresh =
+      section_nudge_info->AddIntStat("Local Refreshes");
 
   // Populate all the fields we declared above.
-  client_version.SetValue(GetVersionString(channel));
+  client_version->Set(GetVersionString(channel));
 
   if (!service) {
-    summary_string.SetValue("Sync service does not exist");
+    summary_string->Set("Sync service does not exist");
+    about_info->SetKey(kDetailsKey, section_list.ToValue());
     return about_info;
   }
 
   SyncStatus full_status;
   bool is_status_valid = service->QueryDetailedSyncStatus(&full_status);
-  bool sync_active = service->IsSyncActive();
   const SyncCycleSnapshot& snapshot = service->GetLastCycleSnapshot();
+  const SyncService::SyncTokenStatus& token_status =
+      service->GetSyncTokenStatus();
 
+  // Summary.
   if (is_status_valid)
-    summary_string.SetValue(service->QuerySyncStatusSummaryString());
+    summary_string->Set(service->QuerySyncStatusSummaryString());
 
-  server_url.SetValue(service->sync_service_url().spec());
+  // Version Info.
+  // |client_version| was already set above.
+  server_url->Set(service->sync_service_url().spec());
 
+  // Identity.
   if (is_status_valid && !full_status.sync_id.empty())
-    sync_id.SetValue(full_status.sync_id);
+    sync_id->Set(full_status.sync_id);
   if (is_status_valid && !full_status.invalidator_client_id.empty())
-    invalidator_id.SetValue(full_status.invalidator_client_id);
-
-  username.SetValue(primary_account_info.email);
+    invalidator_id->Set(full_status.invalidator_client_id);
+  username->Set(primary_account_info.email);
 
-  const SyncService::SyncTokenStatus& token_status =
-      service->GetSyncTokenStatus();
-  server_connection.SetValue(GetConnectionStatus(token_status));
-  request_token_time.SetValue(
-      GetTimeStr(token_status.token_request_time, "n/a"));
-  receive_token_time.SetValue(
-      GetTimeStr(token_status.token_receive_time, "n/a"));
+  // Credentials.
+  request_token_time->Set(GetTimeStr(token_status.token_request_time, "n/a"));
+  receive_token_time->Set(GetTimeStr(token_status.token_receive_time, "n/a"));
   std::string err = token_status.last_get_token_error.error_message();
-  token_request_status.SetValue(err.empty() ? "OK" : err);
-  next_token_request.SetValue(
+  token_request_status->Set(err.empty() ? "OK" : err);
+  next_token_request->Set(
       GetTimeStr(token_status.next_token_request_time, "not scheduled"));
 
-  last_synced.SetValue(GetLastSyncedTimeString(service->GetLastSyncedTime()));
-  is_setup_complete.SetValue(service->IsFirstSetupComplete());
-  is_local_sync_enabled.SetValue(service->IsLocalSyncEnabled());
-  if (service->IsLocalSyncEnabled() && is_status_valid) {
-    local_backend_path.SetValue(full_status.local_sync_folder);
-  }
-  backend_initialization.SetValue(
-      service->GetEngineInitializationStateString());
-  if (is_status_valid) {
-    is_syncing.SetValue(full_status.syncing);
-    retry_time.SetValue(GetTimeStr(full_status.retry_time,
-                                   "Scheduler is not in backoff or throttled"));
-  }
+  // Local State.
+  server_connection->Set(GetConnectionStatus(token_status));
+  last_synced->Set(GetLastSyncedTimeString(service->GetLastSyncedTime()));
+  is_setup_complete->Set(service->IsFirstSetupComplete());
+  backend_initialization->Set(service->GetEngineInitializationStateString());
+  if (is_status_valid)
+    is_syncing->Set(full_status.syncing);
+  is_local_sync_enabled->Set(service->IsLocalSyncEnabled());
+  if (service->IsLocalSyncEnabled() && is_status_valid)
+    local_backend_path->Set(full_status.local_sync_folder);
 
+  // Network.
   if (snapshot.is_initialized())
-    is_any_throttled_or_backoff.SetValue(snapshot.is_silenced());
+    is_any_throttled_or_backoff->Set(snapshot.is_silenced());
   if (is_status_valid) {
-    are_notifications_enabled.SetValue(full_status.notifications_enabled);
+    retry_time->Set(GetTimeStr(full_status.retry_time,
+                               "Scheduler is not in backoff or throttled"));
   }
+  if (is_status_valid)
+    are_notifications_enabled->Set(full_status.notifications_enabled);
 
-  if (sync_active) {
-    is_using_explicit_passphrase.SetValue(
-        service->IsUsingSecondaryPassphrase());
-    is_passphrase_required.SetValue(service->IsPassphraseRequired());
-    passphrase_time.SetValue(
+  // Encryption.
+  if (service->IsSyncActive()) {
+    is_using_explicit_passphrase->Set(service->IsUsingSecondaryPassphrase());
+    is_passphrase_required->Set(service->IsPassphraseRequired());
+    passphrase_time->Set(
         GetTimeStr(service->GetExplicitPassphraseTime(), "No Passphrase Time"));
   }
   if (is_status_valid) {
-    is_cryptographer_ready.SetValue(full_status.cryptographer_ready);
-    has_pending_keys.SetValue(full_status.crypto_has_pending_keys);
-    encrypted_types.SetValue(ModelTypeSetToString(full_status.encrypted_types));
-    has_keystore_key.SetValue(full_status.has_keystore_key);
-    keystore_migration_time.SetValue(
+    is_cryptographer_ready->Set(full_status.cryptographer_ready);
+    has_pending_keys->Set(full_status.crypto_has_pending_keys);
+    encrypted_types->Set(ModelTypeSetToString(full_status.encrypted_types));
+    has_keystore_key->Set(full_status.has_keystore_key);
+    keystore_migration_time->Set(
         GetTimeStr(full_status.keystore_migration_time, "Not Migrated"));
-    passphrase_type.SetValue(
-        PassphraseTypeToString(full_status.passphrase_type));
+    passphrase_type->Set(PassphraseTypeToString(full_status.passphrase_type));
   }
 
+  // Status from Last Completed Session.
   if (snapshot.is_initialized()) {
     if (snapshot.get_updates_origin() != sync_pb::SyncEnums::UNKNOWN_ORIGIN) {
-      session_source.SetValue(ProtoEnumToString(snapshot.get_updates_origin()));
+      session_source->Set(ProtoEnumToString(snapshot.get_updates_origin()));
     }
-    get_key_result.SetValue(GetSyncerErrorString(
+    get_key_result->Set(GetSyncerErrorString(
         snapshot.model_neutral_state().last_get_key_result));
-    download_result.SetValue(GetSyncerErrorString(
+    download_result->Set(GetSyncerErrorString(
         snapshot.model_neutral_state().last_download_updates_result));
-    commit_result.SetValue(
+    commit_result->Set(
         GetSyncerErrorString(snapshot.model_neutral_state().commit_result));
   }
 
+  // Running Totals.
   if (is_status_valid) {
-    notifications_received.SetValue(full_status.notifications_received);
-    updates_received.SetValue(full_status.updates_received);
-    tombstone_updates.SetValue(full_status.tombstone_updates_received);
-    reflected_updates.SetValue(full_status.reflected_updates_received);
-    successful_commits.SetValue(full_status.num_commits_total);
-    conflicts_resolved_local_wins.SetValue(
-        full_status.num_local_overwrites_total);
-    conflicts_resolved_server_wins.SetValue(
+    notifications_received->Set(full_status.notifications_received);
+    updates_received->Set(full_status.updates_received);
+    tombstone_updates->Set(full_status.tombstone_updates_received);
+    reflected_updates->Set(full_status.reflected_updates_received);
+    successful_commits->Set(full_status.num_commits_total);
+    conflicts_resolved_local_wins->Set(full_status.num_local_overwrites_total);
+    conflicts_resolved_server_wins->Set(
         full_status.num_server_overwrites_total);
   }
 
+  // Transient Counters (this cycle).
   if (is_status_valid) {
-    encryption_conflicts.SetValue(full_status.encryption_conflicts);
-    hierarchy_conflicts.SetValue(full_status.hierarchy_conflicts);
-    server_conflicts.SetValue(full_status.server_conflicts);
-    committed_items.SetValue(full_status.committed_count);
-  }
-
-  if (is_status_valid) {
-    nudge_source_notification.SetValue(full_status.nudge_source_notification);
-    nudge_source_local.SetValue(full_status.nudge_source_local);
-    nudge_source_local_refresh.SetValue(full_status.nudge_source_local_refresh);
+    encryption_conflicts->Set(full_status.encryption_conflicts);
+    hierarchy_conflicts->Set(full_status.hierarchy_conflicts);
+    server_conflicts->Set(full_status.server_conflicts);
+    committed_items->Set(full_status.committed_count);
   }
 
+  // Transient Counters (last cycle of last completed session).
   if (snapshot.is_initialized()) {
-    updates_downloaded.SetValue(
+    updates_downloaded->Set(
         snapshot.model_neutral_state().num_updates_downloaded_total);
-    committed_count.SetValue(
-        snapshot.model_neutral_state().num_successful_commits);
-    entries.SetValue(snapshot.num_entries());
+    committed_count->Set(snapshot.model_neutral_state().num_successful_commits);
+    entries->Set(static_cast<int>(snapshot.num_entries()));
   }
 
+  // Nudge Source Counters.
+  if (is_status_valid) {
+    nudge_source_notification->Set(full_status.nudge_source_notification);
+    nudge_source_local->Set(full_status.nudge_source_local);
+    nudge_source_local_refresh->Set(full_status.nudge_source_local_refresh);
+  }
+
+  // This list of sections belongs in the 'details' field of the returned
+  // message.
+  about_info->SetKey(kDetailsKey, section_list.ToValue());
+
   // The values set from this point onwards do not belong in the
   // details list.
 
@@ -581,44 +524,46 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
       full_status.sync_protocol_error.error_type != UNKNOWN_ERROR &&
       full_status.sync_protocol_error.error_type != SYNC_SUCCESS;
 
-  about_info->SetBoolean("actionable_error_detected",
-                         actionable_error_detected);
+  about_info->SetKey("actionable_error_detected",
+                     base::Value(actionable_error_detected));
 
   // NOTE: We won't bother showing any of the following values unless
   // actionable_error_detected is set.
 
-  auto actionable_error = std::make_unique<base::ListValue>();
-  // TODO(crbug.com/702230): Remove the usages of raw pointers in this file.
-  actionable_error->Reserve(4);
-
-  StringSyncStat error_type(actionable_error.get(), "Error Type");
-  StringSyncStat action(actionable_error.get(), "Action");
-  StringSyncStat url(actionable_error.get(), "URL");
-  StringSyncStat description(actionable_error.get(), "Error Description");
-  about_info->Set("actionable_error", std::move(actionable_error));
+  base::Value actionable_error(base::Value::Type::LIST);
+  Stat<std::string> error_type("Error Type", kUninitialized);
+  Stat<std::string> action("Action", kUninitialized);
+  Stat<std::string> url("URL", kUninitialized);
+  Stat<std::string> description("Error Description", kUninitialized);
 
   if (actionable_error_detected) {
-    error_type.SetValue(
+    error_type.Set(
         GetSyncErrorTypeString(full_status.sync_protocol_error.error_type));
-    action.SetValue(
-        GetClientActionString(full_status.sync_protocol_error.action));
-    url.SetValue(full_status.sync_protocol_error.url);
-    description.SetValue(full_status.sync_protocol_error.error_description);
+    action.Set(GetClientActionString(full_status.sync_protocol_error.action));
+    url.Set(full_status.sync_protocol_error.url);
+    description.Set(full_status.sync_protocol_error.error_description);
   }
 
-  about_info->SetBoolean("unrecoverable_error_detected",
-                         service->HasUnrecoverableError());
+  actionable_error.GetList().push_back(error_type.ToValue());
+  actionable_error.GetList().push_back(action.ToValue());
+  actionable_error.GetList().push_back(url.ToValue());
+  actionable_error.GetList().push_back(description.ToValue());
+  about_info->SetKey("actionable_error", std::move(actionable_error));
+
+  about_info->SetKey("unrecoverable_error_detected",
+                     base::Value(service->HasUnrecoverableError()));
 
   if (service->HasUnrecoverableError()) {
     std::string unrecoverable_error_message =
         "Unrecoverable error detected at " +
         service->unrecoverable_error_location().ToString() + ": " +
         service->unrecoverable_error_message();
-    about_info->SetString("unrecoverable_error_message",
-                          unrecoverable_error_message);
+    about_info->SetKey("unrecoverable_error_message",
+                       base::Value(unrecoverable_error_message));
   }
 
-  about_info->Set("type_status", service->GetTypeStatusMap());
+  about_info->SetKey("type_status", base::Value::FromUniquePtrValue(
+                                        service->GetTypeStatusMap()));
 
   return about_info;
 }