Add test for ProcessSingleton hung rendezvous case.

chrome/browser/process_singleton_win_unittest.cc

Index: chrome/browser/process_singleton_win_unittest.cc
diff --git a/chrome/browser/process_singleton_win_unittest.cc b/chrome/browser/process_singleton_win_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b03712c0a8e758e840d4c2bf40660656f0c45055
--- /dev/null
+++ b/chrome/browser/process_singleton_win_unittest.cc
@@ -0,0 +1,292 @@
+// Copyright 2015 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 "chrome/browser/process_singleton.h"
+
+#include <windows.h>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/compiler_specific.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/process/launch.h"
+#include "base/process/process.h"
+#include "base/process/process_handle.h"
+#include "base/strings/string16.h"
+#include "base/strings/stringprintf.h"
+#include "base/test/multiprocess_test.h"
+#include "base/win/scoped_handle.h"
+#include "base/win/wrapped_window_proc.h"
+#include "chrome/browser/chrome_process_finder_win.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_switches.h"
+#include "content/public/common/result_codes.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/multiprocess_func_list.h"
+
+namespace {
+
+const char kReadyEventNameFlag[] = "ready_event_name";
+const char kContinueEventNameFlag[] = "continue_event_name";
+const char kCreateWindowFlag[] = "create_window";
+const int kErrorResultCode = 0x345;
+
+bool NotificationCallback(
+      const base::CommandLine& command_line,
+      const base::FilePath& current_directory) {
+  return true;

[gab, 2015/03/11 13:20:40]

Remind the reader with a comment of what it means to return true here?

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+}
+
+HWND CreateVisibleWindow() {
+  WNDCLASSEX wnd_cls = {};
+  base::win::InitializeWindowClass(
+      L"ProcessSingletonTest",
+      base::win::WrappedWindowProc<::DefWindowProc>,
+      0,  // style
+      0,  // class_extra
+      0,  // window_extra
+      NULL,  // cursor
+      NULL,  // background
+      L"",  // menu_name
+      NULL,  // large_icon
+      NULL,  // small_icon
+      &wnd_cls);
+
+  ATOM clazz = ::RegisterClassEx(&wnd_cls);
+
+  // Create a visible popup window to trigger user interaction in the
+  // process singleton.
+  HWND window = ::CreateWindow(reinterpret_cast<LPCWSTR>(clazz),
+                               0, WS_POPUP, 0, 0, 0, 0, 0, 0, NULL, 0);
+  if (window)

[gab, 2015/03/11 13:20:39]

Can this ever be NULL in a valid run of this test? Should you EXPECT_TRUE that
it's not?

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

This happens in the remote instance - error is flagged by the process exit code.

+    ::ShowWindow(window, SW_SHOW);
+
+  return window;
+}
+
+MULTIPROCESS_TEST_MAIN(ProcessSingletonTestProcessMain) {
+  base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
+  base::FilePath user_data_dir =
+      cmd_line->GetSwitchValuePath(switches::kUserDataDir);
+  if (user_data_dir.empty())

[gab, 2015/03/11 13:20:40]

Doesn't ASSERT semantic work here and below rather than using the
kErrorResultCode? (simply asking, first time I see a base::MultiProcessTest)

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

I just tested this, and there's no support for the ASSERTs in the subprocess.

+    return kErrorResultCode;
+
+  base::string16 ready_event_name =
+      cmd_line->GetSwitchValueNative(kReadyEventNameFlag);
+
+  base::win::ScopedHandle ready_event(
+      ::OpenEvent(EVENT_MODIFY_STATE, FALSE, ready_event_name.c_str()));
+  if (!ready_event.IsValid())
+    return kErrorResultCode;
+
+  base::string16 continue_event_name =
+      cmd_line->GetSwitchValueNative(kContinueEventNameFlag);
+
+  base::win::ScopedHandle continue_event(
+      ::OpenEvent(SYNCHRONIZE, FALSE, continue_event_name.c_str()));
+  if (!continue_event.IsValid())
+    return kErrorResultCode;
+
+  if (cmd_line->HasSwitch(kCreateWindowFlag)) {
+    HWND window = CreateVisibleWindow();
+    if (!window)
+      return kErrorResultCode;
+  }
+
+  // Instantiate the process singleton.
+  ProcessSingleton process_singleton(user_data_dir,
+                                     base::Bind(&NotificationCallback));
+
+  if (!process_singleton.Create())
+    return kErrorResultCode;
+
+  // Signal ready and block for the continue event.
+  if (!::SetEvent(ready_event.Get()))
+    return kErrorResultCode;
+
+  if (::WaitForSingleObject(continue_event.Get(), INFINITE) != WAIT_OBJECT_0)
+    return kErrorResultCode;
+
+  return 0;
+}
+
+class ProcessSingletonTest : public base::MultiProcessTest {
+  typedef base::MultiProcessTest Super;

[gab, 2015/03/11 13:20:40]

I've never seen this style in Chromium, tests typically refer explicitly to
their parent's type when forwarding things like SetUp() and TearDown().

Not using Super doesn't add reading complexity (if anything it reduces it as the
reader has one less type to keep in his mind).

Especially since you not only use Super in SetUp and TearDown (which arguably
are likely to be overridable if the Super type changes), but use it in
MakeCmdLine which is specific to base::MultiProcessTest -- so might as well be
explicit about it.

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+ public:

[gab, 2015/03/11 13:20:39]

s/public/protected

(I was recently told to do so by other reviewers as nothing in the test fixture
needs to be public)

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+  ProcessSingletonTest() : window_option_(NO_WINDOW), msg_box_called_(false) {
+  }
+
+  void SetUp() override {
+    ASSERT_NO_FATAL_FAILURE(Super::SetUp());
+
+    // Drop the process finder notification timeout to one second for testing.
+    old_notification_timeout_ =
+        chrome::SetNotificationTimeoutInSecondsForTesting(1);
+  }
+
+  void TearDown() override {
+    chrome::SetNotificationTimeoutInSecondsForTesting(
+        old_notification_timeout_);

[gab, 2015/03/11 13:20:40]

Why bother restoring it? Other unit tests shouldn't get involved with
ProcessSingleton anyways (besides other ProcessSingleton tests in this fixture
which may run in parallel in the same process, but they all desire the 1s
timeout so wtv).

Not doing so would simplify the setters' interface too which IMO is atypical
(setters rarely return the old value)

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

There are apparently cases where multiple tests run inside the same process.
I've had DrMemory FYI waterfall test failures occur in unrelated tests due to
coupling through histograms as a case in point.

IMHO a global is a very bad idea(TM), but if you must have one, leave it as you
found it.

[gab, 2015/03/11 15:19:43]

Right, tests do run in parallel. But by setting the global back as you found it
you only decrease the likelihood of tripping something that would depend on
that, but if whatever barfs when this is set to 1s happens to run in parallel
with your test it will still barf... So might as well never set it back IMO
(making potential conflicts more deterministic).

+
+    if (browser_victim_.IsValid()) {
+      EXPECT_TRUE(::SetEvent(continue_event_.Get()));
+      int exit_code = 0;
+      EXPECT_TRUE(browser_victim_.WaitForExit(&exit_code));
+    }
+
+    Super::TearDown();
+  }
+
+  enum WindowOption {

[gab, 2015/03/11 13:20:39]

The Google C++ style-guide requires that typedefs/enums be at the top of each
class' sections:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+    WITH_WINDOW,
+    NO_WINDOW
+  };
+
+  void LaunchHungBrowserProcess(WindowOption window_option) {
+    // Create a unique user data dir to rendezvous on.
+    ASSERT_TRUE(user_data_dir_.CreateUniqueTempDir());
+
+    // Create the named "ready" event, this is unique to our process.
+    ready_event_name_ =
+        base::StringPrintf(L"ready-event-%d", base::GetCurrentProcId());
+    base::win::ScopedHandle ready_event(
+        ::CreateEvent(NULL, TRUE, FALSE, ready_event_name_.c_str()));
+    ASSERT_TRUE(ready_event.IsValid());
+
+    // Create the named "continue" event, this is unique to our process.
+    continue_event_name_ =
+        base::StringPrintf(L"continue-event-%d", base::GetCurrentProcId());
+    continue_event_.Set(
+        ::CreateEvent(NULL, TRUE, FALSE, continue_event_name_.c_str()));
+    ASSERT_TRUE(continue_event_.IsValid());
+
+    window_option_ = window_option;
+
+    base::LaunchOptions options;
+    options.start_hidden = true;
+    browser_victim_ =
+        SpawnChildWithOptions("ProcessSingletonTestProcessMain", options);
+
+    // Wait for the ready event (or process exit).
+    HANDLE handles[] = { ready_event.Get(), browser_victim_.Handle() };
+    DWORD result = ::WaitForMultipleObjects(arraysize(handles),
+                                            handles,
+                                            FALSE,
+                                            INFINITE);
+    ASSERT_EQ(result, WAIT_OBJECT_0);

[gab, 2015/03/11 13:20:39]

Assert that |ready_event| was signaled? Other pieces below (i.e. line 197) seem
to assume that if this returns with no fatal failure it's because the
|ready_event| was signaled.

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

I don't follow - WAIT_OBJECT_0 == ready_event?

[gab, 2015/03/11 15:19:42]

Oh actually nvm, I just realized that this is already what this line was doing
(had forgot the WAIT_OBJECT_0 + n semantic (where n == 0 here)).

Maybe add a comment like:
// The wait should always return because |ready_event| was signaled or
|browser_victim_| died unexpectedly.

+  }
+
+  base::CommandLine MakeCmdLine(const std::string& procname) override {
+    base::CommandLine cmd_line = Super::MakeCmdLine(procname);
+
+    cmd_line.AppendSwitchPath(switches::kUserDataDir, user_data_dir_.path());
+    cmd_line.AppendSwitchNative(kReadyEventNameFlag, ready_event_name_);
+    cmd_line.AppendSwitchNative(kContinueEventNameFlag, continue_event_name_);
+    if (window_option_ == WITH_WINDOW)
+      cmd_line.AppendSwitch(kCreateWindowFlag);
+
+    return cmd_line;
+  }
+
+  void PrepareTest(WindowOption window_option, bool allow_kill) {
+    ASSERT_NO_FATAL_FAILURE(LaunchHungBrowserProcess(window_option));
+
+    // The ready event has been signalled - the process singleton is held by
+    // the hung sub process.
+    test_singleton_.reset(
+        new ProcessSingleton(user_data_dir(),
+                            base::Bind(&NotificationCallback)));

[gab, 2015/03/11 13:20:39]

nit: + single space (suggest running "git cl format")

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+
+    test_singleton_->OverrideKillMessageBoxCallbackForTesting(
+        base::Bind(&ProcessSingletonTest::MockDisplayKillMessageBox,
+                   base::Unretained(this), allow_kill));
+  }
+
+  base::Process& browser_victim() { return browser_victim_; }

[gab, 2015/03/11 13:20:40]

Use a * (non-const & are not-allowed by style guide)

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+  const base::FilePath& user_data_dir() const { return user_data_dir_.path(); }
+  ProcessSingleton* test_singleton() const { return test_singleton_.get(); }
+  bool msg_box_called() const { return msg_box_called_; }
+
+ private:
+  bool MockDisplayKillMessageBox(bool result) {

[gab, 2015/03/11 13:20:40]

s/result/allow_kill ? (I had to piece things around to figure out that's what
this meant, seems a better name would help readability?)

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+    msg_box_called_ = true;
+    return result;
+  }
+
+  base::string16 ready_event_name_;
+  base::string16 continue_event_name_;
+
+  WindowOption window_option_;
+  base::ScopedTempDir user_data_dir_;
+  base::Process browser_victim_;
+  base::win::ScopedHandle continue_event_;
+
+  scoped_ptr<ProcessSingleton> test_singleton_;
+
+  int old_notification_timeout_;
+  int msg_box_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProcessSingletonTest);
+};
+
+}  // namespace
+
+TEST_F(ProcessSingletonTest, KillsHungBrowserWithNoWindows) {
+  ASSERT_NO_FATAL_FAILURE(PrepareTest(NO_WINDOW, false));
+
+  // As the hung browser has no visible window, it'll be killed without
+  // user interaction.
+  ProcessSingleton::NotifyResult notify_result =
+      test_singleton()->NotifyOtherProcessOrCreate();
+  ASSERT_EQ(notify_result, ProcessSingleton::PROFILE_IN_USE);
+
+  // The message box should not have been invoked, as the "browser" had no
+  // windows.
+  EXPECT_FALSE(msg_box_called());
+
+  int exit_code = 0;
+  EXPECT_TRUE(
+      browser_victim().WaitForExitWithTimeout(base::TimeDelta(), &exit_code));
+  EXPECT_EQ(exit_code, content::RESULT_CODE_HUNG);

[gab, 2015/03/11 13:20:39]

Add a comment stating that RESULT_CODE_HUNG is expected to be the exit code on
the hung process we just killed (i.e. I had to code search to figure out how
this exit code got set, ideally I read a quick comment here and trust your words
without having to search around).

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

Done.

+}
+
+TEST_F(ProcessSingletonTest, DoesntKillWithoutUserPermission) {
+  ASSERT_NO_FATAL_FAILURE(PrepareTest(WITH_WINDOW, false));
+
+  // As the hung browser has a visible window, this should query the user
+  // before killing the hung process.
+  ProcessSingleton::NotifyResult notify_result =
+      test_singleton()->NotifyOtherProcessOrCreate();
+  ASSERT_EQ(notify_result, ProcessSingleton::PROCESS_NOTIFIED);
+
+  // The message box should have been invoked, as the "browser" has a
+  // visible window.
+  EXPECT_TRUE(msg_box_called());
+
+  // Make sure the process hasn't been killed.
+  int exit_code = 0;
+  EXPECT_FALSE(
+      browser_victim().WaitForExitWithTimeout(base::TimeDelta(), &exit_code));
+}
+
+TEST_F(ProcessSingletonTest, KillWithUserPermission) {
+  ASSERT_NO_FATAL_FAILURE(PrepareTest(WITH_WINDOW, true));
+
+  // As the hung browser has a visible window, this should query the user
+  // before killing the hung process.
+  ProcessSingleton::NotifyResult notify_result =
+      test_singleton()->NotifyOtherProcessOrCreate();
+  ASSERT_EQ(notify_result, ProcessSingleton::PROFILE_IN_USE);
+
+  // The message box should have been invoked, as the "browser" has a
+  // visible window.
+  EXPECT_TRUE(msg_box_called());
+
+  int exit_code = 0;
+  EXPECT_TRUE(
+      browser_victim().WaitForExitWithTimeout(base::TimeDelta(), &exit_code));
+  EXPECT_EQ(exit_code, content::RESULT_CODE_HUNG);
+}

[gab, 2015/03/11 13:20:39]

Should we also add a test that the normal use case for process singleton (i.e.
successful rendez-vous) works?

[Sigurður Ásgeirsson, 2015/03/11 14:32:37]

There are other tests for that, which test the platform-independent success
path.

[gab, 2015/03/11 15:19:42]

I see, maybe stress that in a comment on line 116 (test fixture class
definition) to express the exact purpose of these tests.