Fixing Intermittent SecurityKey Unittest Failures

remoting/host/security_key/security_key_ipc_server_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/security_key/security_key_ipc_server.h"
 
 #include <memory>
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #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"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 const int kTestConnectionId = 42;
 const int kInitialConnectTimeoutMs = 250;
 const int kConnectionTimeoutErrorDeltaMs = 100;
 const int kLargeMessageSizeBytes = 256 * 1024;
@@ skipping 15 matching lines @@
   void SendRequestToClient(int connection_id, const std::string& data);
 
  protected:
   // Returns a unique IPC channel name which prevents conflicts when running
   // tests concurrently.
   std::string GetUniqueTestChannelName();
 
   // 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_; }
 
   // 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_;
 
   // The object under test.
   std::unique_ptr<SecurityKeyIpcServer> security_key_ipc_server_;
 
   // Used to validate the object under test uses the correct ID when
   // communicating over the IPC channel.
   int last_connection_id_received_ = -1;
 
   // Stores the contents of the last IPC message received for validation.
   std::string last_message_received_;
 
   uint32_t peer_session_id_ = UINT32_MAX;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SecurityKeyIpcServerTest);
 };
 
 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_)));
 #endif  // defined(OS_WIN)
 
   security_key_ipc_server_ = remoting::SecurityKeyIpcServer::Create(
       kTestConnectionId, this,
       base::TimeDelta::FromMilliseconds(kInitialConnectTimeoutMs),
       base::Bind(&SecurityKeyIpcServerTest::SendRequestToClient,
                  base::Unretained(this)),
       base::Bind(&base::DoNothing),
       base::Bind(&SecurityKeyIpcServerTest::OperationComplete,
                  base::Unretained(this)));
 }
 
 SecurityKeyIpcServerTest::~SecurityKeyIpcServerTest() {}
 
 void SecurityKeyIpcServerTest::OperationComplete() {
   run_loop_->Quit();
 }
 
 void SecurityKeyIpcServerTest::WaitForOperationComplete() {
   run_loop_->Run();
   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;
   last_message_received_ = data;
   OperationComplete();
 }
 
 std::string SecurityKeyIpcServerTest::GetUniqueTestChannelName() {
   return GetChannelNamePathPrefixForTest() + "Super_Awesome_Test_Channel." +
          IPC::Channel::GenerateUniqueRandomChannelID();
 }
 
 TEST_F(SecurityKeyIpcServerTest, HandleSingleSecurityKeyRequest) {
   mojo::edk::NamedPlatformHandle channel_handle(GetUniqueTestChannelName());
   ASSERT_TRUE(security_key_ipc_server_->CreateChannel(
       channel_handle,
       /*request_timeout=*/base::TimeDelta::FromMilliseconds(500)));
 
   // Create a fake client and connect to the IPC server channel.
   FakeSecurityKeyIpcClient fake_ipc_client(base::Bind(
       &SecurityKeyIpcServerTest::OperationComplete, base::Unretained(this)));
   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!");
   fake_ipc_client.SendSecurityKeyRequestViaIpc(request_data);
   WaitForOperationComplete();
 
   // Verify the request was received.
   ASSERT_EQ(kTestConnectionId, last_connection_id_received_);
   ASSERT_EQ(request_data, last_message_received_);
 
   // Send a response from the IPC server to the IPC client.
   std::string response_data("Blargh!");
   ASSERT_TRUE(security_key_ipc_server_->SendResponse(response_data));
   WaitForOperationComplete();
 
   // Verify the request was received.
   ASSERT_EQ(response_data, fake_ipc_client.last_message_received());
 
   // 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) {
   mojo::edk::NamedPlatformHandle channel_handle(GetUniqueTestChannelName());
   ASSERT_TRUE(security_key_ipc_server_->CreateChannel(
       channel_handle,
       /*request_timeout=*/base::TimeDelta::FromMilliseconds(500)));
 
   // Create a fake client and connect to the IPC server channel.
   FakeSecurityKeyIpcClient fake_ipc_client(base::Bind(
@@ skipping 17 matching lines @@
   // Send a response from the IPC server to the IPC client.
   std::string response_data(kLargeMessageSizeBytes, 'Z');
   ASSERT_TRUE(security_key_ipc_server_->SendResponse(response_data));
   WaitForOperationComplete();
 
   // Verify the request was received.
   ASSERT_EQ(response_data, fake_ipc_client.last_message_received());
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 TEST_F(SecurityKeyIpcServerTest, HandleReallyLargeSecurityKeyRequest) {
   mojo::edk::NamedPlatformHandle channel_handle(GetUniqueTestChannelName());
   ASSERT_TRUE(security_key_ipc_server_->CreateChannel(
       channel_handle,
       /*request_timeout=*/base::TimeDelta::FromMilliseconds(500)));
 
   // Create a fake client and connect to the IPC server channel.
   FakeSecurityKeyIpcClient fake_ipc_client(base::Bind(
@@ skipping 17 matching lines @@
   // Send a response from the IPC server to the IPC client.
   std::string response_data(kLargeMessageSizeBytes * 2, 'Z');
   ASSERT_TRUE(security_key_ipc_server_->SendResponse(response_data));
   WaitForOperationComplete();
 
   // Verify the request was received.
   ASSERT_EQ(response_data, fake_ipc_client.last_message_received());
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 TEST_F(SecurityKeyIpcServerTest, HandleMultipleSecurityKeyRequests) {
   mojo::edk::NamedPlatformHandle channel_handle(GetUniqueTestChannelName());
   ASSERT_TRUE(security_key_ipc_server_->CreateChannel(
       channel_handle,
       /*request_timeout=*/base::TimeDelta::FromMilliseconds(500)));
 
   // Create a fake client and connect to the IPC server channel.
   FakeSecurityKeyIpcClient fake_ipc_client(base::Bind(
@@ skipping 34 matching lines @@
   // Send a response from the IPC server to the IPC client.
   std::string response_data_2("Meh!");
   ASSERT_TRUE(security_key_ipc_server_->SendResponse(response_data_2));
   WaitForOperationComplete();
 
   // Verify the response was received.
   ASSERT_EQ(response_data_2, fake_ipc_client.last_message_received());
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 TEST_F(SecurityKeyIpcServerTest, InitialIpcConnectionTimeout_ConnectOnly) {
   // Create a channel, then wait for the done callback to be called indicating
   // the connection was closed.  This test simulates the IPC Server being
   // created, the client connecting to the OS channel, but never communicating
   // over the channel.
   mojo::edk::NamedPlatformHandle channel_handle(GetUniqueTestChannelName());
   ASSERT_TRUE(security_key_ipc_server_->CreateChannel(
       channel_handle,
@@ skipping 170 matching lines @@
   // Send a response from the IPC server to the IPC client.
   std::string response_data("Blargh!");
   ASSERT_TRUE(security_key_ipc_server_->SendResponse(response_data));
   WaitForOperationComplete();
 
   // Verify the request was received.
   ASSERT_EQ(response_data, fake_ipc_client.last_message_received());
 
   // Typically the client will be the one to close the connection.
   fake_ipc_client.CloseIpcConnection();
+  WaitForOperationComplete();
 }
 
 #if defined(OS_WIN)
 TEST_F(SecurityKeyIpcServerTest, IpcConnectionFailsFromInvalidSession) {
   // Change the expected session ID to not match the current session.
   peer_session_id_++;
 
   base::TimeDelta request_timeout(base::TimeDelta::FromMilliseconds(500));
   mojo::edk::NamedPlatformHandle channel_handle(GetUniqueTestChannelName());
   ASSERT_TRUE(
       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)
 
 }  // namespace remoting