gdata: Fix a bug in GDataLevelDB where errors are not handled right

GDataEntry::FromProtoString() fails if incompatible proto is detected
We should return an error rather than DCHECK it.
See crbug.com/142912 for how incompatible proto is rejected.

BUG=127856
TEST=out/Release/unit_tests --gtest_filter=GDataDBTest.*

Review URL: https://chromiumcodereview.appspot.com/10701118

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@145950 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/gdata/gdata_db.h

@@ -47,6 +47,11 @@ class GDataDB {
   // Will not return NULL.
   virtual scoped_ptr<GDataDBIter> CreateIterator(const FilePath& path) = 0;
 
+  // Puts |raw_value| keyed with |resource_id| to the database.
+  // Used for testing (ex. injecting incompatible proto).
+  virtual Status PutRawForTesting(const std::string& resource_id,
+                                  const std::string& raw_value) = 0;
+
  protected:
   GDataDB() {}
 };

chrome/browser/chromeos/gdata/gdata_db_unittest.cc

@@ -5,10 +5,11 @@
 #include "chrome/browser/chromeos/gdata/gdata_db.h"
 
 #include "base/string_number_conversions.h"
+#include "chrome/browser/chromeos/gdata/gdata.pb.h"
 #include "chrome/browser/chromeos/gdata/gdata_db_factory.h"
 #include "chrome/browser/chromeos/gdata/gdata_files.h"
-#include "testing/gtest/include/gtest/gtest.h"
 #include "chrome/test/base/testing_profile.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace gdata {
 namespace {
@@ -33,6 +34,10 @@ class GDataDBTest : public testing::Test {
   // matching |source| exists.
   void TestGetFound(const GDataEntry& source);
 
+  // Tests GDataDB::GetPath and GDataDB::ResourceId, ensuring that an entry
+  // matching |source| is corrupt.
+  void TestGetCorrupt(const GDataEntry& source);
+
   // Initialize the database with the following entries:
   // drive/dir1
   // drive/dir2
@@ -98,6 +103,17 @@ void GDataDBTest::TestGetFound(const GDataEntry& source) {
   EXPECT_EQ(source.content_url(), entry->content_url());
 }
 
+void GDataDBTest::TestGetCorrupt(const GDataEntry& source) {
+  scoped_ptr<GDataEntry> entry;
+  GDataDB::Status status = gdata_db_->GetByPath(source.GetFilePath(), &entry);
+  EXPECT_EQ(GDataDB::DB_CORRUPTION, status);
+  EXPECT_FALSE(entry.get());
+
+  status = gdata_db_->GetByResourceId(source.resource_id(), &entry);
+  EXPECT_EQ(GDataDB::DB_CORRUPTION, status);
+  EXPECT_FALSE(entry.get());
+}
+
 void GDataDBTest::InitDB() {
   int sequence_id = 1;
   GDataRootDirectory root;
@@ -186,17 +202,18 @@ TEST_F(GDataDBTest, PutTest) {
   TestGetNotFound(dir);
 
   GDataDB::Status status = gdata_db_->Put(dir);
-  EXPECT_EQ(GDataDB::DB_OK, status);
+  ASSERT_EQ(GDataDB::DB_OK, status);
 
   TestGetFound(dir);
 
   scoped_ptr<GDataEntry> entry;
-  gdata_db_->GetByPath(dir.GetFilePath(), &entry);
+  status = gdata_db_->GetByPath(dir.GetFilePath(), &entry);
+  ASSERT_EQ(GDataDB::DB_OK, status);
   EXPECT_EQ(dir.upload_url(), entry->AsGDataDirectory()->upload_url());
   EXPECT_TRUE(entry->AsGDataDirectory()->file_info().is_directory);
 
   status = gdata_db_->DeleteByPath(dir.GetFilePath());
-  EXPECT_EQ(GDataDB::DB_OK, status);
+  ASSERT_EQ(GDataDB::DB_OK, status);
 
   TestGetNotFound(dir);
 
@@ -210,16 +227,17 @@ TEST_F(GDataDBTest, PutTest) {
   TestGetNotFound(file);
 
   status = gdata_db_->Put(file);
-  EXPECT_EQ(GDataDB::DB_OK, status);
+  ASSERT_EQ(GDataDB::DB_OK, status);
 
   TestGetFound(file);
 
-  gdata_db_->GetByPath(file.GetFilePath(), &entry);
+  status = gdata_db_->GetByPath(file.GetFilePath(), &entry);
+  ASSERT_EQ(GDataDB::DB_OK, status);
   EXPECT_EQ(file.file_md5(), entry->AsGDataFile()->file_md5());
   EXPECT_FALSE(entry->AsGDataFile()->file_info().is_directory);
 
   status = gdata_db_->DeleteByPath(file.GetFilePath());
-  EXPECT_EQ(GDataDB::DB_OK, status);
+  ASSERT_EQ(GDataDB::DB_OK, status);
 
   TestGetNotFound(file);
 }
@@ -274,4 +292,37 @@ TEST_F(GDataDBTest, IterTest) {
   TestIter("dir4", NULL, 0);
 }
 
+TEST_F(GDataDBTest, IncompatibleProtoTest) {
+  GDataRootDirectory root;
+  GDataFile file(&root, &root);
+  file.set_title("file");
+  file.set_file_name("file");
+  file.set_resource_id("file_resource_id");
+  file.set_content_url(GURL("http://content/dir/file"));
+  file.set_file_md5("file_md5");
+
+  // Add a file and check if it's found.
+  GDataDB::Status status = gdata_db_->Put(file);
+  ASSERT_EQ(GDataDB::DB_OK, status);
+  TestGetFound(file);
+  // Check if the iterator works too.
+  const char* all_entries[] = {
+    "drive/file",
+  };
+  TestIter("", all_entries, arraysize(all_entries));
+
+  // Tweak the file proto to simulate an incompatible proto in the DB.
+  GDataFileProto file_proto;
+  file.ToProto(&file_proto);
+  file_proto.clear_upload_url();  // This will make FromProto() fail.
+  std::string serialized_proto;
+  file_proto.SerializeToString(&serialized_proto);
+  gdata_db_->PutRawForTesting("file_resource_id", serialized_proto);
+
+  // Check if the corruption is detected.
+  TestGetCorrupt(file);
+  // We should no longer be able to find the file by iteration.
+  TestIter("", NULL, 0);
+}
+
 }  // namespace gdata

chrome/browser/chromeos/gdata/gdata_leveldb.cc

@@ -139,8 +139,7 @@ GDataDB::Status GDataLevelDB::GetByResourceId(const std::string& resource_id,
   if (db_status.ok()) {
     DCHECK(!serialized_proto.empty());
     *entry = GDataEntry::FromProtoString(serialized_proto);
-    DCHECK(entry->get());
-    return DB_OK;
+    return entry->get() ? DB_OK : DB_CORRUPTION;
   }
   return GetStatus(db_status);
 }
@@ -176,6 +175,18 @@ scoped_ptr<GDataDBIter> GDataLevelDB::CreateIterator(const FilePath& path) {
       path));
 }
 
+GDataDB::Status GDataLevelDB::PutRawForTesting(
+    const std::string& resource_id,
+    const std::string& raw_value) {
+  const std::string resource_id_key = ResourceIdToKey(resource_id);
+  leveldb::Status db_status = level_db_->Put(
+      leveldb::WriteOptions(),
+      leveldb::Slice(resource_id_key),
+      leveldb::Slice(raw_value));
+
+  return GetStatus(db_status);
+}
+
 GDataLevelDBIter::GDataLevelDBIter(scoped_ptr<leveldb::Iterator> level_db_iter,
                                    GDataDB* db,
                                    const FilePath& path)
@@ -211,7 +222,8 @@ bool GDataLevelDBIter::GetNext(std::string* path,
 
   GDataDB::Status status =
       db_->GetByResourceId(level_db_iter_->value().ToString(), entry);
-  DCHECK_EQ(GDataDB::DB_OK, status);
+  if (status != GDataDB::DB_OK)
+    return false;
 
   key_slice.remove_prefix(sizeof(kPathPrefix) - 1);
   path->assign(key_slice.ToString());

chrome/browser/chromeos/gdata/gdata_leveldb.h

@@ -32,6 +32,8 @@ class GDataLevelDB : public GDataDB {
   virtual Status GetByPath(const FilePath& path,
                            scoped_ptr<GDataEntry>* file) OVERRIDE;
   virtual scoped_ptr<GDataDBIter> CreateIterator(const FilePath& path) OVERRIDE;
+  virtual Status PutRawForTesting(const std::string& resource_id,
+                                  const std::string& raw_value) OVERRIDE;
 
   // Returns |resource_id| for |path| by looking up path_db_.
   Status ResourceIdForPath(const FilePath& path, std::string* resource_id);