[sql] Database recovery system for Shortcuts.

components/omnibox/browser/shortcuts_database_unittest.cc

Index: components/omnibox/browser/shortcuts_database_unittest.cc
diff --git a/components/omnibox/browser/shortcuts_database_unittest.cc b/components/omnibox/browser/shortcuts_database_unittest.cc
index 15116b803d60a1be6fd6fab1deda1030fb2448fa..d6fda97e79f0dc7562db679185fce64e19723855 100644
--- a/components/omnibox/browser/shortcuts_database_unittest.cc
+++ b/components/omnibox/browser/shortcuts_database_unittest.cc
@@ -17,6 +17,7 @@
 #include "components/omnibox/browser/autocomplete_match_type.h"
 #include "components/omnibox/browser/shortcuts_constants.h"
 #include "sql/statement.h"
+#include "sql/test/scoped_error_ignorer.h"
 #include "sql/test/test_helpers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/page_transition_types.h"
@@ -299,3 +300,53 @@ TEST(ShortcutsDatabaseMigrationTest, MigrateV0ToV1) {
   EXPECT_TRUE(temp_dir.Delete());
 #endif
 }
+
+TEST(ShortcutsDatabaseMigrationTest, Recovery1) {
+#if defined(OS_ANDROID)
+  char kBasename[] = "Shortcuts.v1.sql";
+#else
+  char kBasename[] = "Shortcuts.no_fill_into_edit.sql";
+#endif
+  // Use the pre-v0 test file to create a test database in a temp dir.
+  base::FilePath sql_path = GetTestDataDir().AppendASCII(kBasename);
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  base::FilePath db_path(temp_dir.path().AppendASCII("TestShortcuts1.db"));

[Mark P, 2016/04/08 20:12:06]

This probably should be "3", not "1".  (Otherwise why would other tests use
unique numbers?  There must be a reason.)

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Copy/pasta.  Since it's in a unique temp dir, I'll just strip the 1.  To the
best of my knowledge none of these need to be unique names.

+  ASSERT_TRUE(sql::test::CreateDatabaseFromSQL(db_path, sql_path));
+  ASSERT_TRUE(sql::test::CorruptSizeInHeader(db_path));
+
+  // The database is broken.

[Mark P, 2016/04/08 20:12:06]

How about putting a blank line after 315, then adding
// Break the database.
then corrupt the header,
then a blank line, then a comment such as
// Verify the database is broken.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Done.

+  {
+    sql::ScopedErrorIgnorer ignore_errors;
+    ignore_errors.IgnoreError(SQLITE_CORRUPT);
+
+    sql::Connection connection;
+    ASSERT_TRUE(connection.Open(db_path));
+    ASSERT_FALSE(connection.Execute("SELECT count(*) FROM omni_box_shortcuts"));

[Mark P, 2016/04/08 20:12:06]

Why does this return failure if we're ignoring the error?  Comment.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Production code in debug mode fails with a FATAL, because there should not be
cases where that is acceptable.  Execute() is WARN_UNUSED_RESULT to force
developers to pay attention to the result, because in actual production there
will be failures that are not due to how the code is written.  The ignorer tells
sql::Connection that we're testing the code to handle an error case, so don't do
the FATAL thing.

[To be clear, a lot of the failure code paths for sql returns are entirely
untested in Chromium.  A lot of the code is written to assume nothing can fail,
which is true in development but false when deployed.  I've tried to force the
issue a bit with WARN_UNUSED_RESULT, but generally no tests are written for
failure paths, in part because it's challenging to inject failures.  So I agree,
much of this is dubious.]

+
+    ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());

[Mark P, 2016/04/08 20:12:06]

Side comment: the comment by the declaration of CheckIgnoredErrors() is awful. 
It doesn't say what the return type means.  Does it mean we encountered an error
of the type that we ignored?  That's my best guess.  But it could mean that we
encountered an error that didn't ignore, i.e., check status, taking into account
ignored errors...

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

I don't really follow.  It says it's used to check if the errors were
encountered, "True" seems like the most likely response to that question.

+  }
+
+  // sql::Connection::Open() will hit the corruption, the error callback will

[Mark P, 2016/04/08 20:12:06]

sql::Connection::Open() will hit
->
The sql::Connection::Open() call in ShortcutsDatabase::Init() will hit

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Done.

+  // recover and poison the database, then Open() will retry, allowing Init() to
+  // succeed.
+  {
+    sql::ScopedErrorIgnorer ignore_errors;
+    ignore_errors.IgnoreError(SQLITE_CORRUPT);
+
+    scoped_refptr<ShortcutsDatabase> db(new ShortcutsDatabase(db_path));
+    ASSERT_TRUE(db->Init());
+
+    ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());

[Mark P, 2016/04/08 20:12:06]

I think it's odd that we still see this error.  Shouldn't it be cleared after
the first failure and then not have any errors for the second attempt at
opening?

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

The error will be thrown in the first Open(), the ignorer allows it to fail
gracefully, then the retry will succeed.

+  }
+
+  CheckV2ColumnExistence(db_path, true);
+
+  // All of the data should have been recovered.
+  sql::Connection connection;
+  ASSERT_TRUE(connection.Open(db_path));
+  sql::Statement statement(connection.GetUniqueStatement(

[Mark P, 2016/04/08 20:12:06]

Please use an identical call to the one you used to verify the database was
broken, don't switch from execute to getuniquestatement.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Done.

+      "SELECT COUNT(*) FROM omni_box_shortcuts"));
+  ASSERT_TRUE(statement.Step());
+  EXPECT_EQ(5, statement.ColumnInt(0));

[Mark P, 2016/04/08 20:12:06]

Consider using something like
EXPECT_EQ(arraysize(shortcut_test_db), CountRecords())
a la the other tests in this file.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

CountRecords() runs against the sql::Connection contained in the
ShortcutsDatabase created by SetUp().  That database cannot easily be used at
this point.  I could do this:
  db_ = new ShortcutsDatabase(db_path);
but note that this isn't necessarily describing the same database as SetUp().

I could shift the CountRecords() code out to a helper running against a
sql::Connection, then have CountRecords() call that.

+}