From f6c9d56c4b81eb282e591cc079622697ed99b5ed Mon Sep 17 00:00:00 2001 From: "rvargas@google.com" Date: Tue, 15 Jan 2013 19:28:13 +0000 Subject: Http Cache: Release all references to buffers before invoking callbacks. BUG=131272 TEST=net_unittests Review URL: https://codereview.chromium.org/11854020 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@176951 0039d316-1c4b-4281-b951-d872f2087c98 --- net/base/test_completion_callback.cc | 20 ++++++++++++++++++-- net/base/test_completion_callback.h | 15 +++++++++++++++ net/disk_cache/entry_unittest.cc | 20 +------------------- net/http/http_cache_transaction.cc | 2 ++ net/http/http_cache_unittest.cc | 23 +++++++++++++++++++++++ 5 files changed, 59 insertions(+), 21 deletions(-) diff --git a/net/base/test_completion_callback.cc b/net/base/test_completion_callback.cc index c226478..2ee4c8d 100644 --- a/net/base/test_completion_callback.cc +++ b/net/base/test_completion_callback.cc @@ -8,6 +8,7 @@ #include "base/bind_helpers.h" #include "base/compiler_specific.h" #include "base/message_loop.h" +#include "net/base/io_buffer.h" namespace net { @@ -42,7 +43,8 @@ TestCompletionCallback::TestCompletionCallback() base::Unretained(this)))) { } -TestCompletionCallback::~TestCompletionCallback() {} +TestCompletionCallback::~TestCompletionCallback() { +} TestInt64CompletionCallback::TestInt64CompletionCallback() : ALLOW_THIS_IN_INITIALIZER_LIST(callback_( @@ -50,6 +52,20 @@ TestInt64CompletionCallback::TestInt64CompletionCallback() base::Unretained(this)))) { } -TestInt64CompletionCallback::~TestInt64CompletionCallback() {} +TestInt64CompletionCallback::~TestInt64CompletionCallback() { +} + +ReleaseBufferCompletionCallback::ReleaseBufferCompletionCallback( + IOBuffer* buffer) : buffer_(buffer) { +} + +ReleaseBufferCompletionCallback::~ReleaseBufferCompletionCallback() { +} + +void ReleaseBufferCompletionCallback::SetResult(int result) { + if (!buffer_->HasOneRef()) + result = net::ERR_FAILED; + TestCompletionCallback::SetResult(result); +} } // namespace net diff --git a/net/base/test_completion_callback.h b/net/base/test_completion_callback.h index c886cfb..4a0afe1 100644 --- a/net/base/test_completion_callback.h +++ b/net/base/test_completion_callback.h @@ -24,6 +24,8 @@ namespace net { +class IOBuffer; + namespace internal { class TestCompletionCallbackBaseInternal { @@ -108,6 +110,19 @@ class TestInt64CompletionCallback : public TestInt64CompletionCallbackBase { DISALLOW_COPY_AND_ASSIGN(TestInt64CompletionCallback); }; +// Makes sure that the buffer is not referenced when the callback runs. +class ReleaseBufferCompletionCallback: public TestCompletionCallback { + public: + explicit ReleaseBufferCompletionCallback(IOBuffer* buffer); + virtual ~ReleaseBufferCompletionCallback(); + + private: + virtual void SetResult(int result) OVERRIDE; + + IOBuffer* buffer_; + DISALLOW_COPY_AND_ASSIGN(ReleaseBufferCompletionCallback); +}; + } // namespace net #endif // NET_BASE_TEST_COMPLETION_CALLBACK_H_ diff --git a/net/disk_cache/entry_unittest.cc b/net/disk_cache/entry_unittest.cc index 7a581bd..6438f2e 100644 --- a/net/disk_cache/entry_unittest.cc +++ b/net/disk_cache/entry_unittest.cc @@ -510,24 +510,6 @@ TEST_F(DiskCacheEntryTest, MemoryOnlyExternalAsyncIO) { ExternalAsyncIO(); } -// Makes sure that the buffer is not referenced when the callback runs. -class ReleaseBufferCompletionCallback: public net::TestCompletionCallback { - public: - explicit ReleaseBufferCompletionCallback(net::IOBuffer* buffer) - : buffer_(buffer) { - } - - private: - virtual void SetResult(int result) OVERRIDE { - if (!buffer_->HasOneRef()) - result = net::ERR_FAILED; - TestCompletionCallback::SetResult(result); - } - - net::IOBuffer* buffer_; - DISALLOW_COPY_AND_ASSIGN(ReleaseBufferCompletionCallback); -}; - // Tests that IOBuffers are not referenced after IO completes. void DiskCacheEntryTest::ReleaseBuffer() { disk_cache::Entry* entry = NULL; @@ -538,7 +520,7 @@ void DiskCacheEntryTest::ReleaseBuffer() { scoped_refptr buffer(new net::IOBuffer(kBufferSize)); CacheTestFillBuffer(buffer->data(), kBufferSize, false); - ReleaseBufferCompletionCallback cb(buffer); + net::ReleaseBufferCompletionCallback cb(buffer); int rv = entry->WriteData(0, 0, buffer, kBufferSize, cb.callback(), false); EXPECT_EQ(kBufferSize, cb.GetResult(rv)); entry->Close(); diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc index 624c08f..62f1ee6 100644 --- a/net/http/http_cache_transaction.cc +++ b/net/http/http_cache_transaction.cc @@ -470,6 +470,8 @@ void HttpCache::Transaction::DoCallback(int rv) { DCHECK(rv != ERR_IO_PENDING); DCHECK(!callback_.is_null()); + read_buf_ = NULL; // Release the buffer before invoking the callback. + // Since Run may result in Read being called, clear callback_ up front. CompletionCallback c = callback_; callback_.Reset(); diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc index 51098b4..2225576 100644 --- a/net/http/http_cache_unittest.cc +++ b/net/http/http_cache_unittest.cc @@ -552,6 +552,29 @@ TEST(HttpCache, SimpleGETNoDiskCache2) { EXPECT_FALSE(cache.http_cache()->GetCurrentBackend()); } +// Tests that IOBuffers are not referenced after IO completes. +TEST(HttpCache, ReleaseBuffer) { + MockHttpCache cache; + + // Write to the cache. + RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction); + + MockHttpRequest request(kSimpleGET_Transaction); + scoped_ptr trans; + int rv = cache.http_cache()->CreateTransaction(&trans, NULL); + ASSERT_EQ(net::OK, rv); + + const int kBufferSize = 10; + scoped_refptr buffer(new net::IOBuffer(kBufferSize)); + net::ReleaseBufferCompletionCallback cb(buffer); + + rv = trans->Start(&request, cb.callback(), net::BoundNetLog()); + EXPECT_EQ(net::OK, cb.GetResult(rv)); + + rv = trans->Read(buffer, kBufferSize, cb.callback()); + EXPECT_EQ(kBufferSize, cb.GetResult(rv)); +} + TEST(HttpCache, SimpleGETWithDiskFailures) { MockHttpCache cache; -- cgit v1.1