Introduce QuicStreamFactoryTest::DefaultProofVerifyDetails().

net/quic/quic_stream_factory_test.cc

Index: net/quic/quic_stream_factory_test.cc
diff --git a/net/quic/quic_stream_factory_test.cc b/net/quic/quic_stream_factory_test.cc
index 6bb57e8aa529683d3848aa72a85133389cdb349b..c45a6c36dcaf105a1cc674647bca4e99927a14c2 100644
--- a/net/quic/quic_stream_factory_test.cc
+++ b/net/quic/quic_stream_factory_test.cc
@@ -305,6 +305,21 @@ class QuicStreamFactoryTest : public ::testing::TestWithParam<TestParams> {
         AdjustErrorForVersion(QUIC_RST_ACKNOWLEDGEMENT, GetParam().version));
   }
 
+  // Note that MockCryptoClientStreamFactory::AddProofVerifyDetails() does not

[Ryan Hamilton, 2015/05/19 19:16:56]

I don't understand what this comment means.  What is a "named object"?

[Bence, 2015/05/19 19:45:33]

I'm landing without the comment as you suggested, but I'd like to add this in
the next CL.  Please help me with the wording here.  I'm trying to say that
crypto_client_stream_factory_.AddProofVerifyDetails(DefaultProofVerifyDetails())
would not work, because the argument is a temporary that gets destroyed right
after the call finished.  Instead, the test need to have an object in local
scope that remains valid while the test runs.

Or is it better to proactively change the implementation to prevent
use-after-free error?

[Ryan Hamilton, 2015/05/19 19:55:34]

Will that actually compile? AddProofVerifyDetails takes a ProofVerifyDetails*
but DefaultProofVerifyDetails() returns a ProofVerifyDetails (pointer vs object)

+  // take ownership of ProofVerifyDetailsChromium, therefore it has to be a
+  // named object so that it is valid until the end of the test.
+  static ProofVerifyDetailsChromium DefaultProofVerifyDetails() {
+    // Load a certificate that is valid for www.example.org, mail.example.org,
+    // and mail.example.com.
+    scoped_refptr<X509Certificate> test_cert(
+        ImportCertFromFile(GetTestCertsDirectory(), "spdy_pooling.pem"));
+    EXPECT_TRUE(test_cert.get());
+    ProofVerifyDetailsChromium verify_details;
+    verify_details.cert_verify_result.verified_cert = test_cert;
+    verify_details.cert_verify_result.is_issued_by_known_root = true;
+    return verify_details;
+  }
+
   MockQuicServerInfoFactory quic_server_info_factory_;
   MockHostResolver host_resolver_;
   DeterministicMockClientSocketFactory socket_factory_;
@@ -652,17 +667,7 @@ TEST_P(QuicStreamFactoryTest, HttpsPooling) {
   HostPortPair server1("www.example.org", 443);
   HostPortPair server2("mail.example.org", 443);
 
-  // Load a cert that is valid for:
-  //   www.example.org (server1)
-  //   mail.example.org (server2)
-  //   www.example.com
-  base::FilePath certs_dir = GetTestCertsDirectory();
-  scoped_refptr<X509Certificate> test_cert(
-      ImportCertFromFile(certs_dir, "spdy_pooling.pem"));
-  ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
-  ProofVerifyDetailsChromium verify_details;
-  verify_details.cert_verify_result.verified_cert = test_cert;
-  verify_details.cert_verify_result.is_issued_by_known_root = true;
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
 
   host_resolver_.set_synchronous_mode(true);
@@ -708,17 +713,7 @@ TEST_P(QuicStreamFactoryTest, NoHttpsPoolingIfDisabled) {
   HostPortPair server1("www.example.org", 443);
   HostPortPair server2("mail.example.org", 443);
 
-  // Load a cert that is valid for:
-  //   www.example.org (server1)
-  //   mail.example.org (server2)
-  //   www.example.com
-  base::FilePath certs_dir = GetTestCertsDirectory();
-  scoped_refptr<X509Certificate> test_cert(
-      ImportCertFromFile(certs_dir, "spdy_pooling.pem"));
-  ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
-  ProofVerifyDetailsChromium verify_details;
-  verify_details.cert_verify_result.verified_cert = test_cert;
-  verify_details.cert_verify_result.is_issued_by_known_root = true;
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
 
   host_resolver_.set_synchronous_mode(true);
@@ -769,17 +764,7 @@ TEST_P(QuicStreamFactoryTest, NoHttpsPoolingWithCertMismatch) {
   HostPortPair server1("www.example.org", 443);
   HostPortPair server2("mail.google.com", 443);
 
-  // Load a cert that is valid for:
-  //   www.example.org (server1)
-  //   mail.example.org
-  //   www.example.com
-  // But is not valid for mail.google.com (server2).
-  base::FilePath certs_dir = GetTestCertsDirectory();
-  scoped_refptr<X509Certificate> test_cert(
-      ImportCertFromFile(certs_dir, "spdy_pooling.pem"));
-  ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
-  ProofVerifyDetailsChromium verify_details;
-  verify_details.cert_verify_result.verified_cert = test_cert;
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
 
   host_resolver_.set_synchronous_mode(true);
@@ -828,16 +813,7 @@ TEST_P(QuicStreamFactoryTest, HttpsPoolingWithMatchingPins) {
   test::AddPin(&transport_security_state_, "mail.example.org", primary_pin,
                backup_pin);
 
-  // Load a cert that is valid for:
-  //   www.example.org (server1)
-  //   mail.example.org (server2)
-  base::FilePath certs_dir = GetTestCertsDirectory();
-  scoped_refptr<X509Certificate> test_cert(
-      ImportCertFromFile(certs_dir, "spdy_pooling.pem"));
-  ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
-  ProofVerifyDetailsChromium verify_details;
-  verify_details.cert_verify_result.verified_cert = test_cert;
-  verify_details.cert_verify_result.is_issued_by_known_root = true;
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   verify_details.cert_verify_result.public_key_hashes.push_back(
       test::GetTestHashValue(primary_pin));
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
@@ -889,16 +865,7 @@ TEST_P(QuicStreamFactoryTest, NoHttpsPoolingWithMatchingPinsIfDisabled) {
   test::AddPin(&transport_security_state_, "mail.example.org", primary_pin,
                backup_pin);
 
-  // Load a cert that is valid for:
-  //   www.example.org (server1)
-  //   mail.example.org (server2)
-  base::FilePath certs_dir = GetTestCertsDirectory();
-  scoped_refptr<X509Certificate> test_cert(
-      ImportCertFromFile(certs_dir, "spdy_pooling.pem"));
-  ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
-  ProofVerifyDetailsChromium verify_details;
-  verify_details.cert_verify_result.verified_cert = test_cert;
-  verify_details.cert_verify_result.is_issued_by_known_root = true;
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   verify_details.cert_verify_result.public_key_hashes.push_back(
       test::GetTestHashValue(primary_pin));
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
@@ -956,24 +923,12 @@ TEST_P(QuicStreamFactoryTest, NoHttpsPoolingWithDifferentPins) {
   test::AddPin(&transport_security_state_, "mail.example.org", primary_pin,
                backup_pin);
 
-  // Load a cert that is valid for:
-  //   www.example.org (server1)
-  //   mail.example.org (server2)
-  base::FilePath certs_dir = GetTestCertsDirectory();
-  scoped_refptr<X509Certificate> test_cert(
-      ImportCertFromFile(certs_dir, "spdy_pooling.pem"));
-  ASSERT_NE(static_cast<X509Certificate*>(nullptr), test_cert.get());
-
-  ProofVerifyDetailsChromium verify_details1;
-  verify_details1.cert_verify_result.verified_cert = test_cert;
-  verify_details1.cert_verify_result.is_issued_by_known_root = true;
+  ProofVerifyDetailsChromium verify_details1 = DefaultProofVerifyDetails();
   verify_details1.cert_verify_result.public_key_hashes.push_back(
       test::GetTestHashValue(bad_pin));
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details1);
 
-  ProofVerifyDetailsChromium verify_details2;
-  verify_details2.cert_verify_result.verified_cert = test_cert;
-  verify_details2.cert_verify_result.is_issued_by_known_root = true;
+  ProofVerifyDetailsChromium verify_details2 = DefaultProofVerifyDetails();
   verify_details2.cert_verify_result.public_key_hashes.push_back(
       test::GetTestHashValue(primary_pin));
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details2);