Combine ChannelIDService::RequestHandle and ChannelIDServiceRequest classes

net/ssl/channel_id_service_unittest.cc

Index: net/ssl/channel_id_service_unittest.cc
diff --git a/net/ssl/channel_id_service_unittest.cc b/net/ssl/channel_id_service_unittest.cc
index 4ecef89160f376e655056458655959b6f338bbc2..36b4cf6668d360f3ff39f7ec163823a5f5f271ea 100644
--- a/net/ssl/channel_id_service_unittest.cc
+++ b/net/ssl/channel_id_service_unittest.cc
@@ -137,15 +137,14 @@ TEST_F(ChannelIDServiceTest, GetCacheMiss) {
 
   int error;
   TestCompletionCallback callback;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   // Synchronous completion, because the store is initialized.
   scoped_ptr<crypto::ECPrivateKey> key;
   EXPECT_EQ(0, service_->channel_id_count());
-  error =
-      service_->GetChannelID(host, &key, callback.callback(), &request_handle);
+  error = service_->GetChannelID(host, &key, callback.callback(), &request);
   EXPECT_EQ(ERR_FILE_NOT_FOUND, error);
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_FALSE(request.is_active());
   EXPECT_EQ(0, service_->channel_id_count());
   EXPECT_FALSE(key);
 }
@@ -155,35 +154,34 @@ TEST_F(ChannelIDServiceTest, CacheHit) {
 
   int error;
   TestCompletionCallback callback;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   // Asynchronous completion.
   scoped_ptr<crypto::ECPrivateKey> key1;
   EXPECT_EQ(0, service_->channel_id_count());
   error = service_->GetOrCreateChannelID(host, &key1, callback.callback(),
-                                         &request_handle);
+                                         &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->channel_id_count());
   EXPECT_TRUE(key1);
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_FALSE(request.is_active());
 
   // Synchronous completion.
   scoped_ptr<crypto::ECPrivateKey> key2;
   error = service_->GetOrCreateChannelID(host, &key2, callback.callback(),
-                                         &request_handle);
-  EXPECT_FALSE(request_handle.is_active());
+                                         &request);
+  EXPECT_FALSE(request.is_active());
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->channel_id_count());
   EXPECT_TRUE(KeysEqual(key1.get(), key2.get()));
 
   // Synchronous get.
   scoped_ptr<crypto::ECPrivateKey> key3;
-  error =
-      service_->GetChannelID(host, &key3, callback.callback(), &request_handle);
-  EXPECT_FALSE(request_handle.is_active());
+  error = service_->GetChannelID(host, &key3, callback.callback(), &request);
+  EXPECT_FALSE(request.is_active());
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->channel_id_count());
   EXPECT_TRUE(KeysEqual(key1.get(), key3.get()));
@@ -196,15 +194,15 @@ TEST_F(ChannelIDServiceTest, CacheHit) {
 TEST_F(ChannelIDServiceTest, StoreChannelIDs) {
   int error;
   TestCompletionCallback callback;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   std::string host1("encrypted.google.com");
   scoped_ptr<crypto::ECPrivateKey> key1;
   EXPECT_EQ(0, service_->channel_id_count());
   error = service_->GetOrCreateChannelID(host1, &key1, callback.callback(),
-                                         &request_handle);
+                                         &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->channel_id_count());
@@ -212,9 +210,9 @@ TEST_F(ChannelIDServiceTest, StoreChannelIDs) {
   std::string host2("www.verisign.com");
   scoped_ptr<crypto::ECPrivateKey> key2;
   error = service_->GetOrCreateChannelID(host2, &key2, callback.callback(),
-                                         &request_handle);
+                                         &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(2, service_->channel_id_count());
@@ -222,9 +220,9 @@ TEST_F(ChannelIDServiceTest, StoreChannelIDs) {
   std::string host3("www.twitter.com");
   scoped_ptr<crypto::ECPrivateKey> key3;
   error = service_->GetOrCreateChannelID(host3, &key3, callback.callback(),
-                                         &request_handle);
+                                         &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(3, service_->channel_id_count());
@@ -241,21 +239,21 @@ TEST_F(ChannelIDServiceTest, InflightJoin) {
 
   scoped_ptr<crypto::ECPrivateKey> key1;
   TestCompletionCallback callback1;
-  ChannelIDService::RequestHandle request_handle1;
+  ChannelIDService::Request request1;
 
   scoped_ptr<crypto::ECPrivateKey> key2;
   TestCompletionCallback callback2;
-  ChannelIDService::RequestHandle request_handle2;
+  ChannelIDService::Request request2;
 
   error = service_->GetOrCreateChannelID(host, &key1, callback1.callback(),
-                                         &request_handle1);
+                                         &request1);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle1.is_active());
+  EXPECT_TRUE(request1.is_active());
   // Should join with the original request.
   error = service_->GetOrCreateChannelID(host, &key2, callback2.callback(),
-                                         &request_handle2);
+                                         &request2);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle2.is_active());
+  EXPECT_TRUE(request2.is_active());
 
   error = callback1.WaitForResult();
   EXPECT_EQ(OK, error);
@@ -275,21 +273,20 @@ TEST_F(ChannelIDServiceTest, InflightJoinGetOrCreateAndGet) {
 
   scoped_ptr<crypto::ECPrivateKey> key1;
   TestCompletionCallback callback1;
-  ChannelIDService::RequestHandle request_handle1;
+  ChannelIDService::Request request1;
 
   scoped_ptr<crypto::ECPrivateKey> key2;
   TestCompletionCallback callback2;
-  ChannelIDService::RequestHandle request_handle2;
+  ChannelIDService::Request request2;
 
   error = service_->GetOrCreateChannelID(host, &key1, callback1.callback(),
-                                         &request_handle1);
+                                         &request1);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle1.is_active());
+  EXPECT_TRUE(request1.is_active());
   // Should join with the original request.
-  error = service_->GetChannelID(host, &key2, callback2.callback(),
-                                 &request_handle2);
+  error = service_->GetChannelID(host, &key2, callback2.callback(), &request2);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle2.is_active());
+  EXPECT_TRUE(request2.is_active());
 
   error = callback1.WaitForResult();
   EXPECT_EQ(OK, error);
@@ -308,14 +305,14 @@ TEST_F(ChannelIDServiceTest, CancelRequest) {
   std::string host("encrypted.google.com");
   scoped_ptr<crypto::ECPrivateKey> key;
   int error;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   error = service_->GetOrCreateChannelID(host, &key, base::Bind(&FailTest),
-                                         &request_handle);
+                                         &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
-  request_handle.Cancel();
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
+  request.Cancel();
+  EXPECT_FALSE(request.is_active());
 
   // Wait for reply from ChannelIDServiceWorker to be posted back to the
   // ChannelIDService.
@@ -326,18 +323,18 @@ TEST_F(ChannelIDServiceTest, CancelRequest) {
   EXPECT_EQ(1, service_->channel_id_count());
 }
 
-// Tests that destructing the RequestHandle cancels the request.
+// Tests that destructing the Request cancels the request.
 TEST_F(ChannelIDServiceTest, CancelRequestByHandleDestruction) {
   std::string host("encrypted.google.com");
   scoped_ptr<crypto::ECPrivateKey> key;
   int error;
   {
-    ChannelIDService::RequestHandle request_handle;
+    ChannelIDService::Request request;
 
     error = service_->GetOrCreateChannelID(host, &key, base::Bind(&FailTest),
-                                           &request_handle);
+                                           &request);
     EXPECT_EQ(ERR_IO_PENDING, error);
-    EXPECT_TRUE(request_handle.is_active());
+    EXPECT_TRUE(request.is_active());
   }
 
   // Wait for reply from ChannelIDServiceWorker to be posted back to the
@@ -353,15 +350,15 @@ TEST_F(ChannelIDServiceTest, DestructionWithPendingRequest) {
   std::string host("encrypted.google.com");
   scoped_ptr<crypto::ECPrivateKey> key;
   int error;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   error = service_->GetOrCreateChannelID(host, &key, base::Bind(&FailTest),
-                                         &request_handle);
+                                         &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
 
   // Cancel request and destroy the ChannelIDService.
-  request_handle.Cancel();
+  request.Cancel();
   service_.reset();
 
   // ChannelIDServiceWorker should not post anything back to the
@@ -372,6 +369,30 @@ TEST_F(ChannelIDServiceTest, DestructionWithPendingRequest) {
   // If we got here without crashing or a valgrind error, it worked.
 }
 
+TEST_F(ChannelIDServiceTest, DestructionOfChannelIDServiceRequest) {

[mattm, 2015/06/09 23:08:35]

Is this test different than
ChannelIDServiceTest.CancelRequestByHandleDestruction?

[nharper, 2015/06/09 23:45:03]

It looks like these tests should do the same thing, but came from different
intentions. That test (CancelRequestByHandleDestruction) appears to be testing
that once the Request is Cancel()ed, its callback isn't called, whereas this one
tests that a Request object isn't read from/written to after it is destroyed.
That test also doesn't fail the way this test does, because its Request object
is stack allocated, so reading/writing to it doesn't cause a crash or asan/lsan
failure like in this test where the Request is on the heap.

Even though there are these differences, they're so similar that they can be
combined by changing the stack-allocated Request in that test to be heap
allocated, and getting rid of this test.

[mattm, 2015/06/10 00:14:24]

Oh, that's a good point. I wasn't thinking about that difference between stack
and heap allocated objects.
sgtm

+  std::string host("encrypted.google.com");
+  scoped_ptr<crypto::ECPrivateKey> key;
+  int error;
+  TestCompletionCallback callback;
+  scoped_ptr<ChannelIDService::Request> request(
+      new ChannelIDService::Request());
+
+  error = service_->GetOrCreateChannelID(host, &key, callback.callback(),
+                                         request.get());
+  EXPECT_EQ(ERR_IO_PENDING, error);
+  EXPECT_TRUE(request->is_active());
+
+  // Delete the Request object, which should also cancel the request.
+  request.reset();
+
+  // Run the loop. If deleting the Request object didn't cancel the request,
+  // then running the request until completion will cause the test to crash
+  // (because it will try to update the now deleted Request object).
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // If we got here without crashing or a valgrind error, it worked.
+}
+
 // Tests that shutting down the sequenced worker pool and then making new
 // requests gracefully fails.
 // This is a regression test for http://crbug.com/236387
@@ -384,13 +405,13 @@ TEST_F(ChannelIDServiceTest, RequestAfterPoolShutdown) {
   std::string host("encrypted.google.com");
   scoped_ptr<crypto::ECPrivateKey> key;
   int error;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   error = service_->GetOrCreateChannelID(host, &key, base::Bind(&FailTest),
-                                         &request_handle);
+                                         &request);
   // If we got here without crashing or a valgrind error, it worked.
   ASSERT_EQ(ERR_INSUFFICIENT_RESOURCES, error);
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_FALSE(request.is_active());
 }
 
 // Tests that simultaneous creation of different certs works.
@@ -400,32 +421,32 @@ TEST_F(ChannelIDServiceTest, SimultaneousCreation) {
   std::string host1("encrypted.google.com");
   scoped_ptr<crypto::ECPrivateKey> key1;
   TestCompletionCallback callback1;
-  ChannelIDService::RequestHandle request_handle1;
+  ChannelIDService::Request request1;
 
   std::string host2("foo.com");
   scoped_ptr<crypto::ECPrivateKey> key2;
   TestCompletionCallback callback2;
-  ChannelIDService::RequestHandle request_handle2;
+  ChannelIDService::Request request2;
 
   std::string host3("bar.com");
   scoped_ptr<crypto::ECPrivateKey> key3;
   TestCompletionCallback callback3;
-  ChannelIDService::RequestHandle request_handle3;
+  ChannelIDService::Request request3;
 
   error = service_->GetOrCreateChannelID(host1, &key1, callback1.callback(),
-                                         &request_handle1);
+                                         &request1);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle1.is_active());
+  EXPECT_TRUE(request1.is_active());
 
   error = service_->GetOrCreateChannelID(host2, &key2, callback2.callback(),
-                                         &request_handle2);
+                                         &request2);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle2.is_active());
+  EXPECT_TRUE(request2.is_active());
 
   error = service_->GetOrCreateChannelID(host3, &key3, callback3.callback(),
-                                         &request_handle3);
+                                         &request3);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle3.is_active());
+  EXPECT_TRUE(request3.is_active());
 
   error = callback1.WaitForResult();
   EXPECT_EQ(OK, error);
@@ -456,15 +477,15 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetOrCreateNoChannelIDsInStore) {
 
   int error;
   TestCompletionCallback callback;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   // Asynchronous completion with no certs in the store.
   scoped_ptr<crypto::ECPrivateKey> key;
   EXPECT_EQ(0, service_->channel_id_count());
-  error = service_->GetOrCreateChannelID(host, &key, callback.callback(),
-                                         &request_handle);
+  error =
+      service_->GetOrCreateChannelID(host, &key, callback.callback(), &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
 
   mock_store->CallGetChannelIDCallbackWithResult(ERR_FILE_NOT_FOUND, nullptr);
 
@@ -472,7 +493,7 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetOrCreateNoChannelIDsInStore) {
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->channel_id_count());
   EXPECT_TRUE(key);
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_FALSE(request.is_active());
 }
 
 TEST_F(ChannelIDServiceTest, AsyncStoreGetNoChannelIDsInStore) {
@@ -485,15 +506,14 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetNoChannelIDsInStore) {
 
   int error;
   TestCompletionCallback callback;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   // Asynchronous completion with no certs in the store.
   scoped_ptr<crypto::ECPrivateKey> key;
   EXPECT_EQ(0, service_->channel_id_count());
-  error =
-      service_->GetChannelID(host, &key, callback.callback(), &request_handle);
+  error = service_->GetChannelID(host, &key, callback.callback(), &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
 
   mock_store->CallGetChannelIDCallbackWithResult(ERR_FILE_NOT_FOUND, nullptr);
 
@@ -502,7 +522,7 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetNoChannelIDsInStore) {
   EXPECT_EQ(0, service_->channel_id_count());
   EXPECT_EQ(0u, service_->workers_created());
   EXPECT_FALSE(key);
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_FALSE(request.is_active());
 }
 
 TEST_F(ChannelIDServiceTest, AsyncStoreGetOrCreateOneCertInStore) {
@@ -515,15 +535,15 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetOrCreateOneCertInStore) {
 
   int error;
   TestCompletionCallback callback;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   // Asynchronous completion with a cert in the store.
   scoped_ptr<crypto::ECPrivateKey> key;
   EXPECT_EQ(0, service_->channel_id_count());
-  error = service_->GetOrCreateChannelID(host, &key, callback.callback(),
-                                         &request_handle);
+  error =
+      service_->GetOrCreateChannelID(host, &key, callback.callback(), &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
 
   scoped_ptr<crypto::ECPrivateKey> expected_key(crypto::ECPrivateKey::Create());
   mock_store->CallGetChannelIDCallbackWithResult(OK, expected_key.get());
@@ -538,7 +558,7 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetOrCreateOneCertInStore) {
   EXPECT_EQ(0u, service_->workers_created());
   EXPECT_TRUE(key);
   EXPECT_TRUE(KeysEqual(expected_key.get(), key.get()));
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_FALSE(request.is_active());
 }
 
 TEST_F(ChannelIDServiceTest, AsyncStoreGetOneCertInStore) {
@@ -551,16 +571,15 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetOneCertInStore) {
 
   int error;
   TestCompletionCallback callback;
-  ChannelIDService::RequestHandle request_handle;
+  ChannelIDService::Request request;
 
   // Asynchronous completion with a cert in the store.
   scoped_ptr<crypto::ECPrivateKey> key;
   std::string private_key, spki;
   EXPECT_EQ(0, service_->channel_id_count());
-  error =
-      service_->GetChannelID(host, &key, callback.callback(), &request_handle);
+  error = service_->GetChannelID(host, &key, callback.callback(), &request);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
+  EXPECT_TRUE(request.is_active());
 
   scoped_ptr<crypto::ECPrivateKey> expected_key(crypto::ECPrivateKey::Create());
   mock_store->CallGetChannelIDCallbackWithResult(OK, expected_key.get());
@@ -574,7 +593,7 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetOneCertInStore) {
   // created.
   EXPECT_EQ(0u, service_->workers_created());
   EXPECT_TRUE(KeysEqual(expected_key.get(), key.get()));
-  EXPECT_FALSE(request_handle.is_active());
+  EXPECT_FALSE(request.is_active());
 }
 
 TEST_F(ChannelIDServiceTest, AsyncStoreGetThenCreateNoCertsInStore) {
@@ -589,23 +608,22 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetThenCreateNoCertsInStore) {
 
   // Asynchronous get with no certs in the store.
   TestCompletionCallback callback1;
-  ChannelIDService::RequestHandle request_handle1;
+  ChannelIDService::Request request1;
   scoped_ptr<crypto::ECPrivateKey> key1;
   EXPECT_EQ(0, service_->channel_id_count());
-  error = service_->GetChannelID(host, &key1, callback1.callback(),
-                                 &request_handle1);
+  error = service_->GetChannelID(host, &key1, callback1.callback(), &request1);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle1.is_active());
+  EXPECT_TRUE(request1.is_active());
 
   // Asynchronous get/create with no certs in the store.
   TestCompletionCallback callback2;
-  ChannelIDService::RequestHandle request_handle2;
+  ChannelIDService::Request request2;
   scoped_ptr<crypto::ECPrivateKey> key2;
   EXPECT_EQ(0, service_->channel_id_count());
   error = service_->GetOrCreateChannelID(host, &key2, callback2.callback(),
-                                         &request_handle2);
+                                         &request2);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle2.is_active());
+  EXPECT_TRUE(request2.is_active());
 
   mock_store->CallGetChannelIDCallbackWithResult(ERR_FILE_NOT_FOUND, nullptr);
 
@@ -623,8 +641,8 @@ TEST_F(ChannelIDServiceTest, AsyncStoreGetThenCreateNoCertsInStore) {
   EXPECT_EQ(1u, service_->inflight_joins());
   EXPECT_TRUE(key1);
   EXPECT_TRUE(KeysEqual(key1.get(), key2.get()));
-  EXPECT_FALSE(request_handle1.is_active());
-  EXPECT_FALSE(request_handle2.is_active());
+  EXPECT_FALSE(request1.is_active());
+  EXPECT_FALSE(request2.is_active());
 }
 
 }  // namespace