Store attachment crc in AttachmentStore

In this change:
- Add crc into Attachment class
- Rename Attachment's Create/CreateWithId to CreateNew/RestoreExisting.
- Refactor how AttachmentDownloader extracts and checks crc. Now it also
    needs to pass crc to Attachment construction
- Store crc in RecordMetadata
- Check crc when reading attachment from AttachmentStore.

BUG=417794
R=maniscalco@chromium.org

Review URL: https://codereview.chromium.org/710073003

Cr-Commit-Position: refs/heads/master@{#304123}

11 files changed, 188 insertions, 77 deletions

diff --git a/sync/internal_api/attachments/attachment_downloader_impl.cc b/sync/internal_api/attachments/attachment_downloader_impl.cc
index 7b73218..232b99e 100644
--- a/sync/internal_api/attachments/attachment_downloader_impl.cc
+++ b/sync/internal_api/attachments/attachment_downloader_impl.cc
@@ -4,14 +4,17 @@
 
 #include "sync/internal_api/public/attachments/attachment_downloader_impl.h"
 
+#include "base/base64.h"
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
+#include "base/sys_byteorder.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/http/http_util.h"
 #include "net/url_request/url_fetcher.h"
 #include "sync/internal_api/public/attachments/attachment_uploader_impl.h"
+#include "sync/internal_api/public/attachments/attachment_util.h"
 #include "sync/protocol/sync.pb.h"
 #include "url/gurl.h"
 
@@ -116,8 +119,8 @@ void AttachmentDownloaderImpl::OnGetTokenFailure(
        ++iter) {
     DownloadState* download_state = *iter;
     scoped_refptr<base::RefCountedString> null_attachment_data;
-    ReportResult(
-        *download_state, DOWNLOAD_TRANSIENT_ERROR, null_attachment_data);
+    ReportResult(*download_state, DOWNLOAD_TRANSIENT_ERROR,
+                 null_attachment_data, 0);
     DCHECK(state_map_.find(download_state->attachment_url) != state_map_.end());
     state_map_.erase(download_state->attachment_url);
   }
@@ -137,17 +140,24 @@ void AttachmentDownloaderImpl::OnURLFetchComplete(
 
   DownloadResult result = DOWNLOAD_TRANSIENT_ERROR;
   scoped_refptr<base::RefCountedString> attachment_data;
+  uint32_t attachment_crc32c = 0;
 
   const int response_code = source->GetResponseCode();
   if (response_code == net::HTTP_OK) {
     std::string data_as_string;
     source->GetResponseAsString(&data_as_string);
-    if (VerifyHashIfPresent(*source, data_as_string)) {
-      result = DOWNLOAD_SUCCESS;
-      attachment_data = base::RefCountedString::TakeString(&data_as_string);
-    } else {
-      // TODO(maniscalco): Test me!
+    attachment_data = base::RefCountedString::TakeString(&data_as_string);
+
+    attachment_crc32c = ComputeCrc32c(attachment_data);
+    uint32_t crc32c_from_headers = 0;
+    if (ExtractCrc32c(source->GetResponseHeaders(), &crc32c_from_headers) &&
+        attachment_crc32c != crc32c_from_headers) {
+      // Fail download only if there is useful crc32c in header and it doesn't
+      // match data. All other cases are fine. When crc32c is not in headers
+      // locally calculated one will be stored and used for further checks.
       result = DOWNLOAD_TRANSIENT_ERROR;
+    } else {
+      result = DOWNLOAD_SUCCESS;
     }
   } else if (response_code == net::HTTP_UNAUTHORIZED) {
     // Server tells us we've got a bad token so invalidate it.
@@ -163,7 +173,7 @@ void AttachmentDownloaderImpl::OnURLFetchComplete(
   } else if (response_code == net::URLFetcher::RESPONSE_CODE_INVALID) {
     result = DOWNLOAD_TRANSIENT_ERROR;
   }
-  ReportResult(download_state, result, attachment_data);
+  ReportResult(download_state, result, attachment_data, attachment_crc32c);
   state_map_.erase(iter);
 }
 
@@ -197,15 +207,16 @@ void AttachmentDownloaderImpl::RequestAccessToken(
 void AttachmentDownloaderImpl::ReportResult(
     const DownloadState& download_state,
     const DownloadResult& result,
-    const scoped_refptr<base::RefCountedString>& attachment_data) {
+    const scoped_refptr<base::RefCountedString>& attachment_data,
+    uint32_t attachment_crc32c) {
   std::vector<DownloadCallback>::const_iterator iter;
   for (iter = download_state.user_callbacks.begin();
        iter != download_state.user_callbacks.end();
        ++iter) {
     scoped_ptr<Attachment> attachment;
     if (result == DOWNLOAD_SUCCESS) {
-      attachment.reset(new Attachment(Attachment::CreateWithId(
-          download_state.attachment_id, attachment_data)));
+      attachment.reset(new Attachment(Attachment::CreateFromParts(
+          download_state.attachment_id, attachment_data, attachment_crc32c)));
     }
 
     base::MessageLoop::current()->PostTask(
@@ -213,51 +224,44 @@ void AttachmentDownloaderImpl::ReportResult(
   }
 }
 
-bool AttachmentDownloaderImpl::VerifyHashIfPresent(
-    const net::URLFetcher& fetcher,
-    const std::string& data) {
-  const net::HttpResponseHeaders* headers = fetcher.GetResponseHeaders();
+bool AttachmentDownloaderImpl::ExtractCrc32c(
+    const net::HttpResponseHeaders* headers,
+    uint32_t* crc32c) {
+  DCHECK(crc32c);
   if (!headers) {
-    // No headers?  It passes.
-    return true;
-  }
-
-  std::string value;
-  if (!ExtractCrc32c(*headers, &value)) {
-    // No crc32c?  It passes.
-    return true;
-  }
-
-  if (value ==
-      AttachmentUploaderImpl::ComputeCrc32cHash(data.data(), data.size())) {
-    return true;
-  } else {
     return false;
   }
-}
 
-bool AttachmentDownloaderImpl::ExtractCrc32c(
-    const net::HttpResponseHeaders& headers,
-    std::string* crc32c) {
-  DCHECK(crc32c);
+  std::string crc32c_encoded;
   std::string header_value;
   void* iter = NULL;
   // Iterate over all matching headers.
-  while (headers.EnumerateHeader(&iter, "x-goog-hash", &header_value)) {
+  while (headers->EnumerateHeader(&iter, "x-goog-hash", &header_value)) {
     // Because EnumerateHeader is smart about list values, header_value will
     // either be empty or a single name=value pair.
     net::HttpUtil::NameValuePairsIterator pair_iter(
         header_value.begin(), header_value.end(), ',');
     if (pair_iter.GetNext()) {
       if (pair_iter.name() == "crc32c") {
-        *crc32c = pair_iter.value();
+        crc32c_encoded = pair_iter.value();
         DCHECK(!pair_iter.GetNext());
-        return true;
+        break;
       }
     }
   }
+  // Check if header was found
+  if (crc32c_encoded.empty())
+    return false;
+  std::string crc32c_raw;
+  if (!base::Base64Decode(crc32c_encoded, &crc32c_raw))
+    return false;
+
+  if (crc32c_raw.size() != sizeof(*crc32c))
+    return false;
 
-  return false;
+  *crc32c =
+      base::NetToHost32(*reinterpret_cast<const uint32_t*>(crc32c_raw.c_str()));
+  return true;
 }
 
 }  // namespace syncer
diff --git a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
index 0f8aa8f..f36fbad 100644
--- a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
+++ b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
@@ -16,7 +16,9 @@
 #include "net/url_request/url_request_test_util.h"
 #include "sync/api/attachments/attachment.h"
 #include "sync/internal_api/public/attachments/attachment_uploader_impl.h"
+#include "sync/internal_api/public/attachments/attachment_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/leveldatabase/src/util/crc32c.h"
 
 namespace syncer {
 
@@ -278,8 +280,8 @@ void AttachmentDownloaderImplTest::AddHashHeader(
     case HASH_HEADER_NONE:
       break;
     case HASH_HEADER_VALID:
-      header += AttachmentUploaderImpl::ComputeCrc32cHash(
-          kAttachmentContent, strlen(kAttachmentContent));
+      header += AttachmentUploaderImpl::FormatCrc32cHash(leveldb::crc32c::Value(
+          kAttachmentContent, strlen(kAttachmentContent)));
       headers->AddHeader(header);
       break;
     case HASH_HEADER_INVALID:
@@ -419,6 +421,11 @@ TEST_F(AttachmentDownloaderImplTest, InvalidHash) {
   VerifyDownloadResult(id1, AttachmentDownloader::DOWNLOAD_TRANSIENT_ERROR);
 }
 
+// Verify that extract fails when there is no headers object.
+TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_NoHeaders) {
+  uint32_t extracted;
+  ASSERT_FALSE(AttachmentDownloaderImpl::ExtractCrc32c(nullptr, &extracted));
+}
 
 // Verify that extract fails when there is no crc32c value.
 TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_Empty) {
@@ -430,31 +437,50 @@ TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_Empty) {
   std::replace(raw.begin(), raw.end(), '\n', '\0');
   scoped_refptr<net::HttpResponseHeaders> headers(
       new net::HttpResponseHeaders(raw));
-  std::string extracted;
-  ASSERT_FALSE(AttachmentDownloaderImpl::ExtractCrc32c(*headers, &extracted));
+  uint32_t extracted;
+  ASSERT_FALSE(
+      AttachmentDownloaderImpl::ExtractCrc32c(headers.get(), &extracted));
 }
 
 // Verify that extract finds the first crc32c and ignores others.
 TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_First) {
-  const std::string expected = "z8SuHQ==";
+  const std::string expected_encoded = "z8SuHQ==";
+  const uint32_t expected = 3485773341;
   std::string raw;
   raw += "HTTP/1.1 200 OK\n";
   raw += "Foo: bar\n";
   // Ignored because it's the wrong header.
   raw += "X-Goog-Hashes: crc32c=AAAAAA==\n";
   // Header name matches.  The md5 item is ignored.
-  raw += "X-Goog-HASH: md5=rL0Y20zC+Fzt72VPzMSk2A==,crc32c=" + expected + "\n";
+  raw += "X-Goog-HASH: md5=rL0Y20zC+Fzt72VPzMSk2A==,crc32c=" +
+         expected_encoded + "\n";
   // Ignored because we already found a crc32c in the one above.
   raw += "X-Goog-HASH: crc32c=AAAAAA==\n";
   raw += "\n";
   std::replace(raw.begin(), raw.end(), '\n', '\0');
   scoped_refptr<net::HttpResponseHeaders> headers(
       new net::HttpResponseHeaders(raw));
-  std::string extracted;
-  ASSERT_TRUE(AttachmentDownloaderImpl::ExtractCrc32c(*headers, &extracted));
+  uint32_t extracted;
+  ASSERT_TRUE(
+      AttachmentDownloaderImpl::ExtractCrc32c(headers.get(), &extracted));
   ASSERT_EQ(expected, extracted);
 }
 
+// Verify that extract fails when encoded value is too long.
+TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_TooLong) {
+  std::string raw;
+  raw += "HTTP/1.1 200 OK\n";
+  raw += "Foo: bar\n";
+  raw += "X-Goog-HASH: crc32c=AAAAAAAA\n";
+  raw += "\n";
+  std::replace(raw.begin(), raw.end(), '\n', '\0');
+  scoped_refptr<net::HttpResponseHeaders> headers(
+      new net::HttpResponseHeaders(raw));
+  uint32_t extracted;
+  ASSERT_FALSE(
+      AttachmentDownloaderImpl::ExtractCrc32c(headers.get(), &extracted));
+}
+
 // Verify that extract fails if there is no crc32c.
 TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_None) {
   std::string raw;
@@ -465,8 +491,9 @@ TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_None) {
   std::replace(raw.begin(), raw.end(), '\n', '\0');
   scoped_refptr<net::HttpResponseHeaders> headers(
       new net::HttpResponseHeaders(raw));
-  std::string extracted;
-  ASSERT_FALSE(AttachmentDownloaderImpl::ExtractCrc32c(*headers, &extracted));
+  uint32_t extracted;
+  ASSERT_FALSE(
+      AttachmentDownloaderImpl::ExtractCrc32c(headers.get(), &extracted));
 }
 
 }  // namespace syncer
diff --git a/sync/internal_api/attachments/attachment_service_impl_unittest.cc b/sync/internal_api/attachments/attachment_service_impl_unittest.cc
index 857514a..1d6c8a6 100644
--- a/sync/internal_api/attachments/attachment_service_impl_unittest.cc
+++ b/sync/internal_api/attachments/attachment_service_impl_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/timer/mock_timer.h"
+#include "sync/internal_api/public/attachments/attachment_util.h"
 #include "sync/internal_api/public/attachments/fake_attachment_downloader.h"
 #include "sync/internal_api/public/attachments/fake_attachment_uploader.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
@@ -55,7 +56,9 @@ class MockAttachmentStore : public AttachmentStore,
     for (AttachmentIdList::const_iterator iter = ids.begin(); iter != ids.end();
          ++iter) {
       if (local_attachments.find(*iter) != local_attachments.end()) {
-        Attachment attachment = Attachment::CreateWithId(*iter, data);
+        uint32_t crc32c = ComputeCrc32c(data);
+        Attachment attachment =
+            Attachment::CreateFromParts(*iter, data, crc32c);
         attachments->insert(std::make_pair(*iter, attachment));
       } else {
         unavailable_attachments->push_back(*iter);
@@ -112,7 +115,9 @@ class MockAttachmentDownloader
     scoped_ptr<Attachment> attachment;
     if (result == DOWNLOAD_SUCCESS) {
       scoped_refptr<base::RefCountedString> data = new base::RefCountedString();
-      attachment.reset(new Attachment(Attachment::CreateWithId(id, data)));
+      uint32_t crc32c = ComputeCrc32c(data);
+      attachment.reset(
+          new Attachment(Attachment::CreateFromParts(id, data, crc32c)));
     }
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
diff --git a/sync/internal_api/attachments/attachment_store_test_template.h b/sync/internal_api/attachments/attachment_store_test_template.h
index 1d9bd05..cd1c016 100644
--- a/sync/internal_api/attachments/attachment_store_test_template.h
+++ b/sync/internal_api/attachments/attachment_store_test_template.h
@@ -14,6 +14,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/thread_task_runner_handle.h"
 #include "sync/api/attachments/attachment.h"
+#include "sync/internal_api/public/attachments/attachment_util.h"
 #include "sync/protocol/sync.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -116,8 +117,9 @@ TYPED_TEST_CASE_P(AttachmentStoreTest);
 TYPED_TEST_P(AttachmentStoreTest, Write_NoOverwriteNoError) {
   // Create two attachments with the same id but different data.
   Attachment attachment1 = Attachment::Create(this->some_data1);
-  Attachment attachment2 =
-      Attachment::CreateWithId(attachment1.GetId(), this->some_data2);
+  uint32_t crc32c = ComputeCrc32c(this->some_data2);
+  Attachment attachment2 = Attachment::CreateFromParts(
+      attachment1.GetId(), this->some_data2, crc32c);
 
   // Write the first one.
   AttachmentList some_attachments;
diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc
index 7333596..2f14263 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl.cc
@@ -20,7 +20,6 @@
 #include "net/url_request/url_fetcher_delegate.h"
 #include "sync/api/attachments/attachment.h"
 #include "sync/protocol/sync.pb.h"
-#include "third_party/leveldatabase/src/util/crc32c.h"
 
 namespace {
 
@@ -207,12 +206,9 @@ void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess(
   fetcher_->SetUploadData(kContentType, upload_content);
   const std::string auth_header("Authorization: Bearer " + access_token_);
   fetcher_->AddExtraRequestHeader(auth_header);
-  // TODO(maniscalco): Consider computing the hash once and storing the value as
-  // a new field in the Attachment object to avoid recomputing when an upload
-  // fails and is retried (bug 417794).
+  const uint32_t crc32c = attachment_.GetCrc32c();
   fetcher_->AddExtraRequestHeader(base::StringPrintf(
-      "X-Goog-Hash: crc32c=%s",
-      ComputeCrc32cHash(memory->front_as<char>(), memory->size()).c_str()));
+      "X-Goog-Hash: crc32c=%s", FormatCrc32cHash(crc32c).c_str()));
   fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
                          net::LOAD_DO_NOT_SEND_COOKIES |
                          net::LOAD_DISABLE_CACHE);
@@ -345,10 +341,8 @@ void AttachmentUploaderImpl::OnUploadStateStopped(const UniqueId& unique_id) {
   }
 }
 
-std::string AttachmentUploaderImpl::ComputeCrc32cHash(const char* data,
-                                                      size_t size) {
-  const uint32_t crc32c_big_endian =
-      base::HostToNet32(leveldb::crc32c::Value(data, size));
+std::string AttachmentUploaderImpl::FormatCrc32cHash(uint32_t crc32c) {
+  const uint32_t crc32c_big_endian = base::HostToNet32(crc32c);
   const base::StringPiece raw(reinterpret_cast<const char*>(&crc32c_big_endian),
                         sizeof(crc32c_big_endian));
   std::string encoded;
diff --git a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
index 1213b9f..d31fc18 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
@@ -23,6 +23,7 @@
 #include "net/test/embedded_test_server/http_response.h"
 #include "net/url_request/url_request_test_util.h"
 #include "sync/api/attachments/attachment.h"
+#include "sync/internal_api/public/attachments/attachment_util.h"
 #include "sync/protocol/sync.pb.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -627,18 +628,16 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_BadToken) {
   ASSERT_EQ(1, token_service().num_invalidate_token());
 }
 
-TEST_F(AttachmentUploaderImplTest, ComputeCrc32cHash) {
+TEST_F(AttachmentUploaderImplTest, FormatCrc32cHash) {
   scoped_refptr<base::RefCountedString> empty(new base::RefCountedString);
   empty->data() = "";
   EXPECT_EQ("AAAAAA==",
-            AttachmentUploaderImpl::ComputeCrc32cHash(empty->front_as<char>(),
-                                                      empty->size()));
+            AttachmentUploaderImpl::FormatCrc32cHash(ComputeCrc32c(empty)));
 
   scoped_refptr<base::RefCountedString> hello_world(new base::RefCountedString);
   hello_world->data() = "hello world";
-  EXPECT_EQ("yZRlqg==",
-            AttachmentUploaderImpl::ComputeCrc32cHash(
-                hello_world->front_as<char>(), hello_world->size()));
+  EXPECT_EQ("yZRlqg==", AttachmentUploaderImpl::FormatCrc32cHash(
+                            ComputeCrc32c(hello_world)));
 }
 
 // TODO(maniscalco): Add test case for when we are uploading an attachment that
diff --git a/sync/internal_api/attachments/attachment_util.cc b/sync/internal_api/attachments/attachment_util.cc
new file mode 100644
index 0000000..08d7f99
--- /dev/null
+++ b/sync/internal_api/attachments/attachment_util.cc
@@ -0,0 +1,16 @@
+// Copyright 2014 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 "sync/internal_api/public/attachments/attachment_util.h"
+
+#include "base/memory/ref_counted_memory.h"
+#include "third_party/leveldatabase/src/util/crc32c.h"
+
+namespace syncer {
+
+uint32_t ComputeCrc32c(const scoped_refptr<base::RefCountedMemory>& data) {
+  return leveldb::crc32c::Value(data->front_as<char>(), data->size());
+}
+
+}  // namespace syncer
diff --git a/sync/internal_api/attachments/fake_attachment_downloader.cc b/sync/internal_api/attachments/fake_attachment_downloader.cc
index 1f2795e..53abc61 100644
--- a/sync/internal_api/attachments/fake_attachment_downloader.cc
+++ b/sync/internal_api/attachments/fake_attachment_downloader.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
+#include "sync/internal_api/public/attachments/attachment_util.h"
 
 namespace syncer {
 
@@ -24,8 +25,9 @@ void FakeAttachmentDownloader::DownloadAttachment(
   // attachment.
   scoped_refptr<base::RefCountedMemory> data(new base::RefCountedBytes());
   scoped_ptr<Attachment> attachment;
+  const uint32_t crc32c = ComputeCrc32c(data);
   attachment.reset(
-      new Attachment(Attachment::CreateWithId(attachment_id, data)));
+      new Attachment(Attachment::CreateFromParts(attachment_id, data, crc32c)));
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(callback, DOWNLOAD_SUCCESS, base::Passed(&attachment)));
diff --git a/sync/internal_api/attachments/on_disk_attachment_store.cc b/sync/internal_api/attachments/on_disk_attachment_store.cc
index 25ad186..9af3104 100644
--- a/sync/internal_api/attachments/on_disk_attachment_store.cc
+++ b/sync/internal_api/attachments/on_disk_attachment_store.cc
@@ -10,6 +10,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/sequenced_task_runner.h"
 #include "sync/internal_api/attachments/proto/attachment_store.pb.h"
+#include "sync/internal_api/public/attachments/attachment_util.h"
 #include "sync/protocol/attachments.pb.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/options.h"
@@ -211,16 +212,35 @@ scoped_ptr<Attachment> OnDiskAttachmentStore::ReadSingleAttachment(
   scoped_ptr<Attachment> attachment;
 
   const std::string key = MakeDataKeyFromAttachmentId(attachment_id);
+  const std::string metadata_key =
+      MakeMetadataKeyFromAttachmentId(attachment_id);
+  leveldb::Status status;
+  std::string metadata_str;
+  status = db_->Get(MakeMetadataReadOptions(), metadata_key, &metadata_str);
+  if (!status.ok()) {
+    DVLOG(1) << "DB::Get for metadata failed: status=" << status.ToString();
+    return attachment.Pass();
+  }
+  attachment_store_pb::RecordMetadata record_metadata;
+  if (!record_metadata.ParseFromString(metadata_str)) {
+    DVLOG(1) << "RecordMetadata::ParseFromString failed";
+    return attachment.Pass();
+  }
   std::string data_str;
-  leveldb::Status status = db_->Get(MakeDataReadOptions(), key, &data_str);
-  if (status.ok()) {
-    scoped_refptr<base::RefCountedMemory> data =
-        base::RefCountedString::TakeString(&data_str);
-    attachment.reset(
-        new Attachment(Attachment::CreateWithId(attachment_id, data)));
-  } else {
-    DVLOG(1) << "DB::Get failed: status=" << status.ToString();
+  status = db_->Get(MakeDataReadOptions(), key, &data_str);
+  if (!status.ok()) {
+    DVLOG(1) << "DB::Get for data failed: status=" << status.ToString();
+    return attachment.Pass();
+  }
+  scoped_refptr<base::RefCountedMemory> data =
+      base::RefCountedString::TakeString(&data_str);
+  uint32_t crc32c = ComputeCrc32c(data);
+  if (record_metadata.has_crc32c() && record_metadata.crc32c() != crc32c) {
+    DVLOG(1) << "Attachment crc does not match";
+    return attachment.Pass();
   }
+  attachment.reset(
+      new Attachment(Attachment::CreateFromParts(attachment_id, data, crc32c)));
   return attachment.Pass();
 }
 
@@ -247,6 +267,7 @@ bool OnDiskAttachmentStore::WriteSingleAttachment(
   // Write metadata.
   attachment_store_pb::RecordMetadata metadata;
   metadata.set_attachment_size(attachment.GetData()->size());
+  metadata.set_crc32c(attachment.GetCrc32c());
   metadata_str = metadata.SerializeAsString();
   write_batch.Put(metadata_key, metadata_str);
   // Write data.
diff --git a/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc b/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc
index 0b0bd54..b33a983 100644
--- a/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc
+++ b/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc
@@ -266,6 +266,8 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
   EXPECT_EQ(store_.get(), nullptr);
 }
 
+// Ensure that attachment store correctly maintains metadata records for
+// attachments.
 TEST_F(OnDiskAttachmentStoreSpecificTest, RecordMetadata) {
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   base::FilePath db_path =
@@ -313,4 +315,41 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, RecordMetadata) {
   VerifyAttachmentRecordsPresent(db_path, attachments[1].GetId(), true);
 }
 
+// Ensure that attachment store fails to load attachment with mismatched crc.
+TEST_F(OnDiskAttachmentStoreSpecificTest, MismatchedCrc) {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+
+  // Create attachment store.
+  AttachmentStore::Result result = AttachmentStore::UNSPECIFIED_ERROR;
+  AttachmentStore::CreateOnDiskStore(
+      temp_dir_.path(), base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&AttachmentStoreCreated, &store_, &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Write attachment with incorrect crc32c.
+  const uint32_t intentionally_wrong_crc32c = 0;
+  std::string some_data("data1");
+  Attachment attachment = Attachment::CreateFromParts(
+      AttachmentId::Create(), base::RefCountedString::TakeString(&some_data),
+      intentionally_wrong_crc32c);
+  AttachmentList attachments;
+  attachments.push_back(attachment);
+  store_->Write(attachments,
+                base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResult,
+                           base::Unretained(this), &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Read attachment.
+  AttachmentIdList attachment_ids;
+  attachment_ids.push_back(attachment.GetId());
+  store_->Read(
+      attachment_ids,
+      base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResultAttachments,
+                 base::Unretained(this), &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/attachments/proto/attachment_store.proto b/sync/internal_api/attachments/proto/attachment_store.proto
index 86f9071..a05e1c2 100644
--- a/sync/internal_api/attachments/proto/attachment_store.proto
+++ b/sync/internal_api/attachments/proto/attachment_store.proto
@@ -24,4 +24,6 @@ message StoreMetadata {
 message RecordMetadata {
   // Size of attachment data. Useful for attachment store space management.
   optional int64 attachment_size = 1;
+  // Crc32c of attachment data.
+  optional fixed32 crc32c = 2;
 }