[Sync] Handle history database error in TypedURLSyncBridge

The issue is that during initialization HistoryBackend passes raw pointer of
HistoryDatabase to TypedURLSyncBridge. Later when database error is detected
HistoryBackend resets HistoryDatabase object leaving TypedURLSyncBridge with
invalid pointer. When updates from server arrive TypedURLSyncBridge tries to
write them to database causing crash.

The fix is to notify TypedURLSyncBridge about database error, reset
sync_metadata_database_ pointer and report error to sync so that datatype is
disabled and doesn't receive updates from server.

BUG=789876
R=skym@chromium.org

Change-Id: Ic62709f37eef24f19e7f729998cee56943024084
Reviewed-on: https://chromium-review.googlesource.com/815880Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523145}

components/history/core/browser/history_backend.cc

@@ -2527,6 +2527,10 @@ void HistoryBackend::DatabaseErrorCallback(int error, sql::Statement* stmt) {
 
     db_diagnostics_ = db_->GetDiagnosticInfo(error, stmt);
 
+    // Notify SyncBridge about storage error. It will report failure to sync
+    // engine and stop accepting remote updates.
+    typed_url_sync_bridge_->OnDatabaseError();
+
     // Don't just do the close/delete here, as we are being called by |db| and
     // that seems dangerous.
     // TODO(shess): Consider changing KillHistoryDatabase() to use

components/history/core/browser/typed_url_sync_bridge.cc

@@ -90,7 +90,6 @@ TypedURLSyncBridge::TypedURLSyncBridge(
       history_backend_observer_(this) {
   DCHECK(history_backend_);
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  DCHECK(sync_metadata_database_);
 }
 
 TypedURLSyncBridge::~TypedURLSyncBridge() {
@@ -192,7 +191,6 @@ base::Optional<ModelError> TypedURLSyncBridge::ApplySyncChanges(
     std::unique_ptr<MetadataChangeList> metadata_change_list,
     EntityChangeList entity_changes) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  DCHECK(sync_metadata_database_);
 
   std::vector<GURL> pending_deleted_urls;
   TypedURLVisitVector new_synced_visits;
@@ -203,7 +201,7 @@ base::Optional<ModelError> TypedURLSyncBridge::ApplySyncChanges(
   for (const EntityChange& entity_change : entity_changes) {
     if (entity_change.type() == EntityChange::ACTION_DELETE) {
       URLRow url_row;
-      int64_t url_id = sync_metadata_database_->StorageKeyToURLID(
+      int64_t url_id = TypedURLSyncMetadataDatabase::StorageKeyToURLID(
           entity_change.storage_key());
       if (!history_backend_->GetURLByID(url_id, &url_row)) {
         // Ignoring the case that there is no matching URLRow with URLID
@@ -263,12 +261,11 @@ base::Optional<ModelError> TypedURLSyncBridge::ApplySyncChanges(
 void TypedURLSyncBridge::GetData(StorageKeyList storage_keys,
                                  DataCallback callback) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  DCHECK(sync_metadata_database_);
 
   auto batch = base::MakeUnique<MutableDataBatch>();
   for (const std::string& key : storage_keys) {
     URLRow url_row;
-    URLID url_id = sync_metadata_database_->StorageKeyToURLID(key);
+    URLID url_id = TypedURLSyncMetadataDatabase::StorageKeyToURLID(key);
 
     ++num_db_accesses_;
     if (!history_backend_->GetURLByID(url_id, &url_row)) {
@@ -352,6 +349,7 @@ void TypedURLSyncBridge::OnURLVisited(HistoryBackend* history_backend,
                                       const RedirectList& redirects,
                                       base::Time visit_time) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(sync_metadata_database_);
 
   if (processing_syncer_changes_)
     return;  // These are changes originating from us, ignore.
@@ -370,6 +368,7 @@ void TypedURLSyncBridge::OnURLVisited(HistoryBackend* history_backend,
 void TypedURLSyncBridge::OnURLsModified(HistoryBackend* history_backend,
                                         const URLRows& changed_urls) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(sync_metadata_database_);
 
   if (processing_syncer_changes_)
     return;  // These are changes originating from us, ignore.
@@ -396,6 +395,7 @@ void TypedURLSyncBridge::OnURLsDeleted(HistoryBackend* history_backend,
                                        const URLRows& deleted_rows,
                                        const std::set<GURL>& favicon_urls) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(sync_metadata_database_);
 
   if (processing_syncer_changes_)
     return;  // These are changes originating from us, ignore.
@@ -443,6 +443,12 @@ void TypedURLSyncBridge::Init() {
   LoadMetadata();
 }
 
+void TypedURLSyncBridge::OnDatabaseError() {
+  sync_metadata_database_ = nullptr;
+  change_processor()->ReportError(FROM_HERE,
+                                  "HistoryDatabase encountered error");
+}
+
 int TypedURLSyncBridge::GetErrorPercentage() const {
   return num_db_accesses_ ? (100 * num_db_errors_ / num_db_accesses_) : 0;
 }

components/history/core/browser/typed_url_sync_bridge.h

@@ -56,6 +56,10 @@ class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge,
   // Must be called after creation and before any operations.
   void Init();
 
+  // Called by HistoryBackend when database error is reported through
+  // DatabaseErrorCallback.
+  void OnDatabaseError();
+
   // Returns the percentage of DB accesses that have resulted in an error.
   int GetErrorPercentage() const;
 
@@ -223,7 +227,7 @@ class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge,
 
   // A non-owning pointer to the database, which is for storing typed urls sync
   // metadata and state.
-  TypedURLSyncMetadataDatabase* const sync_metadata_database_;
+  TypedURLSyncMetadataDatabase* sync_metadata_database_;
 
   // Statistics for the purposes of tracking the percentage of DB accesses that
   // fail for each client via UMA.

components/history/core/browser/typed_url_sync_bridge_unittest.cc

@@ -455,7 +455,7 @@ class TypedURLSyncBridgeTest : public testing::Test {
     return bridge()->sync_metadata_database_;
   }
 
-  const RecordingModelTypeChangeProcessor& processor() { return *processor_; }
+  RecordingModelTypeChangeProcessor& processor() { return *processor_; }
 
  protected:
   base::MessageLoop message_loop_;
@@ -1566,4 +1566,10 @@ TEST_F(TypedURLSyncBridgeTest, LocalExpiredTypedUrlDoNotSync) {
             url_specifics.visit_transitions(0));
 }
 
+// Tests that database error gets reported to processor as model type error.
+TEST_F(TypedURLSyncBridgeTest, DatabaseError) {
+  processor().ExpectError();
+  bridge()->OnDatabaseError();
+}
+
 }  // namespace history

components/sync/model_impl/sync_metadata_store_change_list.cc

@@ -15,7 +15,9 @@ SyncMetadataStoreChangeList::SyncMetadataStoreChangeList(
     SyncMetadataStore* store,
     syncer::ModelType type)
     : store_(store), type_(type) {
-  DCHECK(store_);
+  if (!store_) {
+    error_ = ModelError(FROM_HERE, "Invalid SyncMetadataStore");
+  }
 }
 
 SyncMetadataStoreChangeList::~SyncMetadataStoreChangeList() {