Capture a report on failed browser rendez-vous.

chrome/browser/process_singleton_win.cc

 // Copyright (c) 2012 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 <shellapi.h>
 #include <stddef.h>
+#include <winbase.h>

[grt (UTC plus 2), 2016/04/05 17:22:08]

change this to #include <windows.h> and move it above the other two includes.

[manzagop (departed), 2016/04/06 19:36:02]

Done.

+
+#include <cwchar>

[grt (UTC plus 2), 2016/04/05 17:22:07]

why? wchar_t is a keyword. is there something else from this header you need?

[manzagop (departed), 2016/04/06 19:36:01]

Done.

Dead code (was using wcscmp at one point).

 
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/path_service.h"
 #include "base/process/process.h"
 #include "base/process/process_info.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/chrome_process_finder_win.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/ui/simple_message_box.h"
+#include "chrome/chrome_watcher/kasko_util.h"
+#include "chrome/common/channel_info.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/grit/chromium_strings.h"
+#include "chrome/installer/util/util_constants.h"
 #include "chrome/installer/util/wmi.h"
+#include "components/version_info/version_info.h"
 #include "content/public/common/result_codes.h"
 #include "net/base/escape.h"
+#include "third_party/kasko/kasko_features.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/win/hwnd_util.h"
 
 namespace {
 
 const char kLockfile[] = "lockfile";
 
 // A helper class that acquires the given |mutex| while the AutoLockMutex is in
 // scope.
 class AutoLockMutex {
@@ skipping 125 matching lines @@
   return true;
 }
 
 bool DisplayShouldKillMessageBox() {
   return chrome::ShowQuestionMessageBox(
              NULL, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
              l10n_util::GetStringUTF16(IDS_BROWSER_HUNGBROWSER_MESSAGE)) !=
          chrome::MESSAGE_BOX_RESULT_NO;
 }
 
+#if BUILDFLAG(ENABLE_KASKO)
+#if BUILDFLAG(ENABLE_KASKO_FAILED_RDV_REPORTS)
+// Capture a failed rendez-vous hang report of the other process. Kasko needs
+// the exception context to live either in the dumper of the dumpee. This

[grt (UTC plus 2), 2016/04/05 17:22:07]

of -> or?

[manzagop (departed), 2016/04/06 19:36:01]

Done.

+// means we cannot rely on kasko reporters from either browser watcher, and
+// instead spin up a new reporter.
+void SendFailedRdvReport(const base::Process& process, const DWORD thread_id) {

[grt (UTC plus 2), 2016/04/05 17:22:07]

remove "const"

[manzagop (departed), 2016/04/06 19:36:02]

Done.

+  // TODO(manzagop): add a metric for the number of captured hang reports, for
+  //   comparison with uploaded count?
+
+  // Ensure the target process shares the current process' executable name. This

[grt (UTC plus 2), 2016/04/05 17:22:08]

nit: process's

[manzagop (departed), 2016/04/06 19:36:02]

Done. 

+  // is to defend against races that exist in getting a Process from a window
+  // name (HWND and process id recycling).
+  // TODO(manzagop): add a metric for the number of times this does not match.
+  wchar_t exe_name_self[MAX_PATH];
+  if (GetModuleFileName(nullptr, exe_name_self, MAX_PATH) == 0)

[Patrick Monette, 2016/04/04 23:33:40]

You can use PathService::Get(FILE_EXE) instead;

[manzagop (departed), 2016/04/06 19:36:02]

Done.

+    return;
+  wchar_t exe_name_other[MAX_PATH];
+  DWORD exe_name_other_len = MAX_PATH;
+  if (QueryFullProcessImageName(process.Handle(), 0, exe_name_other,
+                                &exe_name_other_len) == 0) {
+    DWORD error = GetLastError();

[grt (UTC plus 2), 2016/04/05 17:22:07]

remove this

[manzagop (departed), 2016/04/06 19:36:02]

Done.

+    DPLOG(INFO) << "Failed to get executable name for other process: " << error;

[grt (UTC plus 2), 2016/04/05 17:22:07]

The *P* class of log functions automatically add the friendly text for the last
error code, so change this to just:
    DPLOG(ERROR) << "Failed to get executable name for other process";

[manzagop (departed), 2016/04/06 19:36:02]

Done.

+    return;
+  }
+  // Note: FilePath handles the complexity of path comparision (eg case).
+  if (base::FilePath(exe_name_self) != base::FilePath(exe_name_other))

[Patrick Monette, 2016/04/04 23:33:40]

Can you move the exe name comparison to a function? I'll need to reuse it in the
case the Wait Chain Traversal API gives me another process.

[Sigurður Ásgeirsson, 2016/04/05 13:52:29]

@grt may be familiar with cases where this comparison could fail. I wonder
whether it might be better/sufficient to compare the directory name.
Also, I'm not sure whether this does right by casing differences and such?

[grt (UTC plus 2), 2016/04/05 17:22:08]

We generally use base::FilePath::CompareEqualIgnoreCase if we think the paths
are going to be the same names for the file, or if false negatives aren't a
concern. If you think they may be different names for the same underlying file,
then I don't think you can do better than getting handles to them and comparing
their volume serial number and file index.

[manzagop (departed), 2016/04/06 19:36:02]

Done.

[manzagop (departed), 2016/04/06 19:36:02]

Acknowledged.

[manzagop (departed), 2016/04/06 19:36:02]

False negatives are not an issue (though I have a TODO to add a metric to know
how prevalent they are). Will go with CompareEqualIgnoreCase.

Done.

+    return;
+
+  // Only report on canary (or unspecified).
+  const version_info::Channel channel = chrome::GetChannel();

[grt (UTC plus 2), 2016/04/05 17:22:08]

consider doing this higher up in the function so that you don't incur the other
costs only to discard them due to the channel.

[manzagop (departed), 2016/04/06 19:36:02]

Done.

+  if (channel != version_info::Channel::UNKNOWN &&
+      channel != version_info::Channel::CANARY) {
+    return;
+  }
+
+  // Initialize a reporter, capture a report and shutdown the reporter.
+  base::FilePath watcher_data_directory;
+  if (PathService::Get(chrome::DIR_WATCHER_DATA, &watcher_data_directory)) {
+    base::string16 endpoint =
+        L"chrome_kasko_rdv_" +
+        base::UintToString16(base::Process::Current().Pid());
+
+    bool launched_kasko = InitializeKaskoReporter(
+        endpoint, watcher_data_directory.value().c_str());
+    if (launched_kasko) {
+      DumpHungProcess(thread_id, installer::kChromeChannelCanary, L"failed-rdv",
+                      process);
+      // We immediately request Kasko shutdown. This may block until the
+      // completion of ongoing background tasks (eg upload). If the report is

[grt (UTC plus 2), 2016/04/05 17:22:08]

nit: "e.g., upload"

[manzagop (departed), 2016/04/06 19:36:01]

Done.

+      // not uploaded by this reporter, any other Kasko reporter may upload it.
+      ShutdownKaskoReporter();
+    }
+  }
+}
+#endif  // BUILDFLAG(ENABLE_KASKO_FAILED_RDV_REPORTS)
+#endif  // BUILDFLAG(ENABLE_KASKO)
+
 }  // namespace
 
 // Microsoft's Softricity virtualization breaks the sandbox processes.
 // So, if we detect the Softricity DLL we use WMI Win32_Process.Create to
 // break out of the virtualization environment.
 // http://code.google.com/p/chromium/issues/detail?id=43650
 bool ProcessSingleton::EscapeVirtualization(
     const base::FilePath& user_data_dir) {
   if (::GetModuleHandle(L"sftldr_wow64.dll") ||
       ::GetModuleHandle(L"sftldr.dll")) {
@@ skipping 50 matching lines @@
     case chrome::NOTIFY_SUCCESS:
       return PROCESS_NOTIFIED;
     case chrome::NOTIFY_FAILED:
       remote_window_ = NULL;
       return PROCESS_NONE;
     case chrome::NOTIFY_WINDOW_HUNG:
       // Fall through and potentially terminate the hung browser.
       break;
   }
 
+  // The window is hung.

[grt (UTC plus 2), 2016/04/05 17:22:07]

https://youtu.be/O4gqsuww6lw

[manzagop (departed), 2016/04/06 19:36:02]

Acknowledged?! ;)

   DWORD process_id = 0;
   DWORD thread_id = ::GetWindowThreadProcessId(remote_window_, &process_id);
   if (!thread_id || !process_id) {
     remote_window_ = NULL;
     return PROCESS_NONE;
   }
+
+  // Get a handle to the process that created the window.
   base::Process process = base::Process::Open(process_id);
 
-  // The window is hung. Scan for every window to find a visible one.
+  // Optionally send a failed rendez-vous report.
+  // Note: we nominate the thread that created the window as the root of the
+  // search for a hung thread.
+#if BUILDFLAG(ENABLE_KASKO)
+#if BUILDFLAG(ENABLE_KASKO_FAILED_RDV_REPORTS)
+  SendFailedRdvReport(process, thread_id);
+#endif  // BUILDFLAG(ENABLE_KASKO_FAILED_RDV_REPORTS)
+#endif  // BUILDFLAG(ENABLE_KASKO)
+
+  // Scan for every window to find a visible one.
   bool visible_window = false;
   ::EnumThreadWindows(thread_id,
                       &BrowserWindowEnumeration,
                       reinterpret_cast<LPARAM>(&visible_window));
 
   // If there is a visible browser window, ask the user before killing it.
   if (visible_window && !should_kill_remote_process_callback_.Run()) {
     // The user denied. Quit silently.
     return PROCESS_NOTIFIED;
   }
@@ skipping 77 matching lines @@
   return window_.hwnd() != NULL;
 }
 
 void ProcessSingleton::Cleanup() {
 }
 
 void ProcessSingleton::OverrideShouldKillRemoteProcessCallbackForTesting(
     const ShouldKillRemoteProcessCallback& display_dialog_callback) {
   should_kill_remote_process_callback_ = display_dialog_callback;
 }