Capture a report on failed browser rendez-vous.

chrome/chrome_watcher/kasko_util.cc

Index: chrome/chrome_watcher/kasko_util.cc
diff --git a/chrome/chrome_watcher/kasko_util.cc b/chrome/chrome_watcher/kasko_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4155f35a32660db5c393f46dde0465ac1d37855d
--- /dev/null
+++ b/chrome/chrome_watcher/kasko_util.cc
@@ -0,0 +1,215 @@
+// 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 "chrome/chrome_watcher/kasko_util.h"
+
+#include <sddl.h>
+
+#include <string>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/environment.h"
+#include "base/files/file_path.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/win/win_util.h"
+
+#include "chrome/chrome_watcher/chrome_watcher_main_api.h"
+
+#if BUILDFLAG(ENABLE_KASKO)
+#include "components/crash/content/app/crashpad.h"
+#include "syzygy/kasko/api/reporter.h"
+#endif
+
+#if BUILDFLAG(ENABLE_KASKO)
+
+namespace {
+
+// Helper function for determining the crash server to use. Defaults to the
+// standard crash server, but can be overridden via an environment variable.
+// Enables easy integration testing.
+void GetKaskoCrashServerUrl(base::string16* crash_server) {

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

return the string16 rather than taking it as an out param.

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

Done.

+  const char kKaskoCrashServerUrl[] = "KASKO_CRASH_SERVER_URL";

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

static const char

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

Done.

+  static const wchar_t kDefaultKaskoCrashServerUrl[] =
+      L"https://clients2.google.com/cr/report";
+
+  auto env = base::Environment::Create();

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

#include <memory>

  std::unique_ptr<base::Environment> env(base::Environment::Create());

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

Ouch. Done.

+  std::string env_var;
+  if (env->GetVar(kKaskoCrashServerUrl, &env_var)) {
+    base::UTF8ToWide(env_var.c_str(), env_var.size(), crash_server);

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

do you really mean UTF8ToUTF16 here (the destination type is a string16, not a
wstring)?

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

Done.

+  } else {
+    *crash_server = kDefaultKaskoCrashServerUrl;
+  }
+}
+
+// Helper function for determining the crash reports directory to use. Defaults
+// to the browser data directory, but can be overridden via an environment
+// variable. Enables easy integration testing.
+void GetKaskoCrashReportsBaseDir(const base::char16* browser_data_directory,
+                                 base::FilePath* base_dir) {

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

return a base::FilePath rather than taking it as an out arg. FilePaths are
treated like value types.

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

Done.

+  const char kKaskoCrashReportBaseDir[] = "KASKO_CRASH_REPORTS_BASE_DIR";

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

static

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

Done.

+  auto env = base::Environment::Create();

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

unique_ptr here, too

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

Done.

+  std::string env_var;
+  if (env->GetVar(kKaskoCrashReportBaseDir, &env_var)) {
+    base::string16 wide_env_var;
+    base::UTF8ToWide(env_var.c_str(), env_var.size(), &wide_env_var);
+    *base_dir = base::FilePath(wide_env_var);
+  } else {
+    *base_dir = base::FilePath(browser_data_directory);
+  }
+}
+
+void LoggedDeregisterEventSource(HANDLE event_source_handle) {
+  if (!::DeregisterEventSource(event_source_handle))
+    DPLOG(ERROR) << "DeregisterEventSource";
+}
+
+void LoggedLocalFree(PSID sid) {
+  if (::LocalFree(sid) != nullptr)
+    DPLOG(ERROR) << "LocalFree";
+}
+
+void OnCrashReportUpload(void* context,
+                         const base::char16* report_id,
+                         const base::char16* minidump_path,
+                         const base::char16* const* keys,
+                         const base::char16* const* values) {
+  // Open the event source.
+  HANDLE event_source_handle = ::RegisterEventSource(NULL, L"Chrome");

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

i somewhat prefer using std::unique_ptr with a custom deleter over
ScopedClosureRunner. it's more efficient, less generated code, and provides a
stronger lifetime guarantee.

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

NULL -> nullptr everywhere

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

Done in this file. Did not change "untouched lines" in other files, eg
process_singleton_win.

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

Done. Also applied to sid below, but it's not as clean.

+  if (!event_source_handle) {
+    PLOG(ERROR) << "RegisterEventSource";
+    return;
+  }
+  // Ensure cleanup on scope exit.
+  base::ScopedClosureRunner deregister_event_source(
+      base::Bind(&LoggedDeregisterEventSource, event_source_handle));
+
+  // Get the user's SID for the log record.
+  base::string16 sid_string;
+  PSID sid = nullptr;
+  if (base::win::GetUserSidString(&sid_string)) {
+    if (!sid_string.empty()) {
+      if (!::ConvertStringSidToSid(sid_string.c_str(), &sid))
+        DPLOG(ERROR) << "ConvertStringSidToSid";
+      DCHECK(sid);

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

do you also want this check if Get UserSidString fails? does it make sense to
get rid of the log above this and change this to DPCHECK(sid)? does this make
sense:

  if (base::win::GetUserSidString(&sid_string) && !sid_string.empty())
    ::ConvertStringSidToSid(sid_string.c_str(), &sid);
  DPCHECK(sid);

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

It seems ok for ReportEvent (below) to get a null sid. In terms of this
function's expectations, I'm not sure. In doubt and without knowing much about
this, I'd tend to leave as is: apply the dcheck only on non empty string. I've
also left the DPLOG / DCHECK separate because I wasn't sure about using a 'P'
logging function when GetUserSidString might not generate a last error.

Please push back if you feel strongly about this.

Note that kasko is going away as soon as CP is able to get reports for other
processes
https://bugs.chromium.org/p/crashpad/issues/detail?id=98

+    }
+  }
+  // Ensure cleanup on scope exit.
+  base::ScopedClosureRunner free_sid(
+      base::Bind(&LoggedLocalFree, base::Unretained(sid)));
+
+  // Generate the message.
+  // Note that the format of this message must match the consumer in
+  // chrome/browser/crash_upload_list_win.cc.
+  base::string16 message =
+      L"Crash uploaded. Id=" + base::string16(report_id) + L".";
+
+  // Matches Omaha.
+  const int kCrashUploadEventId = 2;
+
+  // Report the event.
+  const base::char16* strings[] = {message.c_str()};
+  if (!::ReportEvent(event_source_handle, EVENTLOG_INFORMATION_TYPE,
+                     0,  // category
+                     kCrashUploadEventId, sid,

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

what if |sid| is null here?

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

Looks like it's optional. If it's missing the user's name won't be added to the
log, but other things will.

+                     1,  // count
+                     0, strings, nullptr)) {
+    DPLOG(ERROR);
+  }
+}
+
+void AddCrashKey(const wchar_t *key, const wchar_t *value,
+                 std::vector<kasko::api::CrashKey> *crash_keys) {
+  DCHECK(key);
+  DCHECK(value);
+  DCHECK(crash_keys);
+
+  kasko::api::CrashKey crash_key;
+  std::wcsncpy(crash_key.name, key, kasko::api::CrashKey::kNameMaxLength - 1);
+  std::wcsncpy(crash_key.value, value,
+               kasko::api::CrashKey::kValueMaxLength - 1);
+  crash_keys->push_back(crash_key);
+}
+
+}  // namespace
+
+bool InitializeKaskoReporter(const base::string16& endpoint,
+                             const base::char16* browser_data_directory) {
+  base::string16 crash_server;
+  GetKaskoCrashServerUrl(&crash_server);
+  base::FilePath crash_reports_base_dir;
+  GetKaskoCrashReportsBaseDir(browser_data_directory, &crash_reports_base_dir);
+
+  return kasko::api::InitializeReporter(
+      endpoint.c_str(),
+      crash_server.c_str(),
+      crash_reports_base_dir.Append(L"Crash Reports").value().c_str(),
+      crash_reports_base_dir.Append(kPermanentlyFailedReportsSubdir)
+          .value()
+          .c_str(),
+      &OnCrashReportUpload,
+      nullptr);
+}
+
+void ShutdownKaskoReporter() {
+  kasko::api::ShutdownReporter();
+}
+
+void DumpHungProcess(DWORD main_thread_id, const base::string16& channel,
+                     const base::char16* key, const base::Process& process) {
+  // Read the Crashpad module annotations for the process.
+  std::vector<kasko::api::CrashKey> annotations;
+  crash_reporter::ReadMainModuleAnnotationsForKasko(process, &annotations);
+  AddCrashKey(key, L"1", &annotations);
+
+  std::vector<const base::char16*> key_buffers;
+  std::vector<const base::char16*> value_buffers;
+  for (const auto& crash_key : annotations) {
+    key_buffers.push_back(crash_key.name);
+    value_buffers.push_back(crash_key.value);
+  }
+  key_buffers.push_back(nullptr);
+  value_buffers.push_back(nullptr);
+
+  // Synthesize an exception for the main thread. Populate the record with the
+  // current context of the thread to get the stack trace bucketed on the crash
+  // backend.
+  CONTEXT thread_context = {};
+  EXCEPTION_RECORD exception_record = {};
+  exception_record.ExceptionCode = EXCEPTION_ARRAY_BOUNDS_EXCEEDED;
+  EXCEPTION_POINTERS exception_pointers = {&exception_record, &thread_context};
+
+  base::win::ScopedHandle main_thread(::OpenThread(
+      THREAD_SUSPEND_RESUME | THREAD_GET_CONTEXT | THREAD_QUERY_INFORMATION,
+      FALSE, main_thread_id));
+
+  bool have_context = false;
+  if (main_thread.IsValid()) {
+    DWORD suspend_count = ::SuspendThread(main_thread.Get());
+    const DWORD kSuspendFailed = static_cast<DWORD>(-1);
+    if (suspend_count != kSuspendFailed) {
+      // Best effort capture of the context.
+      thread_context.ContextFlags = CONTEXT_FLOATING_POINT | CONTEXT_SEGMENTS |
+                                    CONTEXT_INTEGER | CONTEXT_CONTROL;
+      if (::GetThreadContext(main_thread.Get(), &thread_context) == TRUE)
+        have_context = true;
+
+      ::ResumeThread(main_thread.Get());
+    }
+  }
+
+  // TODO(erikwright): Make the dump-type channel-dependent.

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

erikwright won't likely be addressing this :)

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

Done.

+  if (have_context) {
+    kasko::api::SendReportForProcess(
+        process.Handle(), main_thread_id, &exception_pointers,
+        kasko::api::LARGER_DUMP_TYPE, key_buffers.data(), value_buffers.data());
+  } else {
+    kasko::api::SendReportForProcess(process.Handle(), 0, nullptr,
+                                     kasko::api::LARGER_DUMP_TYPE,
+                                     key_buffers.data(), value_buffers.data());
+  }
+}
+
+#endif  // BUILDFLAG(ENABLE_KASKO)