Fix PepperTCPSocketMessageFilter crashes.

When a PepperTCPSocketMessageFilter was torn down while it was waiting
on a callback that was passed to NetworkContext to be invoked, Chrome
would crash with a UAF once the callback was invoked. This CL fixes
that by using weak pointers, and fixes a similar issue with the
ServerSocket message filter as well.

Weak pointers are not used with callbacks that are passed to Mojo
pipes the classes own, as they won't be called on teardown, anyways.

This CL also adds a bunch of tests for tearing down the Pepper
MessageFilter while there's a live request in various states.

Bug: 893604
Change-Id: Ibc57ed116bf6b86df975d4dc4cc54fc5da16388d
Reviewed-on: https://chromium-review.googlesource.com/c/1272095
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#598323}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 561d8efe60a0481e0b7cb3fe220476ae7569a618
diff --git a/tests/test_tcp_server_socket_private.cc b/tests/test_tcp_server_socket_private.cc
index 7d4e7dd..1ad8994 100644
--- a/tests/test_tcp_server_socket_private.cc
+++ b/tests/test_tcp_server_socket_private.cc
@@ -18,6 +18,16 @@
 using pp::TCPServerSocketPrivate;
 using pp::TCPSocketPrivate;
 
+namespace {
+
+// Used by pp::CompletionCallbacks that want to delete a PP_Resource, passed as
+// |user_data|, on completion.
+void DeleteResource(void* user_data, int32_t flags) {
+  delete reinterpret_cast<PP_Resource*>(user_data);
+}
+
+}  // namespace
+
 REGISTER_TEST_CASE(TCPServerSocketPrivate);
 
 TestTCPServerSocketPrivate::TestTCPServerSocketPrivate(
@@ -58,7 +68,9 @@
   RUN_CALLBACK_TEST(TestTCPServerSocketPrivate, Backlog, filter);
 
   RUN_CALLBACK_TEST(TestTCPServerSocketPrivate, ListenFails, filter);
+  RUN_CALLBACK_TEST(TestTCPServerSocketPrivate, ListenHangs, filter);
   RUN_CALLBACK_TEST(TestTCPServerSocketPrivate, AcceptFails, filter);
+  RUN_CALLBACK_TEST(TestTCPServerSocketPrivate, AcceptHangs, filter);
 }
 
 std::string TestTCPServerSocketPrivate::GetLocalAddress(
@@ -304,6 +316,18 @@
   PASS();
 }
 
+std::string TestTCPServerSocketPrivate::TestListenHangs() {
+  TCPServerSocketPrivate socket(instance_);
+
+  uint8_t localhost_ip[4] = {127, 0, 0, 1};
+  PP_NetAddress_Private ipv4;
+  ASSERT_TRUE(
+      NetAddressPrivate::CreateFromIPv4Address(localhost_ip, 80, &ipv4));
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  socket.Listen(&ipv4, 2 /* backlog */, DoNothingCallback());
+  PASS();
+}
+
 std::string TestTCPServerSocketPrivate::TestAcceptFails() {
   TCPServerSocketPrivate socket(instance_);
   uint8_t localhost_ip[4] = {127, 0, 0, 1};
@@ -325,3 +349,22 @@
 
   PASS();
 }
+
+std::string TestTCPServerSocketPrivate::TestAcceptHangs() {
+  TCPServerSocketPrivate socket(instance_);
+  uint8_t localhost_ip[4] = {127, 0, 0, 1};
+  PP_NetAddress_Private ipv4;
+  ASSERT_TRUE(
+      NetAddressPrivate::CreateFromIPv4Address(localhost_ip, 80, &ipv4));
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  callback.WaitForResult(
+      socket.Listen(&ipv4, 2 /* backlog */, callback.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_OK, callback.result());
+
+  PP_Resource* resource = new PP_Resource();
+  socket.Accept(resource,
+                pp::CompletionCallback(&DeleteResource, resource,
+                                       PP_COMPLETIONCALLBACK_FLAG_OPTIONAL));
+  PASS();
+}
diff --git a/tests/test_tcp_server_socket_private.h b/tests/test_tcp_server_socket_private.h
index b371389..fcde64d 100644
--- a/tests/test_tcp_server_socket_private.h
+++ b/tests/test_tcp_server_socket_private.h
@@ -53,10 +53,14 @@
   std::string TestListen();
   std::string TestBacklog();
 
-  // The higher level test fixture must configure the corresponding events to
-  // fail with PP_ERROR_FAILED.
+  // The higher level test fixture is responsible for making socket methods
+  // behave in the expected manner.  The *Fails tests expect the specified event
+  // to fail with PP_ERROR_FAILED, and the *Hangs tests expect the specified
+  // operation to never complete, at least until teardown starts.
   std::string TestListenFails();
+  std::string TestListenHangs();
   std::string TestAcceptFails();
+  std::string TestAcceptHangs();
 
   std::string host_;
   uint16_t port_;
diff --git a/tests/test_tcp_socket.cc b/tests/test_tcp_socket.cc
index cd22767..8d30929 100644
--- a/tests/test_tcp_socket.cc
+++ b/tests/test_tcp_socket.cc
@@ -65,6 +65,7 @@
   RUN_CALLBACK_TEST(TestTCPSocket, UnexpectedCalls, filter);
 
   RUN_CALLBACK_TEST(TestTCPSocket, ConnectFails, filter);
+  RUN_CALLBACK_TEST(TestTCPSocket, ConnectHangs, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, WriteFails, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, ReadFails, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, SetSendBufferSizeFails, filter);
@@ -72,11 +73,15 @@
   RUN_CALLBACK_TEST(TestTCPSocket, SetNoDelayFails, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, BindFailsConnectSucceeds, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, BindFails, filter);
+  RUN_CALLBACK_TEST(TestTCPSocket, BindHangs, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, ListenFails, filter);
+  RUN_CALLBACK_TEST(TestTCPSocket, ListenHangs, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, AcceptFails, filter);
+  RUN_CALLBACK_TEST(TestTCPSocket, AcceptHangs, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, AcceptedSocketWriteFails, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, AcceptedSocketReadFails, filter);
   RUN_CALLBACK_TEST(TestTCPSocket, BindConnectFails, filter);
+  RUN_CALLBACK_TEST(TestTCPSocket, BindConnectHangs, filter);
 }
 
 std::string TestTCPSocket::TestConnect() {
@@ -495,6 +500,13 @@
   PASS();
 }
 
+std::string TestTCPSocket::TestConnectHangs() {
+  pp::TCPSocket socket(instance_);
+  TestCompletionCallback cb(instance_->pp_instance(), callback_type());
+  socket.Connect(test_server_addr_, DoNothingCallback());
+  PASS();
+}
+
 std::string TestTCPSocket::TestWriteFails() {
   pp::TCPSocket socket(instance_);
   TestCompletionCallback cb(instance_->pp_instance(), callback_type());
@@ -623,6 +635,12 @@
   PASS();
 }
 
+std::string TestTCPSocket::TestBindHangs() {
+  pp::TCPSocket socket(instance_);
+  socket.Bind(test_server_addr_, DoNothingCallback());
+  PASS();
+}
+
 std::string TestTCPSocket::TestListenFails() {
   pp::TCPSocket socket(instance_);
   TestCompletionCallback callback(instance_->pp_instance(), callback_type());
@@ -643,6 +661,19 @@
   PASS();
 }
 
+std::string TestTCPSocket::TestListenHangs() {
+  pp::TCPSocket socket(instance_);
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  // The address doesn't matter here, other than that it should be valid.
+  callback.WaitForResult(
+      socket.Bind(test_server_addr_, callback.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_OK, callback.result());
+
+  socket.Listen(2 /* backlog */, DoNothingCallback());
+  PASS();
+}
+
 std::string TestTCPSocket::TestAcceptFails() {
   pp::TCPSocket socket(instance_);
   TestCompletionCallback callback(instance_->pp_instance(), callback_type());
@@ -666,6 +697,24 @@
   PASS();
 }
 
+std::string TestTCPSocket::TestAcceptHangs() {
+  pp::TCPSocket socket(instance_);
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  // The address doesn't matter here, other than that it should be valid.
+  callback.WaitForResult(
+      socket.Bind(test_server_addr_, callback.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_OK, callback.result());
+
+  callback.WaitForResult(
+      socket.Listen(2 /* backlog */, callback.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_OK, callback.result());
+
+  socket.Accept(DoNothingCallbackWithOutput<pp::TCPSocket>());
+  PASS();
+}
+
 std::string TestTCPSocket::TestAcceptedSocketWriteFails() {
   pp::TCPSocket socket(instance_);
   TestCompletionCallback callback(instance_->pp_instance(), callback_type());
@@ -770,6 +819,18 @@
   PASS();
 }
 
+std::string TestTCPSocket::TestBindConnectHangs() {
+  pp::TCPSocket socket(instance_);
+  TestCompletionCallback cb(instance_->pp_instance(), callback_type());
+
+  cb.WaitForResult(socket.Bind(test_server_addr_, cb.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(cb);
+  ASSERT_EQ(PP_OK, cb.result());
+
+  socket.Connect(test_server_addr_, DoNothingCallback());
+  PASS();
+}
+
 std::string TestTCPSocket::ReadFirstLineFromSocket(pp::TCPSocket* socket,
                                                    std::string* s) {
   char buffer[1000];
diff --git a/tests/test_tcp_socket.h b/tests/test_tcp_socket.h
index 0a0e206..265f021 100644
--- a/tests/test_tcp_socket.h
+++ b/tests/test_tcp_socket.h
@@ -35,10 +35,12 @@
   std::string TestInterface_1_0();
   std::string TestUnexpectedCalls();
 
-  // The higher level test fixture is responsible for making the appropriate
-  // call in these tests fail with PP_ERROR_FAILED, and all prior events
-  // succeed.
+  // The higher level test fixture is responsible for making socket methods
+  // behave in the expected manner.  The *Fails tests expect the specified even
+  // to fail with PP_ERROR_FAILED, and the *Hangs test expect the specified
+  // operation to never complete, at least until teardown starts.
   std::string TestConnectFails();
+  std::string TestConnectHangs();
   std::string TestWriteFails();
   std::string TestReadFails();
   std::string TestSetSendBufferSizeFails();
@@ -49,11 +51,15 @@
   // This is needed in addition to the above test in the case where a bind
   // failure is simulated in a way that also closes the NetworkContext pipe.
   std::string TestBindFails();
+  std::string TestBindHangs();
   std::string TestListenFails();
+  std::string TestListenHangs();
   std::string TestAcceptFails();
+  std::string TestAcceptHangs();
   std::string TestAcceptedSocketWriteFails();
   std::string TestAcceptedSocketReadFails();
   std::string TestBindConnectFails();
+  std::string TestBindConnectHangs();
 
   std::string ReadFirstLineFromSocket(pp::TCPSocket* socket, std::string* s);
   std::string ReadFirstLineFromSocket_1_0(PP_Resource socket,
diff --git a/tests/test_tcp_socket_private.cc b/tests/test_tcp_socket_private.cc
index 5037d0b..8f9e29a 100644
--- a/tests/test_tcp_socket_private.cc
+++ b/tests/test_tcp_socket_private.cc
@@ -58,6 +58,7 @@
   RUN_CALLBACK_TEST(TestTCPSocketPrivate, LargeRead, filter);
 
   RUN_CALLBACK_TEST(TestTCPSocketPrivate, SSLHandshakeFails, filter);
+  RUN_CALLBACK_TEST(TestTCPSocketPrivate, SSLHandshakeHangs, filter);
   RUN_CALLBACK_TEST(TestTCPSocketPrivate, SSLWriteFails, filter);
   RUN_CALLBACK_TEST(TestTCPSocketPrivate, SSLReadFails, filter);
 }
@@ -244,6 +245,18 @@
   PASS();
 }
 
+std::string TestTCPSocketPrivate::TestSSLHandshakeHangs() {
+  pp::TCPSocketPrivate socket(instance_);
+  TestCompletionCallback cb(instance_->pp_instance(), callback_type());
+
+  cb.WaitForResult(socket.Connect("foo.test", 443, cb.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(cb);
+  ASSERT_EQ(PP_OK, cb.result());
+
+  socket.SSLHandshake("foo.test", 443, DoNothingCallback());
+  PASS();
+}
+
 std::string TestTCPSocketPrivate::TestSSLWriteFails() {
   pp::TCPSocketPrivate socket(instance_);
   TestCompletionCallback cb(instance_->pp_instance(), callback_type());
diff --git a/tests/test_tcp_socket_private.h b/tests/test_tcp_socket_private.h
index 13fa1cf..f76c622 100644
--- a/tests/test_tcp_socket_private.h
+++ b/tests/test_tcp_socket_private.h
@@ -30,10 +30,12 @@
   std::string TestSetOption();
   std::string TestLargeRead();
 
-  // The higher level test fixture is responsible for making the appropriate
-  // call in these tests fail with PP_ERROR_FAILED, and all prior events
-  // succeed.
+  // The higher level test fixture is responsible for making socket methods
+  // behave in the expected manner.  The *Fails tests expect the specified event
+  // to fail with PP_ERROR_FAILED, and the *Hangs test expects the specified
+  // operation to never complete, at least until teardown starts.
   std::string TestSSLHandshakeFails();
+  std::string TestSSLHandshakeHangs();
   std::string TestSSLWriteFails();
   std::string TestSSLReadFails();
 
diff --git a/tests/test_utils.cc b/tests/test_utils.cc
index e091e68..af5de51 100644
--- a/tests/test_utils.cc
+++ b/tests/test_utils.cc
@@ -32,6 +32,8 @@
   return data.integer8[0] == 1;
 }
 
+void DoNothing(void* user_data, int32_t result) {}
+
 }  // namespace
 
 const int kActionTimeoutMs = 10000;
@@ -272,6 +274,11 @@
   static_cast<NestedEvent*>(event)->SignalOnMainThread();
 }
 
+pp::CompletionCallback DoNothingCallback() {
+  return pp::CompletionCallback(&DoNothing, NULL,
+                                PP_COMPLETIONCALLBACK_FLAG_OPTIONAL);
+}
+
 TestCompletionCallback::TestCompletionCallback(PP_Instance instance)
     : wait_for_result_called_(false),
       have_result_(false),
diff --git a/tests/test_utils.h b/tests/test_utils.h
index d2e641a..1611b99 100644
--- a/tests/test_utils.h
+++ b/tests/test_utils.h
@@ -95,6 +95,30 @@
   NestedEvent& operator=(const NestedEvent&);
 };
 
+// Returns a callback that does nothing, so can be invoked when the current
+// function is out of scope, unlike TestCompletionCallback.
+pp::CompletionCallback DoNothingCallback();
+
+template <typename OutputT>
+void DeleteStorage(void* user_data, int32_t flags) {
+  typename pp::CompletionCallbackWithOutput<OutputT>::OutputStorageType*
+      storage = reinterpret_cast<typename pp::CompletionCallbackWithOutput<
+          OutputT>::OutputStorageType*>(user_data);
+  delete storage;
+}
+
+// Same as DoNothingCallback(), but with an OutputStorageType, which it deletes
+// when the callback is invoked.
+template <typename OutputT>
+pp::CompletionCallbackWithOutput<OutputT> DoNothingCallbackWithOutput() {
+  typename pp::CompletionCallbackWithOutput<OutputT>::OutputStorageType*
+      storage = new
+      typename pp::CompletionCallbackWithOutput<OutputT>::OutputStorageType();
+  return pp::CompletionCallbackWithOutput<OutputT>(
+      &DeleteStorage<OutputT>, storage, PP_COMPLETIONCALLBACK_FLAG_OPTIONAL,
+      storage);
+}
+
 enum CallbackType { PP_REQUIRED, PP_OPTIONAL, PP_BLOCKING };
 class TestCompletionCallback {
  public: