Handle the case when StopCaching was called in DoneReading

If StopCaching is called, it is possible for mode_ to be NONE while entry_ is
non-NULL and points to an entry in write mode. Don't incorrectly close it as a
read entry (thus crashing) in DoneReading. Add a test which stresses this case.

BUG=295638
TEST=HttpCache.StopCachingThenDoneReadingDeletesEntry,
     downloading a large-ish download with gzip compression doesn't crash

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

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

2 files changed, 49 insertions, 3 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index a710afb..83efcb1 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -452,7 +452,10 @@ void HttpCache::Transaction::DoneReading() {
     DCHECK_NE(mode_, UPDATE);
     if (mode_ & WRITE) {
       DoneWritingToEntry(true);
-    } else {
+    } else if (mode_ & READ) {
+      // It is necessary to check mode_ & READ because it is possible
+      // for mode_ to be NONE and entry_ non-NULL with a write entry
+      // if StopCaching was called.
       cache_->DoneReadingFromEntry(entry_, this);
       entry_ = NULL;
     }
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index cc4874f..e47852c 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -5960,7 +5960,7 @@ TEST(HttpCache, StopCachingDeletesEntry) {
 
     scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(256));
     rv = trans->Read(buf.get(), 10, callback.callback());
-    EXPECT_EQ(callback.GetResult(rv), 10);
+    EXPECT_EQ(10, callback.GetResult(rv));
 
     trans->StopCaching();
 
@@ -5968,7 +5968,50 @@ TEST(HttpCache, StopCachingDeletesEntry) {
     rv = trans->Read(buf.get(), 256, callback.callback());
     EXPECT_GT(callback.GetResult(rv), 0);
     rv = trans->Read(buf.get(), 256, callback.callback());
-    EXPECT_EQ(callback.GetResult(rv), 0);
+    EXPECT_EQ(0, callback.GetResult(rv));
+  }
+
+  // Make sure that the ActiveEntry is gone.
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Verify that the entry is gone.
+  RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
+
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+}
+
+// Tests that we stop caching when told, even if DoneReading is called
+// after StopCaching.
+TEST(HttpCache, StopCachingThenDoneReadingDeletesEntry) {
+  MockHttpCache cache;
+  net::TestCompletionCallback callback;
+  MockHttpRequest request(kSimpleGET_Transaction);
+
+  {
+    scoped_ptr<net::HttpTransaction> trans;
+    int rv = cache.http_cache()->CreateTransaction(
+        net::DEFAULT_PRIORITY, &trans, NULL);
+    EXPECT_EQ(net::OK, rv);
+
+    rv = trans->Start(&request, callback.callback(), net::BoundNetLog());
+    EXPECT_EQ(net::OK, callback.GetResult(rv));
+
+    scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(256));
+    rv = trans->Read(buf.get(), 10, callback.callback());
+    EXPECT_EQ(10, callback.GetResult(rv));
+
+    trans->StopCaching();
+
+    // We should be able to keep reading.
+    rv = trans->Read(buf.get(), 256, callback.callback());
+    EXPECT_GT(callback.GetResult(rv), 0);
+    rv = trans->Read(buf.get(), 256, callback.callback());
+    EXPECT_EQ(0, callback.GetResult(rv));
+
+    // We should be able to call DoneReading.
+    trans->DoneReading();
   }
 
   // Make sure that the ActiveEntry is gone.