SimpleCache: merge the first and second stream in one file

net/disk_cache/simple/simple_synchronous_entry.cc

 // Copyright (c) 2013 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 "net/disk_cache/simple/simple_synchronous_entry.h"
 
 #include <algorithm>
 #include <cstring>
 #include <functional>
 #include <limits>
@@ skipping 123 matching lines @@
 
 void RecordCloseResult(net::CacheType cache_type, CloseResult result) {
   SIMPLE_CACHE_UMA(ENUMERATION,
                    "SyncCloseResult", cache_type, result, WRITE_RESULT_MAX);
 }
 
 }  // namespace
 
 namespace disk_cache {
 
 using simple_util::ConvertEntryHashKeyToHexString;

[pasko, 2013/09/17 16:40:52]

this one is not used

[clamy, 2013/09/18 16:17:15]

Done.

 using simple_util::GetEntryHashKey;
-using simple_util::GetFilenameFromEntryHashAndIndex;
+using simple_util::GetFilenameFromEntryHashAndFileIndex;
 using simple_util::GetDataSizeFromKeyAndFileSize;
 using simple_util::GetFileSizeFromKeyAndDataSize;
-using simple_util::GetFileOffsetFromKeyAndDataOffset;
+using simple_util::GetFileIndexFromStreamIndex;
 
 SimpleEntryStat::SimpleEntryStat() {}

[pasko, 2013/09/17 16:40:52]

we do not need this constructor

[clamy, 2013/09/18 16:17:15]

Done.

 
 SimpleEntryStat::SimpleEntryStat(base::Time last_used_p,
                                  base::Time last_modified_p,
                                  const int32 data_size_p[])
     : last_used(last_used_p),
       last_modified(last_modified_p) {
   memcpy(data_size, data_size_p, sizeof(data_size));
 }
 
+int SimpleEntryStat::GetOffsetInFile(const std::string& key,
+                                     int offset,
+                                     int stream_index) const {
+  const int64 headers_size = sizeof(SimpleFileHeader) + key.size();
+  const int64 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 stream_index) const {
+  return GetOffsetInFile(key, data_size[stream_index], stream_index);
+}
+
+int SimpleEntryStat::GetLastEOFOffsetInFile(const std::string& key,
+                                            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);
+}
+
+int SimpleEntryStat::GetFileSize(const std::string& key, int file_index) const {
+  const int total_data_size = file_index == 0 ?
+      data_size[0] + data_size[1] + sizeof(SimpleFileEOF) : data_size[2];
+  return GetFileSizeFromKeyAndDataSize(key, total_data_size);
+}
+
 SimpleEntryCreationResults::SimpleEntryCreationResults(
     SimpleEntryStat entry_stat)
     : sync_entry(NULL),
       entry_stat(entry_stat),
       result(net::OK) {
 }
 
 SimpleEntryCreationResults::~SimpleEntryCreationResults() {
 }
 
@@ skipping 28 matching lines @@
 // static
 void SimpleSynchronousEntry::OpenEntry(
     net::CacheType cache_type,
     const FilePath& path,
     const uint64 entry_hash,
     bool had_index,
     SimpleEntryCreationResults *out_results) {
   SimpleSynchronousEntry* sync_entry =
       new SimpleSynchronousEntry(cache_type, path, "", entry_hash);
   out_results->result = sync_entry->InitializeForOpen(
-      had_index, &out_results->entry_stat);
+      had_index, &out_results->entry_stat, &out_results->stream_0_data);
   if (out_results->result != net::OK) {
     sync_entry->Doom();
     delete sync_entry;
     out_results->sync_entry = NULL;
+    out_results->stream_0_data = NULL;
     return;
   }
   out_results->sync_entry = sync_entry;
 }
 
 // static
 void SimpleSynchronousEntry::CreateEntry(
     net::CacheType cache_type,
     const FilePath& path,
     const std::string& key,
@@ skipping 15 matching lines @@
   out_results->sync_entry = sync_entry;
 }
 
 // TODO(gavinp): Move this function to its correct location in this .cc file.
 // static
 bool SimpleSynchronousEntry::DeleteFilesForEntryHash(
     const FilePath& path,
     const uint64 entry_hash) {
   bool result = true;
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
-    FilePath to_delete = path.AppendASCII(
-        GetFilenameFromEntryHashAndIndex(entry_hash, i));
+    FilePath to_delete =
+        path.AppendASCII(GetFilenameFromEntryHashAndFileIndex(entry_hash, i));
     if (!base::DeleteFile(to_delete, false)) {
       result = false;
       DLOG(ERROR) << "Could not delete " << to_delete.MaybeAsASCII();
     }
   }
   return result;
 }
 
 // static
 int SimpleSynchronousEntry::DoomEntry(
@@ skipping 11 matching lines @@
     const FilePath& path) {
   const size_t did_delete_count = std::count_if(
       key_hashes->begin(), key_hashes->end(), std::bind1st(
           std::ptr_fun(SimpleSynchronousEntry::DeleteFilesForEntryHash), path));
   return (did_delete_count == key_hashes->size()) ? net::OK : net::ERR_FAILED;
 }
 
 void SimpleSynchronousEntry::ReadData(const EntryOperationData& in_entry_op,
                                       net::IOBuffer* out_buf,
                                       uint32* out_crc32,
-                                      base::Time* out_last_used,
+                                      SimpleEntryStat* out_entry_stat,
                                       int* out_result) const {
   DCHECK(initialized_);
-  int64 file_offset =
-      GetFileOffsetFromKeyAndDataOffset(key_, in_entry_op.offset);
-  int bytes_read = ReadPlatformFile(files_[in_entry_op.index],
-                                    file_offset,
-                                    out_buf->data(),
-                                    in_entry_op.buf_len);
+  const int64 file_offset = out_entry_stat->GetOffsetInFile(

[gavinp, 2013/09/17 15:07:43]

That doesn't look like an output parameter! Perhaps rename it to "entry_stat" ? 

Is this necessary, though? Is it legal to call ReadData for stream 0? If we make
that illegal, we can get away without this parameter being in/out.

[pasko, 2013/09/17 16:40:52]

seems like a cool simple strategy to assert against reading from stream 0

[pasko, 2013/09/18 12:03:19]

.. but on the other hand I also like the uniformity of using the
SimpleEntryStat::GetOffsetInFile() here

[clamy, 2013/09/18 16:17:15]

All offset functions are now used part of EntryStat, so we need one to compute
the offset. I don't really want to reintroduce them. I could create a fake entry
stat with 0 in data_size, since it is not called on stream 0, wdyt?

[gavinp, 2013/09/18 17:20:35]

Or you could just rename the parameter to remove the "out_" and call that good
enough. Probably we should have the asserts though, just because it's too weird
if anyone reads from stream 0.

[clamy, 2013/09/18 18:29:38]

Done.

+      key_, in_entry_op.offset, in_entry_op.index);
+  int file_index = GetFileIndexFromStreamIndex(in_entry_op.index);
+  int bytes_read = ReadPlatformFile(
+      files_[file_index], file_offset, out_buf->data(), in_entry_op.buf_len);
   if (bytes_read > 0) {
-    *out_last_used = Time::Now();
+    out_entry_stat->last_used = Time::Now();
     *out_crc32 = crc32(crc32(0L, Z_NULL, 0),
                        reinterpret_cast<const Bytef*>(out_buf->data()),
                        bytes_read);
   }
   if (bytes_read >= 0) {
     *out_result = bytes_read;
   } else {
     *out_result = net::ERR_CACHE_READ_FAILURE;
     Doom();
   }
 }
 
 void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
                                        net::IOBuffer* in_buf,
                                        SimpleEntryStat* out_entry_stat,
                                        int* out_result) const {
   DCHECK(initialized_);
   int index = in_entry_op.index;

[pasko, 2013/09/18 12:03:19]

please add a DCHECK_NE(0, index), similarly in SSE::ReadData

[clamy, 2013/09/18 16:17:15]

Done.

+  int file_index = GetFileIndexFromStreamIndex(index);
   int offset = in_entry_op.offset;
   int buf_len = in_entry_op.buf_len;
   int truncate = in_entry_op.truncate;
-
+  const int64 file_offset = out_entry_stat->GetOffsetInFile(
+      key_, in_entry_op.offset, in_entry_op.index);
   bool extending_by_write = offset + buf_len > out_entry_stat->data_size[index];
   if (extending_by_write) {
-    // We are extending the file, and need to insure the EOF record is zeroed.
-    const int64 file_eof_offset = GetFileOffsetFromKeyAndDataOffset(
-        key_, out_entry_stat->data_size[index]);
-    if (!TruncatePlatformFile(files_[index], file_eof_offset)) {
+    // The EOF record and the eventual stream afterward need to be zeroed out.
+    const int64 file_eof_offset =
+        out_entry_stat->GetEOFOffsetInFile(key_, index);
+    if (!TruncatePlatformFile(files_[file_index], file_eof_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_PRETRUNCATE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
   }
-  const int64 file_offset = GetFileOffsetFromKeyAndDataOffset(key_, offset);
   if (buf_len > 0) {
     if (WritePlatformFile(
-            files_[index], file_offset, in_buf->data(), buf_len) != buf_len) {
+            files_[file_index], file_offset, in_buf->data(), buf_len) !=
+        buf_len) {
       RecordWriteResult(cache_type_, WRITE_RESULT_WRITE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
   }
   if (!truncate && (buf_len > 0 || !extending_by_write)) {
     out_entry_stat->data_size[index] =
         std::max(out_entry_stat->data_size[index], offset + buf_len);
   } else {
-    if (!TruncatePlatformFile(files_[index], file_offset + buf_len)) {
+    out_entry_stat->data_size[index] = offset + buf_len;
+    int file_eof_offset = out_entry_stat->GetLastEOFOffsetInFile(key_, index);
+    if (!TruncatePlatformFile(files_[file_index], file_eof_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_TRUNCATE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
-    out_entry_stat->data_size[index] = offset + buf_len;

[pasko, 2013/09/18 12:03:19]

nit:

is there a reason to move this line above the truncation? In case truncation
fails it seems more natural to leave the data size unchanged. This would be on a
Doomed entry, so not too interesting, but still raises unnecessary questions
when reading.

[clamy, 2013/09/18 16:17:15]

Otherwise out_entry_stat will return the wrong offset for truncation.

   }
 
   RecordWriteResult(cache_type_, WRITE_RESULT_SUCCESS);
   out_entry_stat->last_used = out_entry_stat->last_modified = Time::Now();
   *out_result = buf_len;
 }
 
 void SimpleSynchronousEntry::CheckEOFRecord(int index,
-                                            int32 data_size,
+                                            const SimpleEntryStat& entry_stat,
                                             uint32 expected_crc32,
                                             int* out_result) const {
   DCHECK(initialized_);
-
-  SimpleFileEOF eof_record;
-  int64 file_offset = GetFileOffsetFromKeyAndDataOffset(key_, data_size);
-  if (ReadPlatformFile(files_[index],
-                       file_offset,
-                       reinterpret_cast<char*>(&eof_record),
-                       sizeof(eof_record)) != sizeof(eof_record)) {
-    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_READ_FAILURE);
+  uint32 crc32;
+  bool has_crc32;
+  int stream_size;
+  *out_result =
+      GetEOFRecordData(index, entry_stat, &has_crc32, &crc32, &stream_size);
+  if (*out_result != net::OK) {
     Doom();
-    *out_result = net::ERR_CACHE_CHECKSUM_READ_FAILURE;
     return;
   }
-
-  if (eof_record.final_magic_number != kSimpleFinalMagicNumber) {
-    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_MAGIC_NUMBER_MISMATCH);
-    DLOG(INFO) << "eof record had bad magic number.";
+  if (has_crc32 && crc32 != expected_crc32) {
+    DLOG(INFO) << "Eof record had bad crc.";

[pasko, 2013/09/18 12:03:19]

nit: s/Eof/EOF/, same in other places

[clamy, 2013/09/18 16:17:15]

Done.

+    *out_result = net::ERR_CACHE_CHECKSUM_MISMATCH;
+    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
     Doom();
-    *out_result = net::ERR_CACHE_CHECKSUM_READ_FAILURE;
     return;
   }
-
-  const bool has_crc = (eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
-                       SimpleFileEOF::FLAG_HAS_CRC32;
-  SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_, has_crc);
-  if (has_crc && eof_record.data_crc32 != expected_crc32) {
-    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
-    DLOG(INFO) << "eof record had bad crc.";
-    Doom();
-    *out_result = net::ERR_CACHE_CHECKSUM_MISMATCH;
-    return;
-  }
-
   RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
-  *out_result = net::OK;
 }
 
 void SimpleSynchronousEntry::Close(
     const SimpleEntryStat& entry_stat,
-    scoped_ptr<std::vector<CRCRecord> > crc32s_to_write) {
+    scoped_ptr<std::vector<CRCRecord> > crc32s_to_write,
+    net::GrowableIOBuffer* stream_0_data) {
+  if (!stream_0_data) {
+    if (entry_stat.data_size[0])
+      Doom();
+  } else {
+    // Write stream 0 data.
+    int stream_0_offset = entry_stat.GetOffsetInFile(key_, 0, 0);
+    if (WritePlatformFile(files_[0],
+                          stream_0_offset,
+                          stream_0_data->data(),
+                          entry_stat.data_size[0]) != entry_stat.data_size[0]) {
+      RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
+      DLOG(INFO) << "Could not write stream 0 data.";
+      Doom();
+    }
+  }
+
   for (std::vector<CRCRecord>::const_iterator it = crc32s_to_write->begin();
        it != crc32s_to_write->end(); ++it) {
     SimpleFileEOF eof_record;
+    int index = it->index;
+    eof_record.stream_size = entry_stat.data_size[index];
     eof_record.final_magic_number = kSimpleFinalMagicNumber;
     eof_record.flags = 0;
     if (it->has_crc32)
       eof_record.flags |= SimpleFileEOF::FLAG_HAS_CRC32;
     eof_record.data_crc32 = it->data_crc32;
-    int64 file_offset = GetFileOffsetFromKeyAndDataOffset(
-        key_, entry_stat.data_size[it->index]);
-    if (WritePlatformFile(files_[it->index],
-                          file_offset,
+    int file_index = GetFileIndexFromStreamIndex(index);
+    int eof_offset = entry_stat.GetEOFOffsetInFile(key_, index);
+    if (WritePlatformFile(files_[file_index],

[pasko, 2013/09/18 15:08:16]

With your other CL for testing all streams (crrev.com/23702024 awesome change
btw) I found an interesting case. If the file was previously larger, this write
would not change the size, hence we'd get wrong stream sizes at the next open.

this is fixable with:
if (index != 1 && !TruncatePlatformFile(files_[file_index], eof_offset)) {
  // handle error
}

The "index != 1" is necessary because we write EOFs in the backwards order. This
cute detail needs a comment. 

[clamy, 2013/09/18 16:17:15]

Done.

+                          eof_offset,
                           reinterpret_cast<const char*>(&eof_record),
                           sizeof(eof_record)) != sizeof(eof_record)) {
       RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
       DLOG(INFO) << "Could not write eof record.";
       Doom();
       break;
     }
-    const int64 file_size = file_offset + sizeof(eof_record);
+  }
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
+    bool did_close_file = ClosePlatformFile(files_[i]);
+    CHECK(did_close_file);

[pasko, 2013/09/18 12:03:19]

we don't want to crash the browser here on a file error, so DCHECK

[clamy, 2013/09/18 16:17:15]

Done.

+    const int64 file_size = entry_stat.GetFileSize(key_, i);
     SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                      "LastClusterSize", cache_type_,
                      file_size % 4096, 0, 4097, 50);
     const int64 cluster_loss = file_size % 4096 ? 4096 - file_size % 4096 : 0;
     SIMPLE_CACHE_UMA(PERCENTAGE,
                      "LastClusterLossPercent", cache_type_,
                      cluster_loss * 100 / (cluster_loss + file_size));
   }
-
-  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
-    bool did_close_file = ClosePlatformFile(files_[i]);
-    CHECK(did_close_file);
-  }
   RecordCloseResult(cache_type_, CLOSE_RESULT_SUCCESS);
   have_open_files_ = false;
   delete this;
 }
 
 SimpleSynchronousEntry::SimpleSynchronousEntry(net::CacheType cache_type,
                                                const FilePath& path,
                                                const std::string& key,
                                                const uint64 entry_hash)
     : cache_type_(cache_type),
@@ skipping 11 matching lines @@
   DCHECK(!(have_open_files_ && initialized_));
   if (have_open_files_)
     CloseFiles();
 }
 
 bool SimpleSynchronousEntry::OpenOrCreateFiles(
     bool create,
     bool had_index,
     SimpleEntryStat* out_entry_stat) {
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
-    FilePath filename = path_.AppendASCII(
-        GetFilenameFromEntryHashAndIndex(entry_hash_, i));
+    FilePath filename =
+        path_.AppendASCII(GetFilenameFromEntryHashAndFileIndex(entry_hash_, i));
     int flags = PLATFORM_FILE_READ | PLATFORM_FILE_WRITE;
     if (create)
       flags |= PLATFORM_FILE_CREATE;
     else
       flags |= PLATFORM_FILE_OPEN;
     PlatformFileError error;
     files_[i] = CreatePlatformFile(filename, flags, NULL, &error);
     if (error != PLATFORM_FILE_OK) {
       // TODO(ttuttle,gavinp): Remove one each of these triplets of histograms.
       // We can calculate the third as the sum or difference of the other two.
@@ skipping 35 matching lines @@
         DLOG_IF(INFO, !did_close) << "Could not close file "
                                   << filename.MaybeAsASCII();
       }
       return false;
     }
   }
 
   have_open_files_ = true;
   if (create) {
     out_entry_stat->last_modified = out_entry_stat->last_used = Time::Now();
-    for (int i = 0; i < kSimpleEntryFileCount; ++i)
+    for (int i = 0; i < kSimpleEntryStreamCount; ++i)
       out_entry_stat->data_size[i] = 0;
   } else {
     base::TimeDelta entry_age = base::Time::Now() - base::Time::UnixEpoch();
     for (int i = 0; i < kSimpleEntryFileCount; ++i) {
       PlatformFileInfo file_info;
       bool success = GetPlatformFileInfo(files_[i], &file_info);
       base::Time file_last_modified;
       if (!success) {
         DLOG(WARNING) << "Could not get platform file info.";
         continue;
       }
       out_entry_stat->last_used = file_info.last_accessed;
       if (simple_util::GetMTime(path_, &file_last_modified))
         out_entry_stat->last_modified = file_last_modified;
       else
         out_entry_stat->last_modified = file_info.last_modified;
 
       base::TimeDelta stream_age =
           base::Time::Now() - out_entry_stat->last_modified;
       if (stream_age < entry_age)
         entry_age = stream_age;
 
-      // Keep the file size in |data size_| briefly until the key is initialized
-      // properly.
-      out_entry_stat->data_size[i] = file_info.size;
+      // Keep the file size in |data size_| temporarily until the data sizes are
+      // initialized correctly.

[pasko, 2013/09/18 12:03:19]

Let's write a more detailed comment here. The 'data' is confusing. Here we can
explain that sizes of the data streams are not known before the key length is
not read, as well as size of the stream 0 in the final EOF record.

The [i + 1] is a puzzle, explained in InitializeForOpen, but given the
"elegance" of the trick, please explain it in the comment as well.

Frankly, I think this should be refactored to avoid having tricks like these,
but that's certainly for later CLs.

[clamy, 2013/09/18 16:17:15]

Done.

+      out_entry_stat->data_size[i + 1] = file_info.size;
     }
     SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                      "SyncOpenEntryAge", cache_type_,
                      entry_age.InHours(), 1, 1000, 50);
   }
 
   return true;
 }
 
 void SimpleSynchronousEntry::CloseFiles() {
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     DCHECK_NE(kInvalidPlatformFileValue, files_[i]);
     bool did_close = ClosePlatformFile(files_[i]);
     DCHECK(did_close);
   }
 }
 
-int SimpleSynchronousEntry::InitializeForOpen(bool had_index,
-                                              SimpleEntryStat* out_entry_stat) {
+int SimpleSynchronousEntry::InitializeForOpen(
+    bool had_index,
+    SimpleEntryStat* out_entry_stat,
+    scoped_refptr<net::GrowableIOBuffer>* stream_0_data) {
   DCHECK(!initialized_);
   if (!OpenOrCreateFiles(false, had_index, out_entry_stat))
     return net::ERR_FAILED;
-
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     SimpleFileHeader header;
     int header_read_result =
         ReadPlatformFile(files_[i], 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;
     }
@@ skipping 16 matching lines @@
     scoped_ptr<char[]> key(new char[header.key_length]);
     int key_read_result = ReadPlatformFile(files_[i], sizeof(header),
                                            key.get(), header.key_length);
     if (key_read_result != implicit_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;
     }
 
     key_ = std::string(key.get(), header.key_length);
-    out_entry_stat->data_size[i] =
-        GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size[i]);
-    if (out_entry_stat->data_size[i] < 0) {
-      // This entry can't possibly be valid, as it does not have enough space to
-      // store a valid SimpleFileEOF record.
-      return net::ERR_FAILED;
+    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);
+      if (ret_value_stream_0 != net::OK)
+        return ret_value_stream_0;
+    } else {
+      out_entry_stat->data_size[2] =
+          GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size[2]);
+      if (out_entry_stat->data_size[2] < 0)
+        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;
     }
   }
   RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SUCCESS, had_index);
@@ skipping 10 matching lines @@
     return net::ERR_FILE_EXISTS;
   }
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     SimpleFileHeader header;
     header.initial_magic_number = kSimpleInitialMagicNumber;
     header.version = kSimpleVersion;
 
     header.key_length = key_.size();
     header.key_hash = base::Hash(key_);
 
-    if (WritePlatformFile(files_[i], 0, reinterpret_cast<char*>(&header),
-                          sizeof(header)) != sizeof(header)) {
-      DLOG(WARNING) << "Could not write headers to new cache entry.";
+    if (WritePlatformFile(
+            files_[i], 0, reinterpret_cast<char*>(&header), sizeof(header)) !=
+        sizeof(header)) {
+      DLOG(WARNING) << "Could not write cache file header to cache entry.";
       RecordSyncCreateResult(
           cache_type_, CREATE_ENTRY_CANT_WRITE_HEADER, had_index);
       return net::ERR_FAILED;
     }
 
-    if (WritePlatformFile(files_[i], sizeof(header), key_.data(),
-                          key_.size()) != implicit_cast<int>(key_.size())) {
+    if (WritePlatformFile(
+            files_[i], sizeof(SimpleFileHeader), key_.data(), key_.size()) !=
+        implicit_cast<int>(key_.size())) {
       DLOG(WARNING) << "Could not write keys to new cache entry.";
       RecordSyncCreateResult(
           cache_type_, CREATE_ENTRY_CANT_WRITE_KEY, had_index);
       return net::ERR_FAILED;
     }
   }
   RecordSyncCreateResult(cache_type_, CREATE_ENTRY_SUCCESS, had_index);
   initialized_ = true;
   return net::OK;
 }
 
+int SimpleSynchronousEntry::ReadAndValidateStream0(
+    int total_data_size,
+    SimpleEntryStat* out_entry_stat,
+    scoped_refptr<net::GrowableIOBuffer>* stream_0_data) const {
+  // 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.
+  out_entry_stat->data_size[0] = 0;
+  out_entry_stat->data_size[1] = total_data_size - sizeof(SimpleFileEOF);
+
+  bool has_crc32;
+  uint32 read_crc32;
+  int stream_0_size;
+  int ret_value_crc32 = GetEOFRecordData(
+      0, *out_entry_stat, &has_crc32, &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])
+    return net::ERR_FAILED;
+
+  // These are the real values of data size.
+  out_entry_stat->data_size[0] = stream_0_size;

[pasko, 2013/09/18 12:03:19]

so the function ReadAndValidateStream0() also updates data_size[*] which is
counter-intuitive, but I don't know what to suggest beyond a refactoring to
initialize all stream sizes early. I believe this should be postponed for later.
The quick hack to keep a file size in data_size has just backfired.

+  out_entry_stat->data_size[1] -= stream_0_size;
+
+  // Put stream 0 data in memory.
+  *stream_0_data = NULL;

[pasko, 2013/09/18 12:03:19]

is there a reason in touching this here? the value would not be checked on the
error path

[clamy, 2013/09/18 16:17:15]

Done.

+  if (stream_0_size != 0) {
+    *stream_0_data = new net::GrowableIOBuffer();
+    (*stream_0_data)->SetCapacity(stream_0_size);
+    int file_offset = out_entry_stat->GetOffsetInFile(key_, 0, 0);
+    int bytes_read = ReadPlatformFile(
+        files_[0], file_offset, (*stream_0_data)->data(), stream_0_size);
+    if (bytes_read != stream_0_size)
+      return net::ERR_FAILED;
+  }
+
+  // Check the CRC32.
+  uint32 expected_crc32 =
+      stream_0_size == 0
+          ? crc32(0, Z_NULL, 0)
+          : crc32(crc32(0, Z_NULL, 0),
+                  reinterpret_cast<const Bytef*>((*stream_0_data)->data()),
+                  stream_0_size);
+  if (has_crc32 && read_crc32 != expected_crc32) {
+    DLOG(INFO) << "Eof record had bad crc.";
+    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
+    return net::ERR_FAILED;
+  }
+  RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
+  return net::OK;
+}
+
+int SimpleSynchronousEntry::GetEOFRecordData(int index,
+                                             const SimpleEntryStat& entry_stat,
+                                             bool* out_has_crc32,
+                                             uint32* out_crc32,
+                                             int* out_data_size) const {
+  SimpleFileEOF eof_record;
+  int file_offset = entry_stat.GetEOFOffsetInFile(key_, index);
+  int file_index = GetFileIndexFromStreamIndex(index);
+  if (ReadPlatformFile(files_[file_index],
+                       file_offset,
+                       reinterpret_cast<char*>(&eof_record),
+                       sizeof(eof_record)) != sizeof(eof_record)) {
+    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_READ_FAILURE);
+    return net::ERR_CACHE_CHECKSUM_READ_FAILURE;

[pasko, 2013/09/18 12:03:19]

fun fact: cache checksum read failure can happen even if we did not have the
checksum in the EOF record

+  }
+
+  if (eof_record.final_magic_number != kSimpleFinalMagicNumber) {
+    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_MAGIC_NUMBER_MISMATCH);
+    DLOG(INFO) << "Eof record had bad magic number.";
+    return net::ERR_CACHE_CHECKSUM_READ_FAILURE;
+  }
+
+  *out_has_crc32 = (eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
+                   SimpleFileEOF::FLAG_HAS_CRC32;
+  *out_crc32 = eof_record.data_crc32;
+  *out_data_size = eof_record.stream_size;
+  SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_, *out_has_crc32);
+  return net::OK;
+}
+
 void SimpleSynchronousEntry::Doom() const {
   // TODO(gavinp): Consider if we should guard against redundant Doom() calls.
   DeleteFilesForEntryHash(path_, entry_hash_);
 }
 
 }  // namespace disk_cache