Fix the use of SocketDataProviders in HttpNetworkTransactionTest.GenerateAuthToken

net/http/http_network_transaction_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_network_transaction.h"
 
 #include <math.h>  // ceil
 #include <stdarg.h>
 #include <string>
 #include <vector>
@@ skipping 10114 matching lines @@
     }
 
     HttpRequestInfo request;
     request.method = "GET";
     request.url = GURL(test_config.server_url);
     request.load_flags = 0;
 
     scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
     HttpNetworkTransaction trans(DEFAULT_PRIORITY, session.get());
 
+    SSLSocketDataProvider ssl_socket_data_provider(SYNCHRONOUS, OK);
+
+    std::vector<std::vector<MockRead>> mock_reads(1);
+    std::vector<std::vector<MockWrite>> mock_writes(1);
     for (int round = 0; round < test_config.num_auth_rounds; ++round) {
       const TestRound& read_write_round = test_config.rounds[round];
 
       // Set up expected reads and writes.
-      MockRead reads[2];
-      reads[0] = read_write_round.read;
-      size_t length_reads = 1;
-      if (read_write_round.extra_read) {
-        reads[1] = *read_write_round.extra_read;
-        length_reads = 2;
+      mock_reads.back().push_back(read_write_round.read);
+      mock_writes.back().push_back(read_write_round.write);
+
+      if (read_write_round.read.data == kProxyChallenge.data &&

[cbentzel, 2015/05/21 00:10:25]

This isn't immediately obvious to me what is going on here. Could you add a
comment for why this is being done? In the interim, I'll try to figure out why.

[cbentzel, 2015/05/21 00:22:04]

Ah - I'm guessing this is due to Proxy-Connection: Close and needing to create a
new StaticSocketDataProvider down below to be used for the new socket. I think a
comment would likely help future spelunkers here. 

[Ryan Hamilton, 2015/05/21 02:31:24]

You got it. Added a comment. (Good idea)

[Ryan Hamilton, 2015/05/21 02:31:24]

So kProxyChallenge has Proxy-Connection: close which results in the connection
being closed. However, I don't understand why I need the second clause. Do you
know? If the write is kConnect and the read is kProxyChallenge the connection is
not closed. Is this a bug?

[cbentzel, 2015/05/21 10:10:05]

I don't know why off the top of my head. This is worth investigating. 

+          read_write_round.write.data != kConnect.data) {
+        mock_reads.push_back(std::vector<MockRead>());
+        mock_writes.push_back(std::vector<MockWrite>());
       }
 
-      MockWrite writes[2];
-      writes[0] = read_write_round.write;
-      size_t length_writes = 1;
+      if (read_write_round.extra_read) {

[cbentzel, 2015/05/21 00:22:04]

It doesn't look like there is ever a case where a round includes both
kProxyChallenge and extra_read/extra_write, but I was wondering about whether
this should go above the push_back statements (so it would apply to the current
socket, not the new socket).

Not a huge deal either way since it doesn't look like that combination happens,
but wondering if ytou had considered it.

[Ryan Hamilton, 2015/05/21 02:31:24]

I *think* it's correct as is, because if there was an extra read/write it would
happen after the Proxy-Connection: close response was processed which *should*
mean the old socket was closed, right? Happy to revisit this if you think it
best.

[cbentzel, 2015/05/21 10:10:05]

This seems reasonable.

+        mock_reads.back().push_back(*read_write_round.extra_read);
+      }
       if (read_write_round.extra_write) {
-        writes[1] = *read_write_round.extra_write;
-        length_writes = 2;
+        mock_writes.back().push_back(*read_write_round.extra_write);
       }
-      StaticSocketDataProvider data_provider(
-          reads, length_reads, writes, length_writes);
-      session_deps_.socket_factory->AddSocketDataProvider(&data_provider);

[cbentzel, 2015/05/21 00:10:25]

Wow, I'm really surprised this worked. 

Here's what I think was happening (which you also covered in the bug, just
trying to echo back).

- First round, we create StaticSocketDataProvider "A", it get's used, destructor
is called, all is pretty OK so far.

- Second round, we create StaticSocketDataProvider "B". This just happens to be
at the same address as "A". The MockSocket used to be using "A", but because "B"
just happens to line up at the same memory address we use it instead. There's
also an additional reference to it in MockClientSocketFactory::mock_data_, but
this additional reference is never used since we don't create another socket.

[note that I wonder if ~MockClientSocketFactory should check that all of
mock_data_ and mock_ssl_data_ are consumed]

- Subsequent rounds go through similar spells.

Yeah, that's weird.

[Ryan Hamilton, 2015/05/21 02:31:24]

Yeah, you got it exactly right! 

I don't think we can do the check in ~MockClientSocketFactory because there are
actually many tests (mostly of error behavior, I think) in which we don't
consume all data. 

 
       // Add an SSL sequence if necessary.
-      SSLSocketDataProvider ssl_socket_data_provider(SYNCHRONOUS, OK);
       if (round >= test_config.first_ssl_round)
         session_deps_.socket_factory->AddSSLSocketDataProvider(
             &ssl_socket_data_provider);
+    }
 
+    std::vector<StaticSocketDataProvider*> data_providers;

[cbentzel, 2015/05/21 00:10:25]

Would ScopedVector<StaticSocketDataProvider> work here instead - so you can save
the STLDeleteElements below? 

[Ryan Hamilton, 2015/05/21 02:31:24]

Ooo! nice!

+    for (size_t i = 0; i < mock_reads.size(); ++i) {
+      data_providers.push_back(new StaticSocketDataProvider(
+          vector_as_array(&mock_reads[i]), mock_reads[i].size(),
+          vector_as_array(&mock_writes[i]), mock_writes[i].size()));
+      session_deps_.socket_factory->AddSocketDataProvider(
+          data_providers.back());
+    }
+
+    for (int round = 0; round < test_config.num_auth_rounds; ++round) {
+      const TestRound& read_write_round = test_config.rounds[round];
       // Start or restart the transaction.
       TestCompletionCallback callback;
       int rv;
       if (round == 0) {
         rv = trans.Start(&request, callback.callback(), BoundNetLog());
       } else {
         rv = trans.RestartWithAuth(
             AuthCredentials(kFoo, kBar), callback.callback());
       }
       if (rv == ERR_IO_PENDING)
         rv = callback.WaitForResult();
 
       // Compare results with expected data.
       EXPECT_EQ(read_write_round.expected_rv, rv);
       const HttpResponseInfo* response = trans.GetResponseInfo();
       if (read_write_round.expected_rv != OK) {
         EXPECT_EQ(round + 1, test_config.num_auth_rounds);
         continue;
       }
       if (round + 1 < test_config.num_auth_rounds) {
         EXPECT_FALSE(response->auth_challenge.get() == NULL);
       } else {
         EXPECT_TRUE(response->auth_challenge.get() == NULL);
       }
     }
+    STLDeleteElements(&data_providers);
   }
 }
 
 TEST_P(HttpNetworkTransactionTest, MultiRoundAuth) {
   // Do multi-round authentication and make sure it works correctly.
   HttpAuthHandlerMock::Factory* auth_factory(
       new HttpAuthHandlerMock::Factory());
   session_deps_.http_auth_handler_factory.reset(auth_factory);
   session_deps_.proxy_service.reset(ProxyService::CreateDirect());
   session_deps_.host_resolver->rules()->AddRule("www.example.com", "10.0.0.1");
@@ skipping 4040 matching lines @@
   ASSERT_TRUE(response);
   ASSERT_TRUE(response->headers.get());
 
   EXPECT_EQ(101, response->headers->response_code());
 
   trans.reset();
   session->CloseAllConnections();
 }
 
 }  // namespace net