[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..4d3f9edf8549144a87da080a0b145cda5639a901 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,59 @@ 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("TestShortcuts.db"));

[Mark P, 2016/04/19 23:34:28]

If it doesn't matter that they're different, please revise the other tests in
this file to use the same name.

[Scott Hess - ex-Googler, 2016/05/13 21:24:35]

Done.

+  ASSERT_TRUE(sql::test::CreateDatabaseFromSQL(db_path, sql_path));
+
+  // Break the database.
+  ASSERT_TRUE(sql::test::CorruptSizeInHeader(db_path));
+
+  // Verify that the database is broken.
+  {
+    sql::ScopedErrorIgnorer ignore_errors;
+    ignore_errors.IgnoreError(SQLITE_CORRUPT);
+
+    sql::Connection connection;
+    ASSERT_TRUE(connection.Open(db_path));
+    sql::Statement statement(connection.GetUniqueStatement(

[Mark P, 2016/04/19 23:34:28]

Thank you for the explanation.  That said, my original request still applies. 
Please add a comment here, in particular about why the statement.is_valid() test
should be false.  To a naive eye, I'd think it should be true.

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Done.  Pulled up the statement sql, noted that the post-recovery version
succeeds.

+        "SELECT COUNT(*) FROM omni_box_shortcuts"));
+    ASSERT_FALSE(statement.is_valid());
+
+    ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());

[Mark P, 2016/04/19 23:34:28]

I don't agree.  I see at least two ways to understand CheckIgnoredErrors().  One
could simply be "return whether we have an error that was ignored".  Another
could be "return whether everything is okay, i.e., check that it's okay, taking
in account ignored errors".

Furthermore, given that this function declaration is in the context of a test
// Allow containing test to check if the errors were encountered.
// ...
it's very easy to read the "check" as one of the debug macros like ASSERT or
EXPECT, not simply find the value and return.



  // Allow containing test to check if the errors were encountered.
  // Failure to call results in ADD_FAILURE() in destructor.
  bool CheckIgnoredErrors();

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Obviously there's a terminology problem, here, but I'm a little befuddled how to
resolve it.  ScopedErrorIgnorer exists entirely/strictly to track errors within
a scope.  I'm not sure what "everything" might refer to in that context, other
than the errors the ScopedErrorIgnorer has been enlisted to track.

I'd be happy to refactor that a little, I'll open up a discussion thread offline
and this CL can wait on that.  The original reviewer of that code maybe wasn't
enthusiastic enough about being roped into this.

+  }
+
+  // The sql::Connection::Open() in ShortcutsDatabase::Init() will hit the

[Mark P, 2016/04/19 23:34:28]

nit: in -> call in

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Done.

+  // corruption, the error callback will 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/19 23:34:28]

Given this, I don't like this test's structure.  I want a test that verifies the
database opens correctly when not in the context of an IgnoreError regime. 
Either fix this so it clears the error in the middle as I originally expected or
add another block below to verify that.

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

I'm not sure what you're referring to with "clears the error".  Nobody is
clearing errors.  The database can't open correctly when it is corrupt and not
in the context of an error ignorer, there will be a crash.  As far as whether
the Open() retry works when the error callback fixes things, that's already
being tested by SQLConnectionTest.RazeCallbackReopen.  This test is testing that
the Open() retry logic works when the shortcuts error handler is in the loop.

I don't think there is a point where ShortcutsDatabase can crack things open to
allow seeing the flow of control for failure retry within
sql::Connection::Open().  I could add a test that uses the shortcuts error
handler against a plain sql::Connection, but that would imply exporting the
error handler, which may-or-may-not be appropriate (right now it would work
because it's written in terms of only the sql::Connection, but that wouldn't
work if it were revised to reference ShortcutsDatabase).

+  }
+
+  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(
+        "SELECT COUNT(*) FROM omni_box_shortcuts"));

[Mark P, 2016/04/19 23:34:28]

Almost done.  I'd like exactly the same tests, i.e., also add a 
ASSERT_TRUE(statement.is_valid());

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Done.

+    ASSERT_TRUE(statement.Step());
+    EXPECT_EQ(5, statement.ColumnInt(0));

[Mark P, 2016/04/19 23:34:27]

I don't have a strong preference for how you fix it, but fundamentally I don't
want a magic number "5" here because it's not clear where it comes from and
which / whether other changes in this file should cause this number to change
and which shouldn't.  Please solve this code.

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

It's dependent on the contents of the golden file.  I'll generate the expected
value from the uncorrupted database.

+  }
+}