Commit 08988bf9 authored by Pavel Yatsuk's avatar Pavel Yatsuk Committed by Commit Bot

[Sync] Check typed_url_sync_bridge_ pointer before dereferencing

Instantiation of typed_url_sync_bridge_ in HistoryBackend is controlled by Finch
experiment, when experiment is disabled typed_url_sync_bridge_ is nullptr. In my
previous change (https://crrev.com/c/815880) I introduced call to notify
TypedURLSyncBridge about SQL error, but the call happens regardless if
experiment is enabled or not. This caused crashes described in corresponding
bug.

In this change I check typed_url_sync_bridge_ to be valid before dereferencing
it to call a function.

BUG=796138
R=skym@chromium.org

Change-Id: I08b45479c6d6bce42709ccaad76491ca76bfb232
Reviewed-on: https://chromium-review.googlesource.com/849192Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarSky Malice <skym@chromium.org>
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526840}
parent fa6b5750
...@@ -2529,7 +2529,8 @@ void HistoryBackend::DatabaseErrorCallback(int error, sql::Statement* stmt) { ...@@ -2529,7 +2529,8 @@ void HistoryBackend::DatabaseErrorCallback(int error, sql::Statement* stmt) {
// Notify SyncBridge about storage error. It will report failure to sync // Notify SyncBridge about storage error. It will report failure to sync
// engine and stop accepting remote updates. // engine and stop accepting remote updates.
typed_url_sync_bridge_->OnDatabaseError(); if (typed_url_sync_bridge_)
typed_url_sync_bridge_->OnDatabaseError();
// Don't just do the close/delete here, as we are being called by |db| and // Don't just do the close/delete here, as we are being called by |db| and
// that seems dangerous. // that seems dangerous.
......
...@@ -51,6 +51,7 @@ ...@@ -51,6 +51,7 @@
#include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
#include "third_party/skia/include/core/SkBitmap.h" #include "third_party/skia/include/core/SkBitmap.h"
#include "third_party/sqlite/sqlite3.h"
#include "ui/gfx/codec/png_codec.h" #include "ui/gfx/codec/png_codec.h"
#include "url/gurl.h" #include "url/gurl.h"
...@@ -3839,6 +3840,15 @@ TEST_F(HistoryBackendTest, DeleteFTSIndexDatabases) { ...@@ -3839,6 +3840,15 @@ TEST_F(HistoryBackendTest, DeleteFTSIndexDatabases) {
EXPECT_TRUE(base::PathExists(db2_actual)); // Symlinks shouldn't be followed. EXPECT_TRUE(base::PathExists(db2_actual)); // Symlinks shouldn't be followed.
} }
// Tests that calling DatabaseErrorCallback doesn't cause crash. (Regression
// test for https://crbug.com/796138)
TEST_F(HistoryBackendTest, DatabaseError) {
EXPECT_EQ(nullptr, backend_->GetTypedURLSyncBridge());
backend_->DatabaseErrorCallback(SQLITE_CORRUPT, nullptr);
// Run loop to let any posted callbacks run before TearDown().
base::RunLoop().RunUntilIdle();
}
// Common implementation for the two tests below, given that the only difference // Common implementation for the two tests below, given that the only difference
// between them is the type of the notification sent out. // between them is the type of the notification sent out.
void InMemoryHistoryBackendTest::TestAddingAndChangingURLRows( void InMemoryHistoryBackendTest::TestAddingAndChangingURLRows(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment