SimpleCache: open with fewer seeks.

net/disk_cache/simple/simple_synchronous_entry.cc

Index: net/disk_cache/simple/simple_synchronous_entry.cc
diff --git a/net/disk_cache/simple/simple_synchronous_entry.cc b/net/disk_cache/simple/simple_synchronous_entry.cc
index 864c05ac8ac7263cb69726376b0c4827f8b96004..53fff2a182a982499f2f403b35a992530096259f 100644
--- a/net/disk_cache/simple/simple_synchronous_entry.cc
+++ b/net/disk_cache/simple/simple_synchronous_entry.cc
@@ -18,6 +18,8 @@
 #include "base/sha1.h"
 #include "base/strings/stringprintf.h"
 #include "base/timer/elapsed_timer.h"
+#include "crypto/secure_hash.h"
+#include "net/base/hash_value.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/simple/simple_backend_version.h"
@@ -39,7 +41,7 @@ enum OpenEntryResult {
   OPEN_ENTRY_BAD_MAGIC_NUMBER = 3,
   OPEN_ENTRY_BAD_VERSION = 4,
   OPEN_ENTRY_CANT_READ_KEY = 5,
-  // OPEN_ENTRY_KEY_MISMATCH = 6, Deprecated.
+  OPEN_ENTRY_KEY_MISMATCH = 6,
   OPEN_ENTRY_KEY_HASH_MISMATCH = 7,
   OPEN_ENTRY_SPARSE_OPEN_FAILED = 8,
   OPEN_ENTRY_MAX = 9,
@@ -63,6 +65,7 @@ enum CheckEOFResult {
   CHECK_EOF_RESULT_READ_FAILURE,
   CHECK_EOF_RESULT_MAGIC_NUMBER_MISMATCH,
   CHECK_EOF_RESULT_CRC_MISMATCH,
+  CHECK_EOF_RESULT_KEY_SHA256_MISMATCH,
   CHECK_EOF_RESULT_MAX,
 };
 
@@ -72,6 +75,9 @@ enum CloseResult {
   CLOSE_RESULT_WRITE_FAILURE,
 };
 
+// Used in histograms, please only add entries at the end.
+enum class KeySHA256Result { NOT_PRESENT, MATCHED, NO_MATCH, MAX };
+
 void RecordSyncOpenResult(net::CacheType cache_type,
                           OpenEntryResult result,
                           bool had_index) {
@@ -105,6 +111,12 @@ void RecordCloseResult(net::CacheType cache_type, CloseResult result) {
                    "SyncCloseResult", cache_type, result, WRITE_RESULT_MAX);
 }
 
+void RecordKeySHA256Result(net::CacheType cache_type, KeySHA256Result result) {
+  SIMPLE_CACHE_UMA(ENUMERATION, "SyncKeySHA256Result", cache_type,
+                   static_cast<int>(result),
+                   static_cast<int>(KeySHA256Result::MAX));
+}
+
 bool CanOmitEmptyFile(int file_index) {
   DCHECK_GE(file_index, 0);
   DCHECK_LT(file_index, disk_cache::kSimpleEntryFileCount);
@@ -123,6 +135,15 @@ bool TruncatePath(const FilePath& filename_to_truncate) {
   return true;
 }
 
+void CalculateSHA256OfKey(const std::string& key,
+                          net::SHA256HashValue* out_hash_value) {
+  // Write SHA256 of the key immediately after stream 0 data.
+  std::unique_ptr<crypto::SecureHash> hash(
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256));
+  hash->Update(key.data(), key.size());
+  hash->Finish(out_hash_value, sizeof(*out_hash_value));
+}
+
 }  // namespace
 
 namespace disk_cache {
@@ -130,8 +151,9 @@ namespace disk_cache {
 using simple_util::GetEntryHashKey;
 using simple_util::GetFilenameFromEntryHashAndFileIndex;
 using simple_util::GetSparseFilenameFromEntryHash;
-using simple_util::GetDataSizeFromKeyAndFileSize;
-using simple_util::GetFileSizeFromKeyAndDataSize;
+using simple_util::GetHeaderSize;
+using simple_util::GetDataSizeFromFileSize;
+using simple_util::GetFileSizeFromDataSize;
 using simple_util::GetFileIndexFromStreamIndex;
 
 SimpleEntryStat::SimpleEntryStat(base::Time last_used,
@@ -144,35 +166,42 @@ SimpleEntryStat::SimpleEntryStat(base::Time last_used,
   memcpy(data_size_, data_size, sizeof(data_size_));
 }
 
-int SimpleEntryStat::GetOffsetInFile(const std::string& key,
+int SimpleEntryStat::GetOffsetInFile(size_t key_length,
                                      int offset,
                                      int stream_index) const {
-  const size_t headers_size = sizeof(SimpleFileHeader) + key.size();
+  const size_t headers_size = sizeof(SimpleFileHeader) + key_length;
   const size_t additional_offset =
       stream_index == 0 ? data_size_[1] + sizeof(SimpleFileEOF) : 0;
   return headers_size + offset + additional_offset;
 }
 
-int SimpleEntryStat::GetEOFOffsetInFile(const std::string& key,
+int SimpleEntryStat::GetEOFOffsetInFile(size_t key_length,
                                         int stream_index) const {
-  return GetOffsetInFile(key, data_size_[stream_index], stream_index);
+  size_t additional_offset;
+  if (stream_index != 0)
+    additional_offset = 0;
+  else
+    additional_offset = sizeof(net::SHA256HashValue);
+  return additional_offset +
+         GetOffsetInFile(key_length, data_size_[stream_index], stream_index);
 }
 
-int SimpleEntryStat::GetLastEOFOffsetInFile(const std::string& key,
+int SimpleEntryStat::GetLastEOFOffsetInFile(size_t key_length,
                                             int stream_index) const {
-  const int file_index = GetFileIndexFromStreamIndex(stream_index);
-  const int eof_data_offset =
-      file_index == 0 ? data_size_[0] + data_size_[1] + sizeof(SimpleFileEOF)
-                      : data_size_[2];
-  return GetOffsetInFile(key, eof_data_offset, stream_index);
+  if (stream_index == 1)
+    return GetEOFOffsetInFile(key_length, 0);
+  return GetEOFOffsetInFile(key_length, stream_index);
 }
 
-int64_t SimpleEntryStat::GetFileSize(const std::string& key,
-                                     int file_index) const {
-  const int32_t total_data_size =
-      file_index == 0 ? data_size_[0] + data_size_[1] + sizeof(SimpleFileEOF)
-                      : data_size_[2];
-  return GetFileSizeFromKeyAndDataSize(key, total_data_size);
+int64_t SimpleEntryStat::GetFileSize(size_t key_length, int file_index) const {
+  int32_t total_data_size;

[Randy Smith (Not in Mondays), 2016/05/19 19:38:50]

(Suggestion for a followup CL; this problem predates this CL)

I find the use of the "data size" concept in this function confusing;
GetFileSizeFromDataSize() assumes that there's only a single stream in a file,
and that's compensated for here by including in total_data_size the extra EOF
record (and hash value, which is new) that's implied by the two stream file
case.  In my ideal world the variables and would be named in some way that makes
that distinction clearer, possibly with a re-jigger of responsibility between
functions.  I'm not going to make specific suggestions since I don't think the
fix belongs in this CL, but I wanted to point out another place where I did more
tripping over figuring out what was going on than I'd ideally like to have in
Chromium code.

[gavinp, 2016/05/20 01:42:32]

I agree. this has been a problem for a long time.

+  if (file_index == 0) {
+    total_data_size = data_size_[0] + data_size_[1] +
+                      sizeof(net::SHA256HashValue) + sizeof(SimpleFileEOF);
+  } else {
+    total_data_size = data_size_[2];
+  }
+  return GetFileSizeFromDataSize(key_length, total_data_size);
 }
 
 SimpleEntryCreationResults::SimpleEntryCreationResults(
@@ -223,17 +252,16 @@ SimpleSynchronousEntry::EntryOperationData::EntryOperationData(
 void SimpleSynchronousEntry::OpenEntry(
     net::CacheType cache_type,
     const FilePath& path,
+    const std::string& key,
     const uint64_t entry_hash,
-    bool had_index,
+    const bool had_index,
     SimpleEntryCreationResults* out_results) {
   base::ElapsedTimer open_time;
   SimpleSynchronousEntry* sync_entry =
-      new SimpleSynchronousEntry(cache_type, path, "", entry_hash);
-  out_results->result =
-      sync_entry->InitializeForOpen(had_index,
-                                    &out_results->entry_stat,
-                                    &out_results->stream_0_data,
-                                    &out_results->stream_0_crc32);
+      new SimpleSynchronousEntry(cache_type, path, key, entry_hash, had_index);
+  out_results->result = sync_entry->InitializeForOpen(
+      &out_results->entry_stat, &out_results->stream_0_data,
+      &out_results->stream_0_crc32);
   if (out_results->result != net::OK) {
     sync_entry->Doom();
     delete sync_entry;
@@ -251,13 +279,13 @@ void SimpleSynchronousEntry::CreateEntry(
     const FilePath& path,
     const std::string& key,
     const uint64_t entry_hash,
-    bool had_index,
+    const bool had_index,
     SimpleEntryCreationResults* out_results) {
   DCHECK_EQ(entry_hash, GetEntryHashKey(key));
   SimpleSynchronousEntry* sync_entry =
-      new SimpleSynchronousEntry(cache_type, path, key, entry_hash);
-  out_results->result = sync_entry->InitializeForCreate(
-      had_index, &out_results->entry_stat);
+      new SimpleSynchronousEntry(cache_type, path, key, entry_hash, had_index);
+  out_results->result =
+      sync_entry->InitializeForCreate(&out_results->entry_stat);
   if (out_results->result != net::OK) {
     if (out_results->result != net::ERR_FILE_EXISTS)
       sync_entry->Doom();
@@ -298,19 +326,24 @@ void SimpleSynchronousEntry::ReadData(const EntryOperationData& in_entry_op,
                                       net::IOBuffer* out_buf,
                                       uint32_t* out_crc32,
                                       SimpleEntryStat* entry_stat,
-                                      int* out_result) const {
+                                      int* out_result) {
   DCHECK(initialized_);
   DCHECK_NE(0, in_entry_op.index);
-  const int64_t file_offset =
-      entry_stat->GetOffsetInFile(key_, in_entry_op.offset, in_entry_op.index);
   int file_index = GetFileIndexFromStreamIndex(in_entry_op.index);
+  if (header_and_key_check_needed_[file_index] &&
+      !CheckHeaderAndKey(file_index)) {
+    *out_result = net::ERR_FAILED;
+    Doom();
+    return;
+  }
+  const int64_t file_offset = entry_stat->GetOffsetInFile(
+      key_.size(), in_entry_op.offset, in_entry_op.index);
   // Zero-length reads and reads to the empty streams of omitted files should
   // be handled in the SimpleEntryImpl.
   DCHECK_GT(in_entry_op.buf_len, 0);
   DCHECK(!empty_file_omitted_[file_index]);
-  File* file = const_cast<File*>(&files_[file_index]);
-  int bytes_read =
-      file->Read(file_offset, out_buf->data(), in_entry_op.buf_len);
+  int bytes_read = files_[file_index].Read(file_offset, out_buf->data(),
+                                           in_entry_op.buf_len);
   if (bytes_read > 0) {
     entry_stat->set_last_used(Time::Now());
     *out_crc32 = crc32(crc32(0L, Z_NULL, 0),
@@ -333,12 +366,18 @@ void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
   DCHECK_NE(0, in_entry_op.index);
   int index = in_entry_op.index;
   int file_index = GetFileIndexFromStreamIndex(index);
+  if (header_and_key_check_needed_[file_index] &&
+      !empty_file_omitted_[file_index] && !CheckHeaderAndKey(file_index)) {
+    *out_result = net::ERR_FAILED;
+    Doom();
+    return;
+  }
   int offset = in_entry_op.offset;
   int buf_len = in_entry_op.buf_len;
   bool truncate = in_entry_op.truncate;
   bool doomed = in_entry_op.doomed;
   const int64_t file_offset = out_entry_stat->GetOffsetInFile(
-      key_, in_entry_op.offset, in_entry_op.index);
+      key_.size(), in_entry_op.offset, in_entry_op.index);
   bool extending_by_write = offset + buf_len > out_entry_stat->data_size(index);
 
   if (empty_file_omitted_[file_index]) {
@@ -371,7 +410,7 @@ void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
   if (extending_by_write) {
     // The EOF record and the eventual stream afterward need to be zeroed out.
     const int64_t file_eof_offset =
-        out_entry_stat->GetEOFOffsetInFile(key_, index);
+        out_entry_stat->GetEOFOffsetInFile(key_.size(), index);
     if (!files_[file_index].SetLength(file_eof_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_PRETRUNCATE_FAILURE);
       Doom();
@@ -393,7 +432,8 @@ void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
         index, std::max(out_entry_stat->data_size(index), offset + buf_len));
   } else {
     out_entry_stat->set_data_size(index, offset + buf_len);
-    int file_eof_offset = out_entry_stat->GetLastEOFOffsetInFile(key_, index);
+    int file_eof_offset =
+        out_entry_stat->GetLastEOFOffsetInFile(key_.size(), index);
     if (!files_[file_index].SetLength(file_eof_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_TRUNCATE_FAILURE);
       Doom();
@@ -621,9 +661,10 @@ void SimpleSynchronousEntry::CheckEOFRecord(int index,
   DCHECK(initialized_);
   uint32_t crc32;
   bool has_crc32;
+  bool has_key_sha256;
   int stream_size;
-  *out_result =
-      GetEOFRecordData(index, entry_stat, &has_crc32, &crc32, &stream_size);
+  *out_result = GetEOFRecordData(index, entry_stat, &has_crc32, &has_key_sha256,
+                                 &crc32, &stream_size);
   if (*out_result != net::OK) {
     Doom();
     return;
@@ -644,7 +685,7 @@ void SimpleSynchronousEntry::Close(
     net::GrowableIOBuffer* stream_0_data) {
   DCHECK(stream_0_data);
   // Write stream 0 data.
-  int stream_0_offset = entry_stat.GetOffsetInFile(key_, 0, 0);
+  int stream_0_offset = entry_stat.GetOffsetInFile(key_.size(), 0, 0);
   if (files_[0].Write(stream_0_offset, stream_0_data->data(),
                       entry_stat.data_size(0)) !=
       entry_stat.data_size(0)) {
@@ -652,6 +693,15 @@ void SimpleSynchronousEntry::Close(
     DVLOG(1) << "Could not write stream 0 data.";
     Doom();
   }
+  net::SHA256HashValue hash_value;
+  CalculateSHA256OfKey(key_, &hash_value);
+  if (files_[0].Write(stream_0_offset + entry_stat.data_size(0),
+                      reinterpret_cast<char*>(hash_value.data),
+                      sizeof(hash_value)) != sizeof(hash_value)) {
+    RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
+    DVLOG(1) << "Could not write stream 0 data.";
+    Doom();
+  }
 
   for (std::vector<CRCRecord>::const_iterator it = crc32s_to_write->begin();
        it != crc32s_to_write->end(); ++it) {
@@ -666,8 +716,10 @@ void SimpleSynchronousEntry::Close(
     eof_record.flags = 0;
     if (it->has_crc32)
       eof_record.flags |= SimpleFileEOF::FLAG_HAS_CRC32;
+    if (stream_index == 0)
+      eof_record.flags |= SimpleFileEOF::FLAG_HAS_KEY_SHA256;
     eof_record.data_crc32 = it->data_crc32;
-    int eof_offset = entry_stat.GetEOFOffsetInFile(key_, stream_index);
+    int eof_offset = entry_stat.GetEOFOffsetInFile(key_.size(), stream_index);
     // If stream 0 changed size, the file needs to be resized, otherwise the
     // next open will yield wrong stream sizes. On stream 1 and stream 2 proper
     // resizing of the file is handled in SimpleSynchronousEntry::WriteData().
@@ -692,8 +744,11 @@ void SimpleSynchronousEntry::Close(
     if (empty_file_omitted_[i])
       continue;
 
+    if (header_and_key_check_needed_[i] && !CheckHeaderAndKey(i)) {
+      Doom();
+    }
     files_[i].Close();
-    const int64_t file_size = entry_stat.GetFileSize(key_, i);
+    const int64_t file_size = entry_stat.GetFileSize(key_.size(), i);
     SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                      "LastClusterSize", cache_type_,
                      file_size % 4096, 0, 4097, 50);
@@ -720,10 +775,12 @@ void SimpleSynchronousEntry::Close(
 SimpleSynchronousEntry::SimpleSynchronousEntry(net::CacheType cache_type,
                                                const FilePath& path,
                                                const std::string& key,
-                                               const uint64_t entry_hash)
+                                               const uint64_t entry_hash,
+                                               const bool had_index)
     : cache_type_(cache_type),
       path_(path),
       entry_hash_(entry_hash),
+      had_index_(had_index),
       key_(key),
       have_open_files_(false),
       initialized_(false) {
@@ -779,21 +836,19 @@ bool SimpleSynchronousEntry::MaybeCreateFile(
   return files_[file_index].IsValid();
 }
 
-bool SimpleSynchronousEntry::OpenFiles(
-    bool had_index,
-    SimpleEntryStat* out_entry_stat) {
+bool SimpleSynchronousEntry::OpenFiles(SimpleEntryStat* out_entry_stat) {
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     File::Error error;
     if (!MaybeOpenFile(i, &error)) {
       // TODO(juliatuttle,gavinp): Remove one each of these triplets of
       // histograms. We can calculate the third as the sum or difference of the
       // other two.
-      RecordSyncOpenResult(
-          cache_type_, OPEN_ENTRY_PLATFORM_FILE_ERROR, had_index);
+      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_PLATFORM_FILE_ERROR,
+                           had_index_);
       SIMPLE_CACHE_UMA(ENUMERATION,
                        "SyncOpenPlatformFileError", cache_type_,
                        -error, -base::File::FILE_ERROR_MAX);
-      if (had_index) {
+      if (had_index_) {
         SIMPLE_CACHE_UMA(ENUMERATION,
                          "SyncOpenPlatformFileError_WithIndex", cache_type_,
                          -error, -base::File::FILE_ERROR_MAX);
@@ -860,20 +915,18 @@ bool SimpleSynchronousEntry::OpenFiles(
   return true;
 }
 
-bool SimpleSynchronousEntry::CreateFiles(
-    bool had_index,
-    SimpleEntryStat* out_entry_stat) {
+bool SimpleSynchronousEntry::CreateFiles(SimpleEntryStat* out_entry_stat) {
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     File::Error error;
     if (!MaybeCreateFile(i, FILE_NOT_REQUIRED, &error)) {
       // TODO(juliatuttle,gavinp): Remove one each of these triplets of
       // histograms. We can calculate the third as the sum or difference of the
       // other two.
-      RecordSyncCreateResult(CREATE_ENTRY_PLATFORM_FILE_ERROR, had_index);
+      RecordSyncCreateResult(CREATE_ENTRY_PLATFORM_FILE_ERROR, had_index_);
       SIMPLE_CACHE_UMA(ENUMERATION,
                        "SyncCreatePlatformFileError", cache_type_,
                        -error, -base::File::FILE_ERROR_MAX);
-      if (had_index) {
+      if (had_index_) {
         SIMPLE_CACHE_UMA(ENUMERATION,
                          "SyncCreatePlatformFileError_WithIndex", cache_type_,
                          -error, -base::File::FILE_ERROR_MAX);
@@ -919,13 +972,80 @@ void SimpleSynchronousEntry::CloseFiles() {
     CloseFile(i);
 }
 
+bool SimpleSynchronousEntry::CheckHeaderAndKey(int file_index) {
+  // TODO(gavinp): Frequently we are doing this at the same time as we read from
+  // the beginning of an entry. It might improve performance to make a single
+  // read(2) call rather than two separate reads. On the other hand, it would
+  // mean an extra memory to memory copy. In the case where we are opening an
+  // entry without a key, the kInitialHeaderRead setting means that we are
+  // actually already reading stream 1 data here, and tossing it out.
+  std::vector<char> header_data(key_.empty() ? kInitialHeaderRead
+                                             : GetHeaderSize(key_.size()));
+  int bytes_read =
+      files_[file_index].Read(0, header_data.data(), header_data.size());
+  const SimpleFileHeader* header =
+      reinterpret_cast<const SimpleFileHeader*>(header_data.data());
+
+  if (bytes_read == -1 || static_cast<size_t>(bytes_read) < sizeof(*header)) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_HEADER, had_index_);
+    return false;
+  }
+  // This resize will not invalidate iterators since it does not enlarge the
+  // header_data.
+  DCHECK_LE(static_cast<size_t>(bytes_read), header_data.size());
+  header_data.resize(bytes_read);
+
+  if (header->initial_magic_number != kSimpleInitialMagicNumber) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_MAGIC_NUMBER, had_index_);
+    return false;
+  }
+
+  if (header->version != kSimpleEntryVersionOnDisk) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_VERSION, had_index_);
+    return false;
+  }
+
+  size_t expected_header_size = GetHeaderSize(header->key_length);
+  if (header_data.size() < expected_header_size) {
+    size_t old_size = header_data.size();
+    int bytes_to_read = expected_header_size - old_size;
+    // This resize will invalidate iterators, since it is enlarging header_data.
+    header_data.resize(expected_header_size);
+    int bytes_read = files_[file_index].Read(
+        old_size, header_data.data() + old_size, bytes_to_read);
+    if (bytes_read != bytes_to_read) {
+      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_KEY, had_index_);
+      return false;
+    }
+    header = reinterpret_cast<const SimpleFileHeader*>(header_data.data());
+  }
+
+  char* key_data = header_data.data() + sizeof(*header);
+  if (base::Hash(key_data, header->key_length) != header->key_hash) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_KEY_HASH_MISMATCH, had_index_);
+    return false;
+  }
+
+  std::string key_from_header(key_data, header->key_length);
+  if (key_.empty()) {
+    key_.swap(key_from_header);
+  } else {
+    if (key_ != key_from_header) {
+      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_KEY_MISMATCH, had_index_);
+      return false;
+    }
+  }
+
+  header_and_key_check_needed_[file_index] = false;
+  return true;
+}
+
 int SimpleSynchronousEntry::InitializeForOpen(
-    bool had_index,
     SimpleEntryStat* out_entry_stat,
     scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
     uint32_t* out_stream_0_crc32) {
   DCHECK(!initialized_);
-  if (!OpenFiles(had_index, out_entry_stat)) {
+  if (!OpenFiles(out_entry_stat)) {
     DLOG(WARNING) << "Could not open platform files for entry.";
     return net::ERR_FAILED;
   }
@@ -933,69 +1053,35 @@ int SimpleSynchronousEntry::InitializeForOpen(
     if (empty_file_omitted_[i])
       continue;
 
-    SimpleFileHeader header;
-    int header_read_result =
-        files_[i].Read(0, reinterpret_cast<char*>(&header), sizeof(header));
-    if (header_read_result != sizeof(header)) {
-      DLOG(WARNING) << "Cannot read header from entry.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_HEADER, had_index);
-      return net::ERR_FAILED;
-    }
-
-    if (header.initial_magic_number != kSimpleInitialMagicNumber) {
-      // TODO(gavinp): This seems very bad; for now we log at WARNING, but we
-      // should give consideration to not saturating the log with these if that
-      // becomes a problem.
-      DLOG(WARNING) << "Magic number did not match.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_MAGIC_NUMBER, had_index);
-      return net::ERR_FAILED;
-    }
-
-    if (header.version != kSimpleEntryVersionOnDisk) {
-      DLOG(WARNING) << "Unreadable version.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_VERSION, had_index);
-      return net::ERR_FAILED;
-    }
-
-    std::unique_ptr<char[]> key(new char[header.key_length]);
-    int key_read_result = files_[i].Read(sizeof(header), key.get(),
-                                         header.key_length);
-    if (key_read_result != base::checked_cast<int>(header.key_length)) {
-      DLOG(WARNING) << "Cannot read key from entry.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_KEY, had_index);
-      return net::ERR_FAILED;
+    if (!key_.empty()) {
+      header_and_key_check_needed_[i] = true;
+    } else {
+      if (!CheckHeaderAndKey(i))
+        return net::ERR_FAILED;
     }
 
-    key_ = std::string(key.get(), header.key_length);
     if (i == 0) {
       // File size for stream 0 has been stored temporarily in data_size[1].
-      int total_data_size =
-          GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size(1));
-      int ret_value_stream_0 = ReadAndValidateStream0(
-          total_data_size, out_entry_stat, stream_0_data, out_stream_0_crc32);
+      int ret_value_stream_0 =
+          ReadAndValidateStream0(out_entry_stat->data_size(1), out_entry_stat,
+                                 stream_0_data, out_stream_0_crc32);
       if (ret_value_stream_0 != net::OK)
         return ret_value_stream_0;
     } else {
       out_entry_stat->set_data_size(
-          2, GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size(2)));
+          2,
+          GetDataSizeFromFileSize(key_.size(), out_entry_stat->data_size(2)));
       if (out_entry_stat->data_size(2) < 0) {
         DLOG(WARNING) << "Stream 2 file is too small.";
         return net::ERR_FAILED;
       }
     }
-
-    if (base::Hash(key.get(), header.key_length) != header.key_hash) {
-      DLOG(WARNING) << "Hash mismatch on key.";
-      RecordSyncOpenResult(
-          cache_type_, OPEN_ENTRY_KEY_HASH_MISMATCH, had_index);
-      return net::ERR_FAILED;
-    }
   }
 
   int32_t sparse_data_size = 0;
   if (!OpenSparseFileIfExists(&sparse_data_size)) {
-    RecordSyncOpenResult(
-        cache_type_, OPEN_ENTRY_SPARSE_OPEN_FAILED, had_index);
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SPARSE_OPEN_FAILED,
+                         had_index_);
     return net::ERR_FAILED;
   }
   out_entry_stat->set_sparse_data_size(sparse_data_size);
@@ -1015,7 +1101,7 @@ int SimpleSynchronousEntry::InitializeForOpen(
   SIMPLE_CACHE_UMA(BOOLEAN, "EntryOpenedAndStream2Removed", cache_type_,
                    removed_stream2);
 
-  RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SUCCESS, had_index);
+  RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SUCCESS, had_index_);
   initialized_ = true;
   return net::OK;
 }
@@ -1048,10 +1134,9 @@ bool SimpleSynchronousEntry::InitializeCreatedFile(
 }
 
 int SimpleSynchronousEntry::InitializeForCreate(
-    bool had_index,
     SimpleEntryStat* out_entry_stat) {
   DCHECK(!initialized_);
-  if (!CreateFiles(had_index, out_entry_stat)) {
+  if (!CreateFiles(out_entry_stat)) {
     DLOG(WARNING) << "Could not create platform files.";
     return net::ERR_FILE_EXISTS;
   }
@@ -1061,49 +1146,55 @@ int SimpleSynchronousEntry::InitializeForCreate(
 
     CreateEntryResult result;
     if (!InitializeCreatedFile(i, &result)) {
-      RecordSyncCreateResult(result, had_index);
+      RecordSyncCreateResult(result, had_index_);
       return net::ERR_FAILED;
     }
   }
-  RecordSyncCreateResult(CREATE_ENTRY_SUCCESS, had_index);
+  RecordSyncCreateResult(CREATE_ENTRY_SUCCESS, had_index_);
   initialized_ = true;
   return net::OK;
 }
 
 int SimpleSynchronousEntry::ReadAndValidateStream0(
-    int total_data_size,
+    int file_size,
     SimpleEntryStat* out_entry_stat,
     scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
-    uint32_t* out_stream_0_crc32) const {
+    uint32_t* out_stream_0_crc32) {
   // Temporarily assign all the data size to stream 1 in order to read the
   // EOF record for stream 0, which contains the size of stream 0.
+  int total_data_size = GetDataSizeFromFileSize(key_.size(), file_size);
   out_entry_stat->set_data_size(0, 0);
-  out_entry_stat->set_data_size(1, total_data_size - sizeof(SimpleFileEOF));
+  out_entry_stat->set_data_size(
+      1,
+      total_data_size - sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF));

[Randy Smith (Not in Mondays), 2016/05/19 19:38:50]

So I looked and didn't find any problems for this possibility, but might this
value now be negative?  I.e. if the hash isn't there, and the total data for
streams 0 and 1 is, say, 0?  I think everything works out ok if it is, but the
thought scares me, so I wanted to run it by you in case you saw something I
didn't.

[gavinp, 2016/05/20 01:42:32]

It is negative occasionally, I believe -7 at most (least?). It's clean, some
unit tests exercise this path.

 
   bool has_crc32;
+  bool has_key_sha256;
   uint32_t read_crc32;
   int stream_0_size;
-  int ret_value_crc32 = GetEOFRecordData(
-      0, *out_entry_stat, &has_crc32, &read_crc32, &stream_0_size);
+  int ret_value_crc32 =
+      GetEOFRecordData(0, *out_entry_stat, &has_crc32, &has_key_sha256,
+                       &read_crc32, &stream_0_size);
   if (ret_value_crc32 != net::OK)
     return ret_value_crc32;
-
-  if (stream_0_size > out_entry_stat->data_size(1))
+  // Calculate and set the real values for data size.
+  int stream_1_size = out_entry_stat->data_size(1) - stream_0_size;
+  if (!has_key_sha256)
+    stream_1_size += sizeof(net::SHA256HashValue);
+  if (stream_1_size < 0)
     return net::ERR_FAILED;
-
-  // These are the real values of data size.
   out_entry_stat->set_data_size(0, stream_0_size);
-  out_entry_stat->set_data_size(
-      1, out_entry_stat->data_size(1) - stream_0_size);
+  out_entry_stat->set_data_size(1, stream_1_size);
 
   // Put stream 0 data in memory.
   *stream_0_data = new net::GrowableIOBuffer();
-  (*stream_0_data)->SetCapacity(stream_0_size);
-  int file_offset = out_entry_stat->GetOffsetInFile(key_, 0, 0);
-  File* file = const_cast<File*>(&files_[0]);
-  int bytes_read =
-      file->Read(file_offset, (*stream_0_data)->data(), stream_0_size);
-  if (bytes_read != stream_0_size)
+  (*stream_0_data)->SetCapacity(stream_0_size + sizeof(net::SHA256HashValue));
+  int file_offset = out_entry_stat->GetOffsetInFile(key_.size(), 0, 0);
+  int read_size = stream_0_size;
+  if (has_key_sha256)
+    read_size += sizeof(net::SHA256HashValue);
+  if (files_[0].Read(file_offset, (*stream_0_data)->data(), read_size) !=
+      read_size)
     return net::ERR_FAILED;
 
   // Check the CRC32.
@@ -1119,6 +1210,27 @@ int SimpleSynchronousEntry::ReadAndValidateStream0(
     return net::ERR_FAILED;
   }
   *out_stream_0_crc32 = expected_crc32;
+
+  // If present, check the key SHA256.
+  if (has_key_sha256) {
+    net::SHA256HashValue hash_value;
+    CalculateSHA256OfKey(key_, &hash_value);
+    bool matched =
+        std::memcmp(&hash_value, (*stream_0_data)->data() + stream_0_size,
+                    sizeof(hash_value)) == 0;
+    if (!matched) {
+      RecordKeySHA256Result(cache_type_, KeySHA256Result::NO_MATCH);
+      return net::ERR_FAILED;
+    }
+    RecordKeySHA256Result(cache_type_, KeySHA256Result::MATCHED);
+  } else {
+    RecordKeySHA256Result(cache_type_, KeySHA256Result::NOT_PRESENT);
+  }
+
+  // Ensure the key is validated before completion.
+  if (!has_key_sha256 && header_and_key_check_needed_[0])
+    CheckHeaderAndKey(0);
+
   RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
   return net::OK;
 }
@@ -1126,10 +1238,11 @@ int SimpleSynchronousEntry::ReadAndValidateStream0(
 int SimpleSynchronousEntry::GetEOFRecordData(int index,
                                              const SimpleEntryStat& entry_stat,
                                              bool* out_has_crc32,
+                                             bool* out_has_key_sha256,
                                              uint32_t* out_crc32,
                                              int* out_data_size) const {
   SimpleFileEOF eof_record;
-  int file_offset = entry_stat.GetEOFOffsetInFile(key_, index);
+  int file_offset = entry_stat.GetEOFOffsetInFile(key_.size(), index);
   int file_index = GetFileIndexFromStreamIndex(index);
   File* file = const_cast<File*>(&files_[file_index]);
   if (file->Read(file_offset, reinterpret_cast<char*>(&eof_record),
@@ -1147,6 +1260,9 @@ int SimpleSynchronousEntry::GetEOFRecordData(int index,
 
   *out_has_crc32 = (eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
                    SimpleFileEOF::FLAG_HAS_CRC32;
+  *out_has_key_sha256 =
+      (eof_record.flags & SimpleFileEOF::FLAG_HAS_KEY_SHA256) ==
+      SimpleFileEOF::FLAG_HAS_KEY_SHA256;
   *out_crc32 = eof_record.data_crc32;
   *out_data_size = eof_record.stream_size;
   SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_, *out_has_crc32);