Fix the use of SocketDataProviders in HttpNetworkTransactionTest.GenerateAuthToken

net/http/http_network_transaction_unittest.cc

Index: net/http/http_network_transaction_unittest.cc
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 09919406616830dae1f7790e2fc6f618ca2b169f..518d07cb770e4b5667fa7714aa79a3d501aac757 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -10132,35 +10132,47 @@ TEST_P(HttpNetworkTransactionTest, GenerateAuthToken) {
     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;
@@ -10186,6 +10198,7 @@ TEST_P(HttpNetworkTransactionTest, GenerateAuthToken) {
         EXPECT_TRUE(response->auth_challenge.get() == NULL);
       }
     }
+    STLDeleteElements(&data_providers);
   }
 }