views: wireup widget name to crash data

And specifically set it when processing windows messages. As almost everything
starts from a windows message setting the id when processing a windows message
should help us better attribute which code is at fault.

BUG=966526
TEST=covered by test

Change-Id: I48851150fa55523cffb936d0f2f199ade04cac0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1626640
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663079}

ui/gfx/BUILD.gn

@@ -170,6 +170,8 @@ jumbo_component("gfx") {
     "utf16_indexing.h",
     "vsync_provider.cc",
     "vsync_provider.h",
+    "win/crash_id_helper.cc",
+    "win/crash_id_helper.h",
     "win/direct_write.cc",
     "win/direct_write.h",
     "win/hwnd_util.cc",
@@ -331,6 +333,7 @@ jumbo_component("gfx") {
       "setupapi.lib",
       "dwrite.lib",
     ]
+    deps += [ "//components/crash/core/common" ]
   } else {
     sources -= [
       "gdi_util.cc",
@@ -780,6 +783,7 @@ test("gfx_unittests") {
       "path_win_unittest.cc",
       "platform_font_win_unittest.cc",
       "system_fonts_win_unittest.cc",
+      "win/crash_id_helper_unittest.cc",
       "win/direct_write_unittest.cc",
       "win/text_analysis_source_unittest.cc",
     ]

ui/gfx/win/DEPS

@@ -0,0 +1,3 @@
+include_rules = [
+  "+components/crash",
+]

ui/gfx/win/crash_id_helper.cc

@@ -0,0 +1,67 @@
+// Copyright 2019 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 "ui/gfx/win/crash_id_helper.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_util.h"
+
+namespace gfx {
+
+// static
+CrashIdHelper* CrashIdHelper::Get() {
+  static base::NoDestructor<CrashIdHelper> helper;
+  return helper.get();
+}
+
+CrashIdHelper::ScopedLogger::~ScopedLogger() {
+  CrashIdHelper::Get()->OnDidProcessMessages();
+}
+
+CrashIdHelper::ScopedLogger::ScopedLogger() = default;
+
+std::unique_ptr<CrashIdHelper::ScopedLogger>
+CrashIdHelper::OnWillProcessMessages(const std::string& id) {
+  if (!ids_.empty())
+    was_nested_ = true;
+  ids_.push_back(id.empty() ? "unspecified" : id);
+  debugging_crash_key_.Set(CurrentCrashId());
+  // base::WrapUnique() as constructor is private.
+  return base::WrapUnique(new ScopedLogger);
+}
+
+void CrashIdHelper::OnDidProcessMessages() {
+  DCHECK(!ids_.empty());
+  ids_.pop_back();
+  if (ids_.empty()) {
+    debugging_crash_key_.Clear();
+    was_nested_ = false;
+  } else {
+    debugging_crash_key_.Set(CurrentCrashId());
+  }
+}
+
+CrashIdHelper::CrashIdHelper() = default;
+
+CrashIdHelper::~CrashIdHelper() = default;
+
+std::string CrashIdHelper::CurrentCrashId() const {
+  // This should only be called when there is at least one id.
+  DCHECK(!ids_.empty());
+  // Common case is only one id.
+  if (ids_.size() == 1) {
+    // If the size of |ids_| is 1, then the message loop is not nested. If
+    // |was_nested_| is true, it means in processing the message corresponding
+    // to ids_[0] another message was processed, resulting in nested message
+    // loops.  A nested message loop can lead to reentrancy and/or problems when
+    // the stack unravels. For example, it's entirely possible that when a
+    // nested message loop completes, objects further up in the stack have been
+    // deleted. "(N)" is added to signify that a nested message loop was run at
+    // some point during the current message loop.
+    return was_nested_ ? "(N) " + ids_[0] : ids_[0];
+  }
+  return base::JoinString(ids_, ">");
+}
+
+}  // namespace gfx

ui/gfx/win/crash_id_helper.h

@@ -0,0 +1,79 @@
+// Copyright 2019 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.
+
+#ifndef UI_GFX_WIN_CRASH_ID_HELPER_H_
+#define UI_GFX_WIN_CRASH_ID_HELPER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/no_destructor.h"
+#include "components/crash/core/common/crash_key.h"
+#include "ui/gfx/gfx_export.h"
+
+namespace gfx {
+
+// CrashIdHelper is used to log (in crash dumps) the id(s) of the window/widget
+// currently dispatching an event. Often times crashes occur purely in ui
+// code, while the bug lies in client code. Logging an id helps better identify
+// the client code that created the window/widget.
+//
+// Example usage:
+// {
+//   auto logger = CrashIdHelper::Get()->OnWillProcessMessages(crash_id);
+//   <do message processing>
+// }
+class GFX_EXPORT CrashIdHelper {
+ public:
+  static CrashIdHelper* Get();
+
+  // RAII style class that unregisters in the destructor.
+  class GFX_EXPORT ScopedLogger {
+   public:
+    ~ScopedLogger();
+
+   private:
+    friend class CrashIdHelper;
+    ScopedLogger();
+
+    DISALLOW_COPY_AND_ASSIGN(ScopedLogger);
+  };
+
+  // Adds |id| to the list of active debugging ids. When the returned object
+  // is destroyed, |id| is removed from the list of active debugging ids.
+  std::unique_ptr<ScopedLogger> OnWillProcessMessages(const std::string& id);
+
+ private:
+  friend base::NoDestructor<CrashIdHelper>;
+  friend class CrashIdHelperTest;
+
+  CrashIdHelper();
+  ~CrashIdHelper();
+
+  // Called from ~ScopedLogger. Removes the most recently added id.
+  void OnDidProcessMessages();
+
+  // Returns the identifier to put in the crash key.
+  std::string CurrentCrashId() const;
+
+  // Ordered list of debugging identifiers added.
+  std::vector<std::string> ids_;
+
+  // Set to true once |ids_| has more than one object, and false once |ids_| is
+  // empty. That is, this is true once processing the windows message resulted
+  // in processing another windows message (nested message loops). See comment
+  // in implementation of CurrentCrashId() as to why this is tracked.
+  bool was_nested_ = false;
+
+  // This uses the name 'widget' as this code is most commonly triggered from
+  // views, which uses the term Widget.
+  crash_reporter::CrashKeyString<128> debugging_crash_key_{"widget-id"};
+
+  DISALLOW_COPY_AND_ASSIGN(CrashIdHelper);
+};
+
+}  // namespace gfx
+
+#endif  // UI_GFX_WIN_CRASH_ID_HELPER_H_

ui/gfx/win/crash_id_helper_unittest.cc

@@ -0,0 +1,50 @@
+// Copyright 2019 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 "ui/gfx/win/crash_id_helper.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace gfx {
+
+class CrashIdHelperTest : public testing::Test {
+ public:
+  CrashIdHelperTest() = default;
+  ~CrashIdHelperTest() override = default;
+
+  std::string CurrentCrashId() {
+    return CrashIdHelper::Get()->CurrentCrashId();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CrashIdHelperTest);
+};
+
+// This test verifies CurrentCrashId(). Ideally this would verify at
+// crash_reporter::CrashKeyString, but that class isn't particularly test
+// friendly (and the implementation varies depending upon compile time flags).
+TEST_F(CrashIdHelperTest, Basic) {
+  const std::string id1 = "id";
+  {
+    auto scoper = CrashIdHelper::Get()->OnWillProcessMessages(id1);
+    EXPECT_EQ(id1, CurrentCrashId());
+  }
+
+  // No assertions for empty as CurrentCrashId() DCHECKs there is at least
+  // one id.
+
+  const std::string id2 = "id2";
+  {
+    auto scoper = CrashIdHelper::Get()->OnWillProcessMessages(id2);
+    EXPECT_EQ(id2, CurrentCrashId());
+
+    {
+      auto scoper2 = CrashIdHelper::Get()->OnWillProcessMessages(id1);
+      EXPECT_EQ("id2>id", CurrentCrashId());
+    }
+    EXPECT_EQ("(N) id2", CurrentCrashId());
+  }
+}
+
+}  // namespace gfx

ui/gfx/win/window_impl.cc

@@ -14,14 +14,14 @@
 #include "base/synchronization/lock.h"
 #include "base/win/win_util.h"
 #include "base/win/wrapped_window_proc.h"
+#include "ui/gfx/win/crash_id_helper.h"
 #include "ui/gfx/win/hwnd_util.h"
 
 namespace gfx {
 
 static const DWORD kWindowDefaultChildStyle =
     WS_CHILD | WS_VISIBLE | WS_CLIPCHILDREN | WS_CLIPSIBLINGS;
 static const DWORD kWindowDefaultStyle = WS_OVERLAPPEDWINDOW | WS_CLIPCHILDREN;
-static const DWORD kWindowDefaultExStyle = 0;
 
 ///////////////////////////////////////////////////////////////////////////////
 // WindowImpl class tracking.
@@ -33,10 +33,6 @@ const wchar_t* const WindowImpl::kBaseClassName = L"Chrome_WidgetWin_";
 
 // WindowImpl class information used for registering unique windows.
 struct ClassInfo {
-  UINT style;
-  HICON icon;
-  HICON small_icon;
-
   ClassInfo(int style, HICON icon, HICON small_icon)
       : style(style), icon(icon), small_icon(small_icon) {}
 
@@ -45,6 +41,10 @@ struct ClassInfo {
     return (other.style == style && other.icon == icon &&
             other.small_icon == small_icon);
   }
+
+  UINT style;
+  HICON icon;
+  HICON small_icon;
 };
 
 // WARNING: this class may be used on multiple threads.
@@ -158,15 +158,8 @@ ClassRegistrar::ClassRegistrar() : registered_count_(0) {}
 ///////////////////////////////////////////////////////////////////////////////
 // WindowImpl, public
 
-WindowImpl::WindowImpl()
-    : window_style_(0),
-      window_ex_style_(kWindowDefaultExStyle),
-      class_style_(CS_DBLCLKS),
-      hwnd_(NULL),
-      got_create_(false),
-      got_valid_hwnd_(false),
-      destroyed_(NULL) {
-}
+WindowImpl::WindowImpl(const std::string& debugging_id)
+    : debugging_id_(debugging_id), class_style_(CS_DBLCLKS) {}
 
 WindowImpl::~WindowImpl() {
   if (destroyed_)
@@ -263,7 +256,7 @@ LRESULT WindowImpl::OnWndProc(UINT message, WPARAM w_param, LPARAM l_param) {
 
   HWND hwnd = hwnd_;
   if (message == WM_NCDESTROY)
-    hwnd_ = NULL;
+    hwnd_ = nullptr;
 
   // Handle the message if it's in our message map; otherwise, let the system
   // handle it.
@@ -275,7 +268,7 @@ LRESULT WindowImpl::OnWndProc(UINT message, WPARAM w_param, LPARAM l_param) {
 
 void WindowImpl::ClearUserData() {
   if (::IsWindow(hwnd_))
-    gfx::SetWindowUserData(hwnd_, NULL);
+    gfx::SetWindowUserData(hwnd_, nullptr);
 }
 
 // static
@@ -300,6 +293,8 @@ LRESULT CALLBACK WindowImpl::WndProc(HWND hwnd,
   if (!window)
     return 0;
 
+  auto logger =
+      CrashIdHelper::Get()->OnWillProcessMessages(window->debugging_id_);
   return window->OnWndProc(message, w_param, l_param);
 }
 

ui/gfx/win/window_impl.h

@@ -39,7 +39,9 @@ class MessageMapInterface {
 ///////////////////////////////////////////////////////////////////////////////
 class GFX_EXPORT WindowImpl : public MessageMapInterface {
  public:
-  WindowImpl();
+  // |debugging_id| is reported with crashes to help attribute the code that
+  // created the WindowImpl.
+  explicit WindowImpl(const std::string& debugging_id = std::string());
   virtual ~WindowImpl();
 
   // Causes all generated windows classes to be unregistered at exit.
@@ -74,6 +76,8 @@ class GFX_EXPORT WindowImpl : public MessageMapInterface {
   }
   UINT initial_class_style() const { return class_style_; }
 
+  const std::string& debugging_id() const { return debugging_id_; }
+
  protected:
   // Handles the WndProc callback for this object.
   virtual LRESULT OnWndProc(UINT message, WPARAM w_param, LPARAM l_param);
@@ -100,23 +104,25 @@ class GFX_EXPORT WindowImpl : public MessageMapInterface {
   // All classes registered by WindowImpl start with this name.
   static const wchar_t* const kBaseClassName;
 
+  const std::string debugging_id_;
+
   // Window Styles used when creating the window.
-  DWORD window_style_;
+  DWORD window_style_ = 0;
 
   // Window Extended Styles used when creating the window.
-  DWORD window_ex_style_;
+  DWORD window_ex_style_ = 0;
 
   // Style of the class to use.
   UINT class_style_;
 
   // Our hwnd.
-  HWND hwnd_;
+  HWND hwnd_ = nullptr;
 
   // For debugging.
   // TODO(sky): nuke this when get crash data.
-  bool got_create_;
-  bool got_valid_hwnd_;
-  bool* destroyed_;
+  bool got_create_ = false;
+  bool got_valid_hwnd_ = false;
+  bool* destroyed_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(WindowImpl);
 };

ui/views/test/desktop_window_tree_host_win_test_api.cc

@@ -3,11 +3,11 @@
 // found in the LICENSE file.
 
 #include "ui/views/test/desktop_window_tree_host_win_test_api.h"
+
 #include "ui/views/widget/desktop_aura/desktop_window_tree_host_win.h"
 #include "ui/views/win/hwnd_message_handler.h"
 
 namespace views {
-
 namespace test {
 
 DesktopWindowTreeHostWinTestApi::DesktopWindowTreeHostWinTestApi(
@@ -27,6 +27,9 @@ DesktopWindowTreeHostWinTestApi::GetNativeViewAccessible() {
   return host_->GetNativeViewAccessible();
 }
 
-}  // namespace test
+HWNDMessageHandler* DesktopWindowTreeHostWinTestApi::GetHwndMessageHandler() {
+  return host_->message_handler_.get();
+}
 
+}  // namespace test
 }  // namespace views