SimpleCache optimistic writes should not keep references to their IO buffer.

After the write operation returns OK, the caller is free to reuse the IOBuffer.
It is invalid to do, as the SimpleCache did previously, return OK and keep a
reference and use the IOBuffer.

As it happens, the BufferResource handler does reuse IO buffers, and this was
causing cache read errors due to CRC mismatches from the IO buffer being reused.

R=pasko,felipeg
BUG=239223

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

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

2 files changed, 45 insertions, 4 deletions

diff --git a/net/disk_cache/entry_unittest.cc b/net/disk_cache/entry_unittest.cc
index 7563131..9af6e65 100644
--- a/net/disk_cache/entry_unittest.cc
+++ b/net/disk_cache/entry_unittest.cc
@@ -2324,7 +2324,7 @@ void DiskCacheEntryTest::SimpleCacheMakeBadChecksumEntry(const char* key) {
   ASSERT_NE(base::kInvalidPlatformFileValue, entry_file0);
 
   const std::string bad_data = "HAHAHA";
-  DCHECK_LE(bad_data.size(), data.size());
+  EXPECT_LE(bad_data.size(), data.size());
   int64 file_offset =
       disk_cache::simple_util::GetFileOffsetFromKeyAndDataOffset(key, 0);
   ASSERT_EQ(implicit_cast<int>(bad_data.size()),
@@ -2347,7 +2347,7 @@ TEST_F(DiskCacheEntryTest, SimpleCacheBadChecksum) {
   EXPECT_EQ(net::OK, OpenEntry(key, &entry));
 
   const int kReadBufferSize = 200;
-  DCHECK_GE(kReadBufferSize, entry->GetDataSize(0));
+  EXPECT_GE(kReadBufferSize, entry->GetDataSize(0));
   scoped_refptr<net::IOBuffer> read_buffer(new net::IOBuffer(kReadBufferSize));
   EXPECT_EQ(net::ERR_CACHE_CHECKSUM_MISMATCH,
             ReadData(entry, 0, 0, read_buffer, kReadBufferSize));
@@ -2369,7 +2369,7 @@ TEST_F(DiskCacheEntryTest, SimpleCacheErrorThenDoom) {
   EXPECT_EQ(net::OK, OpenEntry(key, &entry));
 
   const int kReadBufferSize = 200;
-  DCHECK_GE(kReadBufferSize, entry->GetDataSize(0));
+  EXPECT_GE(kReadBufferSize, entry->GetDataSize(0));
   scoped_refptr<net::IOBuffer> read_buffer(new net::IOBuffer(kReadBufferSize));
   EXPECT_EQ(net::ERR_CACHE_CHECKSUM_MISMATCH,
             ReadData(entry, 0, 0, read_buffer, kReadBufferSize));
@@ -2704,6 +2704,39 @@ TEST_F(DiskCacheEntryTest, SimpleCacheOptimistic6) {
   entry->Close();
 }
 
+// Confirm that IO buffers are not referenced by the Simple Cache after a write
+// completes.
+TEST_F(DiskCacheEntryTest, SimpleCacheOptimisticWriteReleases) {
+  SetSimpleCacheMode();
+  InitCache();
+
+  const char key[] = "the first key";
+  disk_cache::Entry* entry = NULL;
+
+  // First, an optimistic create.
+  ASSERT_EQ(net::OK,
+            cache_->CreateEntry(key, &entry, net::CompletionCallback()));
+
+  ASSERT_TRUE(entry);
+  const int kWriteSize = 512;
+  scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kWriteSize));
+  EXPECT_TRUE(buffer1->HasOneRef());
+
+  // An optimistic write happens only when there is an empty queue of pending
+  // operations. To ensure the queue is empty, we issue a write and wait until
+  // it completes.
+  EXPECT_EQ(kWriteSize,
+            WriteData(entry, 0, 0, buffer1, kWriteSize, false));
+  EXPECT_TRUE(buffer1->HasOneRef());
+
+  // Finally, we should perform an optimistic write and confirm that all
+  // references to the IO buffer have been released.
+  EXPECT_EQ(kWriteSize, entry->WriteData(
+      1, 0, buffer1, kWriteSize, net::CompletionCallback(), false));
+  EXPECT_TRUE(buffer1->HasOneRef());
+  entry->Close();
+}
+
 TEST_F(DiskCacheEntryTest, DISABLED_SimpleCacheCreateDoomRace) {
   // Test sequence:
   // Create, Doom, Write, Close, Check files are not on disk anymore.
diff --git a/net/disk_cache/simple/simple_entry_impl.cc b/net/disk_cache/simple/simple_entry_impl.cc
index 6d1e70c..8ed14be 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -287,9 +287,17 @@ int SimpleEntryImpl::WriteData(int stream_index,
     // prevents from previous possibly-conflicting writes that could be stacked
     // in the |pending_operations_|. We could optimize this for when we have
     // only read operations enqueued.
+    // TODO(gavinp,pasko): For performance, don't use a copy of an IOBuffer
+    // here to avoid paying the price of the RefCountedThreadSafe atomic
+    // operations.
+    IOBuffer* buf_copy = NULL;
+    if (buf) {
+      buf_copy = new IOBuffer(buf_len);
+      memcpy(buf_copy->data(), buf->data(), buf_len);
+    }
     pending_operations_.push(
         base::Bind(&SimpleEntryImpl::WriteDataInternal, this, stream_index,
-                   offset, make_scoped_refptr(buf), buf_len,
+                   offset, make_scoped_refptr(buf_copy), buf_len,
                    CompletionCallback(), truncate));
     ret_value = buf_len;
   } else {