Fixing Intermittent SecurityKey Unittest Failures

remoting/host/security_key/security_key_ipc_server_unittest.cc

Index: remoting/host/security_key/security_key_ipc_server_unittest.cc
diff --git a/remoting/host/security_key/security_key_ipc_server_unittest.cc b/remoting/host/security_key/security_key_ipc_server_unittest.cc
index 075b27edf98e821e3b5f37df5e162786eae1492b..810a388d6bb5d306b3a5b11d9101e111d1cc5aa9 100644
--- a/remoting/host/security_key/security_key_ipc_server_unittest.cc
+++ b/remoting/host/security_key/security_key_ipc_server_unittest.cc
@@ -16,7 +16,6 @@
 #include "ipc/ipc_channel.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/named_platform_handle_utils.h"
-#include "mojo/edk/embedder/scoped_ipc_support.h"
 #include "remoting/host/client_session_details.h"
 #include "remoting/host/security_key/fake_security_key_ipc_client.h"
 #include "remoting/host/security_key/security_key_ipc_constants.h"
@@ -52,6 +51,9 @@ class SecurityKeyIpcServerTest : public testing::Test,
   // Waits until the current |run_loop_| instance is signaled, then resets it.
   void WaitForOperationComplete();
 
+  // Waits until all tasks have been run on the current message loop.
+  void WaitForPendingOperations();
+
   // ClientSessionControl overrides:
   ClientSessionControl* session_control() override { return nullptr; }
   uint32_t desktop_session_id() const override { return peer_session_id_; }
@@ -59,8 +61,6 @@ class SecurityKeyIpcServerTest : public testing::Test,
   // IPC tests require a valid MessageLoop to run.
   base::MessageLoopForIO message_loop_;
 
-  mojo::edk::ScopedIPCSupport ipc_support_;
-
   // Used to allow |message_loop_| to run during tests.  The instance is reset
   // after each stage of the tests has been completed.
   std::unique_ptr<base::RunLoop> run_loop_;
@@ -82,9 +82,7 @@ class SecurityKeyIpcServerTest : public testing::Test,
 };
 
 SecurityKeyIpcServerTest::SecurityKeyIpcServerTest()
-    : ipc_support_(message_loop_.task_runner(),
-                   mojo::edk::ScopedIPCSupport::ShutdownPolicy::FAST),
-      run_loop_(new base::RunLoop()) {
+    : run_loop_(new base::RunLoop()) {
 #if defined(OS_WIN)
   EXPECT_TRUE(ProcessIdToSessionId(
       GetCurrentProcessId(), reinterpret_cast<DWORD*>(&peer_session_id_)));
@@ -111,6 +109,17 @@ void SecurityKeyIpcServerTest::WaitForOperationComplete() {
   run_loop_.reset(new base::RunLoop());
 }
 
+void SecurityKeyIpcServerTest::WaitForPendingOperations() {

[Sergey Ulanov, 2017/02/02 01:46:40]

Maybe call this RunPendingTasks()? This function doesn't really wait for
anything. It just runs the tasks in the queue.

[joedow, 2017/02/03 22:32:46]

Done.

+  // Ensure we have a new, runnable instance.
+  run_loop_.reset(new base::RunLoop());

[Sergey Ulanov, 2017/02/02 01:46:40]

Each RunLoop is usable only once, i.e. Run[UntilIdle]() cannot be called more
than once (see
https://codesearch.chromium.org/chromium/src/base/run_loop.cc?l=71). But this
function creates RunLoop twice, but uses it only once, so I'm not sure you want
to reset RunLoop here, given that it's replaced after each Run() call in
WaitForOperationComplete()

Also, since this function doesn't depend on OperationComplete(), can you just
call RunLoop().RunUntilIdle(), without replacing run_loop_?

[joedow, 2017/02/03 22:32:46]

Excellent suggestion, thanks!

+
+  // Run until there are no pending work items in the queue.
+  run_loop_->RunUntilIdle();
+
+  // Create a new instance for future operations.
+  run_loop_.reset(new base::RunLoop());
+}
+
 void SecurityKeyIpcServerTest::SendRequestToClient(int connection_id,
                                                    const std::string& data) {
   last_connection_id_received_ = connection_id;
@@ -135,9 +144,9 @@ TEST_F(SecurityKeyIpcServerTest, HandleSingleSecurityKeyRequest) {
   ASSERT_TRUE(fake_ipc_client.ConnectViaIpc(channel_handle));
   WaitForOperationComplete();
 
+  ASSERT_TRUE(fake_ipc_client.ipc_channel_connected());
   ASSERT_FALSE(fake_ipc_client.invalid_session_error());
   ASSERT_TRUE(fake_ipc_client.connection_ready());
-  ASSERT_TRUE(fake_ipc_client.ipc_channel_connected());
 
   // Send a request from the IPC client to the IPC server.
   std::string request_data("Blergh!");
@@ -158,6 +167,7 @@ TEST_F(SecurityKeyIpcServerTest, HandleSingleSecurityKeyRequest) {
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();

[Sergey Ulanov, 2017/02/02 01:46:40]

Since this needs to be done after every test maybe put it in TearDown()?
There are many tests that use the same logic for classes that requires async
shutdown, e.g. see
https://codesearch.chromium.org/chromium/src/remoting/protocol/connection_uni...

[joedow, 2017/02/03 22:32:46]

There are some cases which do not complete the connection process so they don't
need this logic.

 }
 
 TEST_F(SecurityKeyIpcServerTest, HandleLargeSecurityKeyRequest) {
@@ -195,6 +205,7 @@ TEST_F(SecurityKeyIpcServerTest, HandleLargeSecurityKeyRequest) {
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 TEST_F(SecurityKeyIpcServerTest, HandleReallyLargeSecurityKeyRequest) {
@@ -232,6 +243,7 @@ TEST_F(SecurityKeyIpcServerTest, HandleReallyLargeSecurityKeyRequest) {
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 TEST_F(SecurityKeyIpcServerTest, HandleMultipleSecurityKeyRequests) {
@@ -286,6 +298,7 @@ TEST_F(SecurityKeyIpcServerTest, HandleMultipleSecurityKeyRequests) {
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 TEST_F(SecurityKeyIpcServerTest, InitialIpcConnectionTimeout_ConnectOnly) {
@@ -476,6 +489,7 @@ TEST_F(SecurityKeyIpcServerTest, CleanupPendingConnection) {
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 #if defined(OS_WIN)
@@ -489,16 +503,15 @@ TEST_F(SecurityKeyIpcServerTest, IpcConnectionFailsFromInvalidSession) {
       security_key_ipc_server_->CreateChannel(channel_handle, request_timeout));
 
   // Create a fake client and attempt to connect to the IPC server channel.
-  FakeSecurityKeyIpcClient fake_ipc_client(base::Bind(
-      &SecurityKeyIpcServerTest::OperationComplete, base::Unretained(this)));
+  FakeSecurityKeyIpcClient fake_ipc_client(base::Bind(&base::DoNothing));
   ASSERT_TRUE(fake_ipc_client.ConnectViaIpc(channel_handle));
   WaitForOperationComplete();
-  WaitForOperationComplete();
-  WaitForOperationComplete();
 
   // Verify the connection failed.
   ASSERT_TRUE(fake_ipc_client.invalid_session_error());
   ASSERT_FALSE(fake_ipc_client.connection_ready());
+
+  WaitForPendingOperations();
   ASSERT_FALSE(fake_ipc_client.ipc_channel_connected());
 }
 #endif  // defined(OS_WIN)