AppCache: Run a quick integrity check on the sqlite database when opening.

webkit/browser/appcache/appcache_database_unittest.cc

 // 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.
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/strings/stringprintf.h"
 #include "sql/connection.h"
 #include "sql/meta_table.h"
@@ skipping 55 matching lines @@
   EXPECT_TRUE(base::DirectoryExists(kNestedDir));
   EXPECT_TRUE(base::PathExists(kOtherFile));
 
   EXPECT_TRUE(db.DeleteExistingAndCreateNewDatabase());
 
   EXPECT_TRUE(base::PathExists(kDbFile));
   EXPECT_FALSE(base::DirectoryExists(kNestedDir));
   EXPECT_FALSE(base::PathExists(kOtherFile));
 }
 
+#ifdef NDEBUG
+// Only run in release builds because sql::Connection and familiy
+// crank up DLOG(FATAL)'ness and this test presents it with
+// intentionally bad data which causes debug builds to exit instead
+// of run to completion. In release builds, errors the are deliverd
+// to the consumer so  we can test the error handling of the consumer.

[Scott Hess - ex-Googler, 2013/12/13 20:03:45]

Does sql::ScopedErrorIgnorer not handle this?  There are edge cases out there,
but mostly it's not a problem.

[michaeln, 2013/12/13 22:46:05]

It does not, other DLOG(FATAL)'s get hit given these inputs, one example is in
Connnection::GetUniqueStatement() but there are others too.
  int rc = sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL);
  if (rc != SQLITE_OK) {
    // This is evidence of a syntax error in the incoming SQL.
    DLOG(FATAL) << "SQL compile error " << GetErrorMessage();

I could modify this to use CorruptSizeInHeader instead to make the inputs?

[Scott Hess - ex-Googler, 2013/12/13 23:15:46]

I think CorruptSizeInHeader() will let it get through things, though let me know
if it doesn't.

I think I've seen that same problem with using garbage data.  That's why a
couple tests in connection_unittest.cc use IsSQLValid() instead of executing it.
 I'm not sure how feasible that it in this case, though.  Long-term, I should
maybe augment the error ignorer.

My main concern is doing things like NDEBUG.  It makes it harder to track down
failures, because you'll get "Failing BlahTest.SpecificSomething" and your first
build won't show any failures, eventually you'll realize it's not even running
the test, swear a bit, etc.

[michaeln, 2013/12/13 23:57:01]

The test is modifed to use that helper, but... it gets tripped up on a
DLOG(FATAL) in the Preload() method. I've opened that bug like we talked about
and restored the NDEBUG only'ness.

+TEST(AppCacheDatabaseTest, QuickIntegrityCheck) {
+  // Real files on disk for this test too, a corrupt database file.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath kDbFile = temp_dir.path().AppendASCII("appcache.db");
+  const std::string kCorruptData("deadbeef");
+  EXPECT_EQ(static_cast<int>(kCorruptData.length()),
+            file_util::WriteFile(
+                kDbFile, kCorruptData.data(), kCorruptData.length()));
+  const base::FilePath kOtherFile = temp_dir.path().AppendASCII("other_file");
+  EXPECT_EQ(3, file_util::WriteFile(kOtherFile, "foo", 3));
+
+  // See that the sql::Connection opens, but fails to pass the integrity check.
+  sql::Connection raw_db;
+  EXPECT_TRUE(raw_db.Open(kDbFile));
+  EXPECT_FALSE(raw_db.QuickIntegrityCheck());
+  raw_db.Close();
+
+  // Should delete the corrupt data and start over.
+  AppCacheDatabase db(kDbFile);
+  EXPECT_TRUE(db.LazyOpen(true));
+  EXPECT_FALSE(base::PathExists(kOtherFile));
+  EXPECT_TRUE(base::PathExists(kDbFile));

[Scott Hess - ex-Googler, 2013/12/13 20:03:45]

If I understand things correctly, the AppCacheDatabase must be deleting the
entire directory that kDbFile is in?  If so, prefer to create a subdir under
temp_dir, rather than delete the dir and re-create.  I don't think anything will
go wrong as-is, but messing with the temp_dir itself potentially breaks
assumptions.

[michaeln, 2013/12/13 22:46:05]

Done.

+  db.CloseConnection();
+
+  // See that the Connection now passes its integrity check.
+  EXPECT_TRUE(raw_db.Open(kDbFile));
+  EXPECT_TRUE(raw_db.QuickIntegrityCheck());
+}
+#endif  // NDEBUG
+
 TEST(AppCacheDatabaseTest, ExperimentalFlags) {
   const char kExperimentFlagsKey[] = "ExperimentFlags";
   std::string kInjectedFlags("exp1,exp2");
 
   // Real files on disk for this test.
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   const base::FilePath kDbFile = temp_dir.path().AppendASCII("appcache.db");
   const base::FilePath kOtherFile =  temp_dir.path().AppendASCII("other_file");
   EXPECT_EQ(3, file_util::WriteFile(kOtherFile, "foo", 3));
@@ skipping 1041 matching lines @@
     EXPECT_EQ(expected_namespace_url, fallbacks[i].namespace_.namespace_url);
     EXPECT_EQ(expected_target_url, fallbacks[i].namespace_.target_url);
     EXPECT_FALSE(fallbacks[i].namespace_.is_pattern);
     EXPECT_EQ(expected_whitelist_url, whitelists[i].namespace_url);
     EXPECT_FALSE(whitelists[i].is_pattern);
   }
 }
 #endif  // !APPCACHE_USE_SIMPLE_CACHE
 
 }  // namespace appcache