[Sync] Replace ReportUnrecoverableErrorFunction with base::Closure

Fixes a bug where the raw pointer wasn't being default initialized in all
cases (SyncManager::InitArgs in particular), and ensure that kind of bug
doesn't happen again.

BUG=123223

Review URL: https://codereview.chromium.org/1167183002

Cr-Commit-Position: refs/heads/master@{#333520}

14 files changed, 43 insertions, 78 deletions

diff --git a/sync/BUILD.gn b/sync/BUILD.gn
index e262686..3a5438c 100644
--- a/sync/BUILD.gn
+++ b/sync/BUILD.gn
@@ -250,7 +250,6 @@ source_set("sync_core") {
     "internal_api/public/user_share.h",
     "internal_api/public/util/experiments.h",
     "internal_api/public/util/immutable.h",
-    "internal_api/public/util/report_unrecoverable_error_function.h",
     "internal_api/public/util/sync_db_util.h",
     "internal_api/public/util/sync_string_conversions.cc",
     "internal_api/public/util/sync_string_conversions.h",
diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
index 41d9cae..7899162 100644
--- a/sync/internal_api/public/sync_manager.h
+++ b/sync/internal_api/public/sync_manager.h
@@ -9,7 +9,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
-#include "base/callback_forward.h"
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -30,7 +30,6 @@
 #include "sync/internal_api/public/shutdown_reason.h"
 #include "sync/internal_api/public/sync_context_proxy.h"
 #include "sync/internal_api/public/sync_encryption_handler.h"
-#include "sync/internal_api/public/util/report_unrecoverable_error_function.h"
 #include "sync/internal_api/public/util/unrecoverable_error_handler.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/protocol/sync_protocol_error.h"
@@ -263,7 +262,7 @@ class SYNC_EXPORT SyncManager {
     Encryptor* encryptor;
 
     scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler;
-    ReportUnrecoverableErrorFunction report_unrecoverable_error_function;
+    base::Closure report_unrecoverable_error_function;
 
     // Carries shutdown requests across threads and will be used to cut short
     // any network I/O and tell the syncer to exit early.
diff --git a/sync/internal_api/public/util/report_unrecoverable_error_function.h b/sync/internal_api/public/util/report_unrecoverable_error_function.h
deleted file mode 100644
index c0686ca..0000000
--- a/sync/internal_api/public/util/report_unrecoverable_error_function.h
+++ /dev/null
@@ -1,18 +0,0 @@
-// 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.
-
-#ifndef SYNC_UTIL_REPORT_UNRECOVERABLE_ERROR_FUNCTION_H_
-#define SYNC_UTIL_REPORT_UNRECOVERABLE_ERROR_FUNCTION_H_
-
-namespace syncer {
-
-// A ReportUnrecoverableErrorFunction is a function that is called
-// immediately when an unrecoverable error is encountered.  Unlike
-// UnrecoverableErrorHandler, it should just log the error and any
-// context surrounding it.
-typedef void (*ReportUnrecoverableErrorFunction)(void);
-
-}  // namespace syncer
-
-#endif  // SYNC_UTIL_REPORT_UNRECOVERABLE_ERROR_FUNCTION_H_
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index fe0bb0f..42de79e 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -88,7 +88,6 @@ SyncManagerImpl::SyncManagerImpl(const std::string& name)
       change_delegate_(NULL),
       initialized_(false),
       observing_network_connectivity_changes_(false),
-      report_unrecoverable_error_function_(NULL),
       weak_ptr_factory_(this) {
   // Pre-fill |notification_info_map_|.
   for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h
index f07cbfb..283d3f1 100644
--- a/sync/internal_api/sync_manager_impl.h
+++ b/sync/internal_api/sync_manager_impl.h
@@ -330,7 +330,7 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl
   ProtocolEventBuffer protocol_event_buffer_;
 
   scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler_;
-  ReportUnrecoverableErrorFunction report_unrecoverable_error_function_;
+  base::Closure report_unrecoverable_error_function_;
 
   // Sync's encryption handler. It tracks the set of encrypted types, manages
   // changing passphrases, and in general handles sync-specific interactions
diff --git a/sync/internal_api/sync_rollback_manager_base.cc b/sync/internal_api/sync_rollback_manager_base.cc
index a5501b7..3e748fc 100644
--- a/sync/internal_api/sync_rollback_manager_base.cc
+++ b/sync/internal_api/sync_rollback_manager_base.cc
@@ -37,8 +37,7 @@ class DummyEntryptionHandler : public syncer::SyncEncryptionHandler {
 namespace syncer {
 
 SyncRollbackManagerBase::SyncRollbackManagerBase()
-    : report_unrecoverable_error_function_(NULL),
-      dummy_handler_(new DummyEntryptionHandler),
+    : dummy_handler_(new DummyEntryptionHandler),
       initialized_(false),
       weak_ptr_factory_(this) {
 }
@@ -51,7 +50,7 @@ bool SyncRollbackManagerBase::InitInternal(
     InternalComponentsFactory* internal_components_factory,
     InternalComponentsFactory::StorageOption storage,
     scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler,
-    ReportUnrecoverableErrorFunction report_unrecoverable_error_function) {
+    const base::Closure& report_unrecoverable_error_function) {
   unrecoverable_error_handler_ = unrecoverable_error_handler.Pass();
   report_unrecoverable_error_function_ = report_unrecoverable_error_function;
 
diff --git a/sync/internal_api/sync_rollback_manager_base.h b/sync/internal_api/sync_rollback_manager_base.h
index a284f1f..aa415a2 100644
--- a/sync/internal_api/sync_rollback_manager_base.h
+++ b/sync/internal_api/sync_rollback_manager_base.h
@@ -100,7 +100,7 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
       InternalComponentsFactory* internal_components_factory,
       InternalComponentsFactory::StorageOption storage,
       scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler,
-      ReportUnrecoverableErrorFunction report_unrecoverable_error_function);
+      const base::Closure& report_unrecoverable_error_function);
 
   void RegisterDirectoryTypeDebugInfoObserver(
       syncer::TypeDebugInfoObserver* observer) override;
@@ -129,7 +129,7 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
   base::ObserverList<SyncManager::Observer> observers_;
 
   scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler_;
-  ReportUnrecoverableErrorFunction report_unrecoverable_error_function_;
+  base::Closure report_unrecoverable_error_function_;
 
   scoped_ptr<SyncEncryptionHandler> dummy_handler_;
 
diff --git a/sync/sync.gyp b/sync/sync.gyp
index 6e47201..07d1c2e 100644
--- a/sync/sync.gyp
+++ b/sync/sync.gyp
@@ -303,7 +303,6 @@
         'internal_api/public/user_share.h',
         'internal_api/public/util/experiments.h',
         'internal_api/public/util/immutable.h',
-        'internal_api/public/util/report_unrecoverable_error_function.h',
         'internal_api/public/util/sync_db_util.h',
         'internal_api/public/util/sync_string_conversions.cc',
         'internal_api/public/util/sync_string_conversions.h',
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index 2fbceec..e23065b 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -103,12 +103,11 @@ Directory::Kernel::~Kernel() {
                                        metahandles_map.end());
 }
 
-Directory::Directory(
-    DirectoryBackingStore* store,
-    UnrecoverableErrorHandler* unrecoverable_error_handler,
-    ReportUnrecoverableErrorFunction report_unrecoverable_error_function,
-    NigoriHandler* nigori_handler,
-    Cryptographer* cryptographer)
+Directory::Directory(DirectoryBackingStore* store,
+                     UnrecoverableErrorHandler* unrecoverable_error_handler,
+                     const base::Closure& report_unrecoverable_error_function,
+                     NigoriHandler* nigori_handler,
+                     Cryptographer* cryptographer)
     : kernel_(NULL),
       store_(store),
       unrecoverable_error_handler_(unrecoverable_error_handler),
@@ -1031,6 +1030,12 @@ Cryptographer* Directory::GetCryptographer(const BaseTransaction* trans) {
   return cryptographer_;
 }
 
+void Directory::ReportUnrecoverableError() {
+  if (!report_unrecoverable_error_function_.is_null()) {
+    report_unrecoverable_error_function_.Run();
+  }
+}
+
 void Directory::GetAllMetaHandles(BaseTransaction* trans,
                                   MetahandleSet* result) {
   result->clear();
diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h
index d8a02d9..7358f17 100644
--- a/sync/syncable/directory.h
+++ b/sync/syncable/directory.h
@@ -11,13 +11,13 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #include "base/files/file_util.h"
 #include "base/gtest_prod_util.h"
 #include "base/values.h"
 #include "sync/api/attachments/attachment_id.h"
 #include "sync/base/sync_export.h"
-#include "sync/internal_api/public/util/report_unrecoverable_error_function.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/syncable/dir_open_result.h"
 #include "sync/syncable/entry.h"
@@ -248,13 +248,11 @@ class SYNC_EXPORT Directory {
   // Does not take ownership of |encryptor|.
   // |report_unrecoverable_error_function| may be NULL.
   // Takes ownership of |store|.
-  Directory(
-      DirectoryBackingStore* store,
-      UnrecoverableErrorHandler* unrecoverable_error_handler,
-      ReportUnrecoverableErrorFunction
-          report_unrecoverable_error_function,
-      NigoriHandler* nigori_handler,
-      Cryptographer* cryptographer);
+  Directory(DirectoryBackingStore* store,
+            UnrecoverableErrorHandler* unrecoverable_error_handler,
+            const base::Closure& report_unrecoverable_error_function,
+            NigoriHandler* nigori_handler,
+            Cryptographer* cryptographer);
   virtual ~Directory();
 
   // Does not take ownership of |delegate|, which must not be NULL.
@@ -327,11 +325,7 @@ class SYNC_EXPORT Directory {
 
   // Called to immediately report an unrecoverable error (but don't
   // propagate it up).
-  void ReportUnrecoverableError() {
-    if (report_unrecoverable_error_function_) {
-      report_unrecoverable_error_function_();
-    }
-  }
+  void ReportUnrecoverableError();
 
   // Called to set the unrecoverable error on the directory and to propagate
   // the error to upper layers.
@@ -643,7 +637,7 @@ class SYNC_EXPORT Directory {
   scoped_ptr<DirectoryBackingStore> store_;
 
   UnrecoverableErrorHandler* const unrecoverable_error_handler_;
-  const ReportUnrecoverableErrorFunction report_unrecoverable_error_function_;
+  base::Closure report_unrecoverable_error_function_;
   bool unrecoverable_error_set_;
 
   // Not owned.
diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc
index 96aaa70..65298b1 100644
--- a/sync/syncable/directory_unittest.cc
+++ b/sync/syncable/directory_unittest.cc
@@ -75,10 +75,7 @@ DirOpenResult SyncableDirectoryTest::ReopenDirectory() {
   // performance benefits of not writing to disk.
   dir_.reset(
       new Directory(new TestDirectoryBackingStore(kDirectoryName, &connection_),
-                    &handler_,
-                    NULL,
-                    NULL,
-                    NULL));
+                    &handler_, base::Closure(), NULL, NULL));
 
   DirOpenResult open_result =
       dir_->Open(kDirectoryName, &delegate_, NullTransactionObserver());
@@ -2006,7 +2003,8 @@ TEST_F(SyncableDirectoryTest, SaveChangesSnapshot_HasUnsavedMetahandleChanges) {
 TEST_F(SyncableDirectoryTest, CatastrophicError) {
   MockUnrecoverableErrorHandler unrecoverable_error_handler;
   Directory dir(new InMemoryDirectoryBackingStore("catastrophic_error"),
-                &unrecoverable_error_handler, nullptr, nullptr, nullptr);
+                &unrecoverable_error_handler, base::Closure(), nullptr,
+                nullptr);
   ASSERT_EQ(OPENED, dir.Open(kDirectoryName, directory_change_delegate(),
                              NullTransactionObserver()));
   ASSERT_EQ(0, unrecoverable_error_handler.invocation_count());
diff --git a/sync/syncable/syncable_unittest.cc b/sync/syncable/syncable_unittest.cc
index f7d85cc..1cbd457 100644
--- a/sync/syncable/syncable_unittest.cc
+++ b/sync/syncable/syncable_unittest.cc
@@ -116,7 +116,7 @@ TestDirectory* TestDirectory::Create(
 TestDirectory::TestDirectory(Encryptor* encryptor,
                              UnrecoverableErrorHandler* handler,
                              TestBackingStore* backing_store)
-    : Directory(backing_store, handler, NULL, NULL, NULL),
+    : Directory(backing_store, handler, base::Closure(), NULL, NULL),
       backing_store_(backing_store) {
 }
 
@@ -344,12 +344,9 @@ TEST_F(OnDiskSyncableDirectoryTest,
   }
 
   dir()->SaveChanges();
-  dir().reset(
-      new Directory(new OnDiskDirectoryBackingStore(kDirectoryName, file_path_),
-                    unrecoverable_error_handler(),
-                    NULL,
-                    NULL,
-                    NULL));
+  dir().reset(new Directory(
+      new OnDiskDirectoryBackingStore(kDirectoryName, file_path_),
+      unrecoverable_error_handler(), base::Closure(), NULL, NULL));
 
   ASSERT_TRUE(dir().get());
   ASSERT_EQ(OPENED,
@@ -563,7 +560,7 @@ TEST_F(SyncableDirectoryManagement, TestFileRelease) {
 
   {
     Directory dir(new OnDiskDirectoryBackingStore("ScopeTest", path), &handler_,
-                  NULL, NULL, NULL);
+                  base::Closure(), NULL, NULL);
     DirOpenResult result =
         dir.Open("ScopeTest", &delegate_, NullTransactionObserver());
     ASSERT_EQ(result, OPENED);
diff --git a/sync/test/engine/test_directory_setter_upper.cc b/sync/test/engine/test_directory_setter_upper.cc
index 847a7bb..85e8aa3 100644
--- a/sync/test/engine/test_directory_setter_upper.cc
+++ b/sync/test/engine/test_directory_setter_upper.cc
@@ -27,13 +27,10 @@ void TestDirectorySetterUpper::SetUp() {
   WeakHandle<syncable::TransactionObserver> transaction_observer =
       MakeWeakHandle(test_transaction_observer_->AsWeakPtr());
 
-  directory_.reset(
-      new syncable::Directory(
-          new syncable::InMemoryDirectoryBackingStore(name_),
-          &handler_,
-          NULL,
-          &encryption_handler_,
-          encryption_handler_.cryptographer()));
+  directory_.reset(new syncable::Directory(
+      new syncable::InMemoryDirectoryBackingStore(name_), &handler_,
+      base::Closure(), &encryption_handler_,
+      encryption_handler_.cryptographer()));
   ASSERT_EQ(syncable::OPENED, directory_->Open(
           name_, &delegate_, transaction_observer));
 }
@@ -45,12 +42,9 @@ void TestDirectorySetterUpper::SetUpWith(
   WeakHandle<syncable::TransactionObserver> transaction_observer =
       MakeWeakHandle(test_transaction_observer_->AsWeakPtr());
 
-  directory_.reset(
-      new syncable::Directory(directory_store,
-                              &handler_,
-                              NULL,
-                              &encryption_handler_,
-                              encryption_handler_.cryptographer()));
+  directory_.reset(new syncable::Directory(
+      directory_store, &handler_, base::Closure(), &encryption_handler_,
+      encryption_handler_.cryptographer()));
     ASSERT_EQ(syncable::OPENED, directory_->Open(
           name_, &delegate_, transaction_observer));
 }
diff --git a/sync/tools/sync_client.cc b/sync/tools/sync_client.cc
index 99a2a48..a433fa9 100644
--- a/sync/tools/sync_client.cc
+++ b/sync/tools/sync_client.cc
@@ -38,7 +38,6 @@
 #include "sync/internal_api/public/read_node.h"
 #include "sync/internal_api/public/sync_manager.h"
 #include "sync/internal_api/public/sync_manager_factory.h"
-#include "sync/internal_api/public/util/report_unrecoverable_error_function.h"
 #include "sync/internal_api/public/util/unrecoverable_error_handler.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/js/js_event_details.h"
@@ -437,7 +436,8 @@ int SyncClientMain(int argc, char* argv[]) {
       new InternalComponentsFactoryImpl(factory_switches));
   args.encryptor = &null_encryptor;
   args.unrecoverable_error_handler.reset(new LoggingUnrecoverableErrorHandler);
-  args.report_unrecoverable_error_function = &LogUnrecoverableErrorContext;
+  args.report_unrecoverable_error_function =
+      base::Bind(LogUnrecoverableErrorContext);
   args.cancelation_signal = &scm_cancelation_signal;
   sync_manager->Init(&args);
   // TODO(akalin): Avoid passing in model parameters multiple times by