Revert 97821 - Do not call callback->Run() until we are about to return from this

object because callback->Run() may cause this object to be deleted.

Previously we were calling callback->Run() in DoWaitForDataReadyDone(),
which returns to the do-while loop in DoLoop().  The do-while loop
reads the state_ member. If the object was deleted in callback->Run(),
we would be reading state_ after free.

Miscellaneous cleanups: omit net::, and use the assign() method of
std::string to avoid constructing a temporary object.

R=agl@chromium.org,rvargas@chromium.org
BUG=84426
TEST=net_unittests --gtest_filter=DiskCacheBasedSSLHostInfo.DeleteInCallback

Review URL: http://codereview.chromium.org/7648007

TBR=wtc@chromium.org
Review URL: http://codereview.chromium.org/7709022

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

3 files changed, 15 insertions, 61 deletions

diff --git a/net/http/disk_cache_based_ssl_host_info.cc b/net/http/disk_cache_based_ssl_host_info.cc
index 55f471d5..2b509ca 100644
--- a/net/http/disk_cache_based_ssl_host_info.cc
+++ b/net/http/disk_cache_based_ssl_host_info.cc
@@ -41,7 +41,7 @@ DiskCacheBasedSSLHostInfo::DiskCacheBasedSSLHostInfo(
     : SSLHostInfo(hostname, ssl_config, cert_verifier),
       weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       callback_(new CallbackImpl(weak_ptr_factory_.GetWeakPtr(),
-                                 &DiskCacheBasedSSLHostInfo::OnIOComplete)),
+                                 &DiskCacheBasedSSLHostInfo::DoLoop)),
       state_(GET_BACKEND),
       ready_(false),
       hostname_(hostname),
@@ -98,17 +98,7 @@ std::string DiskCacheBasedSSLHostInfo::key() const {
   return "sslhostinfo:" + hostname_;
 }
 
-void DiskCacheBasedSSLHostInfo::OnIOComplete(int rv) {
-  rv = DoLoop(rv);
-  if (rv != ERR_IO_PENDING) {
-    CompletionCallback* callback = user_callback_;
-    user_callback_ = NULL;
-    if (callback)
-      callback->Run(rv);
-  }
-}
-
-int DiskCacheBasedSSLHostInfo::DoLoop(int rv) {
+void DiskCacheBasedSSLHostInfo::DoLoop(int rv) {
   do {
     switch (state_) {
       case GET_BACKEND:
@@ -152,8 +142,6 @@ int DiskCacheBasedSSLHostInfo::DoLoop(int rv) {
         NOTREACHED();
     }
   } while (rv != ERR_IO_PENDING && state_ != NONE);
-
-  return rv;
 }
 
 int DiskCacheBasedSSLHostInfo::DoGetBackendComplete(int rv) {
@@ -179,7 +167,7 @@ int DiskCacheBasedSSLHostInfo::DoOpenComplete(int rv) {
 
 int DiskCacheBasedSSLHostInfo::DoReadComplete(int rv) {
   if (rv > 0)
-    data_.assign(read_buffer_->data(), rv);
+    data_ = std::string(read_buffer_->data(), rv);
 
   state_ = WAIT_FOR_DATA_READY_DONE;
   return OK;
@@ -239,15 +227,23 @@ int DiskCacheBasedSSLHostInfo::DoCreate() {
 }
 
 int DiskCacheBasedSSLHostInfo::DoWaitForDataReadyDone() {
+  CompletionCallback* callback;
+
   DCHECK(!ready_);
   state_ = NONE;
   ready_ = true;
+  callback = user_callback_;
+  user_callback_ = NULL;
   // We close the entry because, if we shutdown before ::Persist is called,
   // then we might leak a cache reference, which causes a DCHECK on shutdown.
   if (entry_)
     entry_->Close();
   entry_ = NULL;
   Parse(data_);
+
+  if (callback)
+    callback->Run(OK);
+
   return OK;
 }
 
diff --git a/net/http/disk_cache_based_ssl_host_info.h b/net/http/disk_cache_based_ssl_host_info.h
index 873e4bd..c28d403 100644
--- a/net/http/disk_cache_based_ssl_host_info.h
+++ b/net/http/disk_cache_based_ssl_host_info.h
@@ -79,9 +79,7 @@ class DiskCacheBasedSSLHostInfo : public SSLHostInfo,
 
   std::string key() const;
 
-  void OnIOComplete(int rv);
-
-  int DoLoop(int rv);
+  void DoLoop(int rv);
 
   int DoGetBackendComplete(int rv);
   int DoOpenComplete(int rv);
@@ -114,8 +112,8 @@ class DiskCacheBasedSSLHostInfo : public SSLHostInfo,
   disk_cache::Backend* backend_;
   disk_cache::Entry* entry_;
   CompletionCallback* user_callback_;
-  scoped_refptr<IOBuffer> read_buffer_;
-  scoped_refptr<IOBuffer> write_buffer_;
+  scoped_refptr<net::IOBuffer> read_buffer_;
+  scoped_refptr<net::IOBuffer> write_buffer_;
   std::string data_;
 };
 
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 6ea1be2..dceabf8 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -16,9 +16,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_log_unittest.h"
 #include "net/base/ssl_cert_request_info.h"
-#include "net/base/ssl_config_service.h"
 #include "net/disk_cache/disk_cache.h"
-#include "net/http/disk_cache_based_ssl_host_info.h"
 #include "net/http/http_byte_range.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_request_info.h"
@@ -1040,7 +1038,7 @@ struct Context {
 
 
 //-----------------------------------------------------------------------------
-// HttpCache tests
+// tests
 
 TEST(HttpCache, CreateThenDestroy) {
   MockHttpCache cache;
@@ -5182,41 +5180,3 @@ TEST(HttpCache, TruncatedByContentLength2) {
   EXPECT_TRUE(truncated);
   entry->Close();
 }
-
-//-----------------------------------------------------------------------------
-// DiskCacheBasedSSLHostInfo tests
-
-class DeleteSSLHostInfoCompletionCallback : public TestCompletionCallback {
- public:
-  explicit DeleteSSLHostInfoCompletionCallback(net::SSLHostInfo* ssl_host_info)
-      : ssl_host_info_(ssl_host_info) {}
-
-  virtual void RunWithParams(const Tuple1<int>& params) {
-    delete ssl_host_info_;
-    TestCompletionCallback::RunWithParams(params);
-  }
-
- private:
-  net::SSLHostInfo* ssl_host_info_;
-};
-
-// Tests that we can delete a DiskCacheBasedSSLHostInfo object in a
-// completion callback for DiskCacheBasedSSLHostInfo::WaitForDataReady.
-TEST(DiskCacheBasedSSLHostInfo, DeleteInCallback) {
-  net::CertVerifier cert_verifier;
-  // Use the blocking mock backend factory to force asynchronous completion
-  // of ssl_host_info->WaitForDataReady(), so that the callback will run.
-  MockBlockingBackendFactory* factory = new MockBlockingBackendFactory();
-  MockHttpCache cache(factory);
-  net::SSLConfig ssl_config;
-  net::SSLHostInfo* ssl_host_info =
-      new net::DiskCacheBasedSSLHostInfo("https://www.verisign.com", ssl_config,
-                                         &cert_verifier, cache.http_cache());
-  ssl_host_info->Start();
-  DeleteSSLHostInfoCompletionCallback callback(ssl_host_info);
-  int rv = ssl_host_info->WaitForDataReady(&callback);
-  EXPECT_EQ(net::ERR_IO_PENDING, rv);
-  // Now complete the backend creation and let the callback run.
-  factory->FinishCreation();
-  EXPECT_EQ(net::OK, callback.GetResult(rv));
-}