Fully reset HttpNetworkTransaction::response_ when restarting the transaction. The reset is done using the default constructor.

This is less fragile than resetting each member manually, and in fact not all members were being reset (for example, vary_data and ssl_info).

In the case of vary_data this could cause a subtle glitch, since calling HttpVaryData::Init() twice on the same object doesn't fully re-initialize.

The changes outside of http_network_transaction.cc are just a safety net -- it seemed reasonable to make HttpVaryData::Init() support the multiple-init model.
Review URL: http://codereview.chromium.org/50031

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

8 files changed, 98 insertions, 11 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 04ee364..2a41c6a 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -651,7 +651,6 @@ int HttpNetworkTransaction::DoWriteHeaders() {
   // out the first bytes of the request headers.
   if (request_headers_bytes_sent_ == 0) {
     response_.request_time = Time::Now();
-    response_.was_cached = false;
   }
 
   const char* buf = request_headers_.data() + request_headers_bytes_sent_;
@@ -1178,9 +1177,8 @@ void HttpNetworkTransaction::ResetStateForRestart() {
   request_headers_.clear();
   request_headers_bytes_sent_ = 0;
   chunked_decoder_.reset();
-  // Reset the scoped_refptr
-  response_.headers = NULL;
-  response_.auth_challenge = NULL;
+  // Reset all the members of response_.
+  response_ = HttpResponseInfo();
 }
 
 bool HttpNetworkTransaction::ShouldResendRequest() {
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index 56dc54a..9a0b8e9 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -17,6 +17,7 @@
 #include "net/http/http_response_info.h"
 #include "net/http/http_transaction.h"
 #include "net/proxy/proxy_service.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 
 namespace net {
 
@@ -45,6 +46,8 @@ class HttpNetworkTransaction : public HttpTransaction {
   virtual uint64 GetUploadProgress() const;
 
  private:
+  FRIEND_TEST(HttpNetworkTransactionTest, ResetStateForRestart);
+
   void BuildRequestHeaders();
   void BuildTunnelRequest();
   void DoCallback(int result);
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 337f6c7..340dfe4 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -18,6 +18,10 @@
 
 //-----------------------------------------------------------------------------
 
+namespace net {
+
+// TODO(eroman): Now that this is inside the net namespace, remove the redundant
+// net:: qualifiers.
 
 struct MockConnect {
   // Asynchronous connection success.
@@ -2560,3 +2564,64 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthCacheAndPreauth) {
     EXPECT_EQ(100, response->headers->GetContentLength());
   }
 }
+
+// Test the ResetStateForRestart() private method.
+TEST_F(HttpNetworkTransactionTest, ResetStateForRestart) {
+  // Create a transaction (the dependencies aren't important).
+  scoped_ptr<ProxyService> proxy_service(CreateNullProxyService());
+  scoped_ptr<HttpNetworkTransaction> trans(new HttpNetworkTransaction(
+      CreateSession(proxy_service.get()), &mock_socket_factory));
+
+  // Setup some state (which we expect ResetStateForRestart() will clear).
+  trans->header_buf_.reset(static_cast<char*>(malloc(10)));
+  trans->header_buf_capacity_ = 10;
+  trans->header_buf_len_ = 3;
+  trans->header_buf_body_offset_ = 11;
+  trans->header_buf_http_offset_ = 0;
+  trans->response_body_length_ = 100;
+  trans->response_body_read_ = 1;
+  trans->read_buf_ = new IOBuffer(15);
+  trans->read_buf_len_ = 15;
+  trans->request_headers_ = "Authorization: NTLM";
+  trans->request_headers_bytes_sent_ = 3;
+
+  // Setup state in response_
+  trans->response_.auth_challenge = new AuthChallengeInfo();
+  trans->response_.ssl_info.cert_status = -15;
+  trans->response_.response_time = base::Time::Now();
+  trans->response_.was_cached = true; // (Wouldn't ever actually be true...)
+
+  { // Setup state for response_.vary_data
+    HttpRequestInfo request;
+    std::string temp("HTTP/1.1 200 OK\nVary: foo, bar\n\n");
+    std::replace(temp.begin(), temp.end(), '\n', '\0');
+    scoped_refptr<HttpResponseHeaders> response = new HttpResponseHeaders(temp);
+    request.extra_headers = "Foo: 1\nbar: 23";
+    EXPECT_TRUE(trans->response_.vary_data.Init(request, *response));
+  }
+
+  // Cause the above state to be reset.
+  trans->ResetStateForRestart();
+
+  // Verify that the state that needed to be reset, has been reset.
+  EXPECT_EQ(NULL, trans->header_buf_.get());
+  EXPECT_EQ(0, trans->header_buf_capacity_);
+  EXPECT_EQ(0, trans->header_buf_len_);
+  EXPECT_EQ(-1, trans->header_buf_body_offset_);
+  EXPECT_EQ(-1, trans->header_buf_http_offset_);
+  EXPECT_EQ(-1, trans->response_body_length_);
+  EXPECT_EQ(0, trans->response_body_read_);
+  EXPECT_EQ(NULL, trans->read_buf_.get());
+  EXPECT_EQ(0, trans->read_buf_len_);
+  EXPECT_EQ("", trans->request_headers_);
+  EXPECT_EQ(0U, trans->request_headers_bytes_sent_);
+  EXPECT_EQ(NULL, trans->response_.auth_challenge.get());
+  EXPECT_EQ(NULL, trans->response_.headers.get());
+  EXPECT_EQ(false, trans->response_.was_cached);
+  EXPECT_EQ(base::kInvalidPlatformFileValue,
+            trans->response_.response_data_file);
+  EXPECT_EQ(0, trans->response_.ssl_info.cert_status);
+  EXPECT_FALSE(trans->response_.vary_data.is_valid());
+}
+
+}  // namespace net
diff --git a/net/http/http_response_info.cc b/net/http/http_response_info.cc
index 536d42b..a94aa91 100644
--- a/net/http/http_response_info.cc
+++ b/net/http/http_response_info.cc
@@ -8,7 +8,7 @@
 namespace net {
 
 HttpResponseInfo::HttpResponseInfo()
-    : response_data_file(base::kInvalidPlatformFileValue) {
+    : was_cached(false), response_data_file(base::kInvalidPlatformFileValue) {
 }
 
 HttpResponseInfo::~HttpResponseInfo() {
diff --git a/net/http/http_response_info.h b/net/http/http_response_info.h
index caa49df..d2803d1 100644
--- a/net/http/http_response_info.h
+++ b/net/http/http_response_info.h
@@ -19,8 +19,9 @@ class HttpResponseInfo {
  public:
   HttpResponseInfo();
   ~HttpResponseInfo();
+  // Default copy-constructor and assignment operator are OK!
 
-  // The following is only defined if the request_time memmber is set.
+  // The following is only defined if the request_time member is set.
   // If this response was resurrected from cache, then this bool is set, and
   // request_time may corresponds to a time "far" in the past.  Note that
   // stale content (perhaps un-cacheable) may be fetched from cache subject to
diff --git a/net/http/http_vary_data.cc b/net/http/http_vary_data.cc
index 765ff11..fa7a325 100644
--- a/net/http/http_vary_data.cc
+++ b/net/http/http_vary_data.cc
@@ -15,7 +15,6 @@
 namespace net {
 
 HttpVaryData::HttpVaryData() : is_valid_(false) {
-  memset(&request_digest_, 0, sizeof(request_digest_));
 }
 
 bool HttpVaryData::Init(const HttpRequestInfo& request_info,
@@ -23,6 +22,7 @@ bool HttpVaryData::Init(const HttpRequestInfo& request_info,
   MD5Context ctx;
   MD5Init(&ctx);
 
+  is_valid_ = false;
   bool processed_header = false;
 
   // Feed the MD5 context in the order of the Vary header enumeration.  If the
@@ -64,6 +64,7 @@ bool HttpVaryData::Init(const HttpRequestInfo& request_info,
 }
 
 bool HttpVaryData::InitFromPickle(const Pickle& pickle, void** iter) {
+  is_valid_ = false;
   const char* data;
   if (pickle.ReadBytes(iter, &data, sizeof(request_digest_))) {
     memcpy(&request_digest_, data, sizeof(request_digest_));
@@ -73,6 +74,7 @@ bool HttpVaryData::InitFromPickle(const Pickle& pickle, void** iter) {
 }
 
 void HttpVaryData::Persist(Pickle* pickle) const {
+  DCHECK(is_valid());
   pickle->WriteBytes(&request_digest_, sizeof(request_digest_));
 }
 
diff --git a/net/http/http_vary_data.h b/net/http/http_vary_data.h
index a96abe0..360799d 100644
--- a/net/http/http_vary_data.h
+++ b/net/http/http_vary_data.h
@@ -38,7 +38,7 @@ class HttpVaryData {
   // Vary header was not empty and did not contain the '*' value.  Upon
   // success, the object is also marked as valid such that is_valid() will
   // return true.  Otherwise, false is returned to indicate that this object
-  // was not modified.
+  // is marked as invalid.
   //
   bool Init(const HttpRequestInfo& request_info,
             const HttpResponseHeaders& response_headers);
@@ -48,11 +48,12 @@ class HttpVaryData {
   //
   // Upon success, true is returned and the object is marked as valid such that
   // is_valid() will return true.  Otherwise, false is returned to indicate
-  // that this object was not modified.
+  // that this object is marked as invalid.
   //
   bool InitFromPickle(const Pickle& pickle, void** pickle_iter);
 
-  // Call this method to persist the vary data.
+  // Call this method to persist the vary data. Illegal to call this on an
+  // invalid object.
   void Persist(Pickle* pickle) const;
 
   // Call this method to test if the given request matches the previous request
diff --git a/net/http/http_vary_data_unittest.cc b/net/http/http_vary_data_unittest.cc
index 852d7d4..7b2bd16 100644
--- a/net/http/http_vary_data_unittest.cc
+++ b/net/http/http_vary_data_unittest.cc
@@ -29,7 +29,7 @@ struct TestTransaction {
 
 }  // namespace
 
-TEST(HttpVaryDataTest, IsValid) {
+TEST(HttpVaryDataTest, IsInvalid) {
   // All of these responses should result in an invalid vary data object.
   const char* kTestResponses[] = {
     "HTTP/1.1 200 OK\n\n",
@@ -43,11 +43,28 @@ TEST(HttpVaryDataTest, IsValid) {
     t.Init("", kTestResponses[i]);
 
     net::HttpVaryData v;
+    EXPECT_FALSE(v.is_valid());
     EXPECT_FALSE(v.Init(t.request, *t.response));
     EXPECT_FALSE(v.is_valid());
   }
 }
 
+TEST(HttpVaryDataTest, MultipleInit) {
+  net::HttpVaryData v;
+
+  // Init to something valid.
+  TestTransaction t1;
+  t1.Init("Foo: 1\nbar: 23", "HTTP/1.1 200 OK\nVary: foo, bar\n\n");
+  EXPECT_TRUE(v.Init(t1.request, *t1.response));
+  EXPECT_TRUE(v.is_valid());
+
+  // Now overwrite by initializing to something invalid.
+  TestTransaction t2;
+  t2.Init("Foo: 1\nbar: 23", "HTTP/1.1 200 OK\nVary: *\n\n");
+  EXPECT_FALSE(v.Init(t2.request, *t2.response));
+  EXPECT_FALSE(v.is_valid());
+}
+
 TEST(HttpVaryDataTest, DoesVary) {
   TestTransaction a;
   a.Init("Foo: 1", "HTTP/1.1 200 OK\nVary: foo\n\n");