Avoid redefining IsMsgHandled by making all Win message handlers safe

There were two sets of message map macros, one with IsMsgHandled as a
function, one with it as a macro. If combined in some translation unit
that could cause compilation problems, and it did in some Opera builds.

This patch changes the code so that there is only one implementation
of the macros and it's the "safe" one, where "safe" means that it
can handle that the underlying object is deleted inside a message
handler without crashing.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I2f039d8d93a539c8322caa664e642956c5d6d372
Reviewed-on: https://chromium-review.googlesource.com/995896Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550711}

gpu/ipc/service/child_window_win.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/thread.h"
 #include "base/win/scoped_hdc.h"
@@ -114,6 +115,8 @@ class HiddenPopupWindow : public gfx::WindowImpl {
   CR_BEGIN_MSG_MAP_EX(HiddenPopupWindow)
     CR_MSG_WM_CLOSE(OnClose)
   CR_END_MSG_MAP()
+
+  CR_MSG_MAP_CLASS_DECLARATIONS(HiddenPopupWindow)
 };
 
 // This runs on the window owner thread.

ui/base/win/foreground_helper.cc

@@ -9,6 +9,9 @@
 
 namespace ui {
 
+ForegroundHelper::ForegroundHelper() : window_(NULL) {}
+ForegroundHelper::~ForegroundHelper() = default;
+
 // static
 HRESULT ForegroundHelper::SetForeground(HWND window) {
   DCHECK(::IsWindow(window));

ui/base/win/foreground_helper.h

@@ -23,7 +23,8 @@ namespace ui {
 // This is probably leveraging a windows bug.
 class UI_BASE_EXPORT ForegroundHelper : public gfx::WindowImpl {
  public:
-  ForegroundHelper() : window_(NULL) { }
+  ForegroundHelper();
+  ~ForegroundHelper() override;
 
   CR_BEGIN_MSG_MAP_EX(ForegroundHelper)
     CR_MSG_WM_HOTKEY(OnHotKey)
@@ -42,6 +43,8 @@ class UI_BASE_EXPORT ForegroundHelper : public gfx::WindowImpl {
 
   HWND window_;
 
+  CR_MSG_MAP_CLASS_DECLARATIONS(ForegroundHelper)
+
   DISALLOW_COPY_AND_ASSIGN(ForegroundHelper);
 };
 

ui/base/win/hidden_window.cc

@@ -42,9 +42,11 @@ class TempParent : public gfx::WindowImpl {
   void OnClose() {
   }
 
-  CR_BEGIN_MSG_MAP_EX(WebContentsViewWin)
+  CR_BEGIN_MSG_MAP_EX(TempParent)
     CR_MSG_WM_CLOSE(OnClose)
   CR_END_MSG_MAP()
+
+  CR_MSG_MAP_CLASS_DECLARATIONS(TempParent)
 };
 
 }  // namespace

ui/compositor/test/test_compositor_host_win.cc

@@ -59,6 +59,8 @@ class TestCompositorHostWin : public TestCompositorHost,
 
   std::unique_ptr<ui::Compositor> compositor_;
 
+  CR_MSG_MAP_CLASS_DECLARATIONS(TestCompositorHostWin)
+
   DISALLOW_COPY_AND_ASSIGN(TestCompositorHostWin);
 };
 

ui/platform_window/win/win_window.cc

@@ -41,8 +41,7 @@ gfx::Rect GetWindowBoundsForClientBounds(DWORD style, DWORD ex_style,
 
 }  // namespace
 
-WinWindow::WinWindow(PlatformWindowDelegate* delegate,
-                     const gfx::Rect& bounds)
+WinWindow::WinWindow(PlatformWindowDelegate* delegate, const gfx::Rect& bounds)
     : delegate_(delegate) {
   CHECK(delegate_);
   DWORD window_style = WS_OVERLAPPEDWINDOW;

ui/platform_window/win/win_window.h

@@ -7,6 +7,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "ui/gfx/win/window_impl.h"
 #include "ui/platform_window/platform_window.h"
 #include "ui/platform_window/win/win_window_export.h"
@@ -48,8 +49,7 @@ class WIN_WINDOW_EXPORT WinWindow : public PlatformWindow,
 
   CR_BEGIN_MSG_MAP_EX(WinWindow)
     CR_MESSAGE_RANGE_HANDLER_EX(WM_MOUSEFIRST, WM_MOUSELAST, OnMouseRange)
-    CR_MESSAGE_RANGE_HANDLER_EX(WM_NCMOUSEMOVE,
-                                WM_NCXBUTTONDBLCLK,
+    CR_MESSAGE_RANGE_HANDLER_EX(WM_NCMOUSEMOVE, WM_NCXBUTTONDBLCLK,
                                 OnMouseRange)
     CR_MESSAGE_HANDLER_EX(WM_CAPTURECHANGED, OnCaptureChanged)
 
@@ -81,6 +81,8 @@ class WIN_WINDOW_EXPORT WinWindow : public PlatformWindow,
 
   PlatformWindowDelegate* delegate_;
 
+  CR_MSG_MAP_CLASS_DECLARATIONS(WinWindow)
+
   DISALLOW_COPY_AND_ASSIGN(WinWindow);
 };
 

ui/views/win/hwnd_message_handler.cc

@@ -339,8 +339,7 @@ base::LazyInstance<HWNDMessageHandler::FullscreenWindowMonitorMap>::
 long HWNDMessageHandler::last_touch_or_pen_message_time_ = 0;
 
 HWNDMessageHandler::HWNDMessageHandler(HWNDMessageHandlerDelegate* delegate)
-    : msg_handled_(FALSE),
-      delegate_(delegate),
+    : delegate_(delegate),
       fullscreen_handler_(new FullscreenHandler),
       waiting_for_close_now_(false),
       use_system_default_icon_(false),
@@ -370,8 +369,7 @@ HWNDMessageHandler::HWNDMessageHandler(HWNDMessageHandlerDelegate* delegate)
       pointer_events_for_touch_(features::IsUsingWMPointerForTouch()),
       precision_touchpad_scroll_phase_enabled_(base::FeatureList::IsEnabled(
           features::kPrecisionTouchpadScrollPhase)),
-      autohide_factory_(this),
-      weak_factory_(this) {}
+      autohide_factory_(this) {}
 
 HWNDMessageHandler::~HWNDMessageHandler() {
   DCHECK(delegate_->GetHWNDMessageDelegateInputMethod());
@@ -451,7 +449,7 @@ void HWNDMessageHandler::Close() {
     waiting_for_close_now_ = true;
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(&HWNDMessageHandler::CloseNow,
-                                  weak_factory_.GetWeakPtr()));
+                                  msg_handler_weak_factory_.GetWeakPtr()));
   }
 }
 
@@ -927,7 +925,7 @@ LRESULT HWNDMessageHandler::OnWndProc(UINT message,
   // NOTE: We inline ProcessWindowMessage() as 'this' may be destroyed during
   // dispatch and ProcessWindowMessage() doesn't deal with that well.
   const BOOL old_msg_handled = msg_handled_;
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   const BOOL processed =
       _ProcessWindowMessage(window, message, w_param, l_param, result, 0);
   if (!ref)
@@ -992,9 +990,9 @@ LRESULT HWNDMessageHandler::HandleMouseMessage(unsigned int message,
                                                bool* handled) {
   // Don't track forwarded mouse messages. We expect the caller to track the
   // mouse.
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   LRESULT ret = HandleMouseEventInternal(message, w_param, l_param, false);
-  *handled = IsMsgHandled();
+  *handled = !ref.get() || msg_handled_;
   return ret;
 }
 
@@ -1002,13 +1000,13 @@ LRESULT HWNDMessageHandler::HandleKeyboardMessage(unsigned int message,
                                                   WPARAM w_param,
                                                   LPARAM l_param,
                                                   bool* handled) {
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   LRESULT ret = 0;
   if ((message == WM_CHAR) || (message == WM_SYSCHAR))
     ret = OnImeMessages(message, w_param, l_param);
   else
     ret = OnKeyEvent(message, w_param, l_param);
-  *handled = IsMsgHandled();
+  *handled = !ref.get() || msg_handled_;
   return ret;
 }
 
@@ -1016,9 +1014,9 @@ LRESULT HWNDMessageHandler::HandleTouchMessage(unsigned int message,
                                                WPARAM w_param,
                                                LPARAM l_param,
                                                bool* handled) {
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   LRESULT ret = OnTouchEvent(message, w_param, l_param);
-  *handled = IsMsgHandled();
+  *handled = !ref.get() || msg_handled_;
   return ret;
 }
 
@@ -1026,9 +1024,9 @@ LRESULT HWNDMessageHandler::HandlePointerMessage(unsigned int message,
                                                  WPARAM w_param,
                                                  LPARAM l_param,
                                                  bool* handled) {
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   LRESULT ret = OnPointerEvent(message, w_param, l_param);
-  *handled = IsMsgHandled();
+  *handled = !ref.get() || msg_handled_;
   return ret;
 }
 
@@ -1036,9 +1034,9 @@ LRESULT HWNDMessageHandler::HandleScrollMessage(unsigned int message,
                                                 WPARAM w_param,
                                                 LPARAM l_param,
                                                 bool* handled) {
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   LRESULT ret = OnScrollMessage(message, w_param, l_param);
-  *handled = IsMsgHandled();
+  *handled = !ref.get() || msg_handled_;
   return ret;
 }
 
@@ -1046,10 +1044,10 @@ LRESULT HWNDMessageHandler::HandleNcHitTestMessage(unsigned int message,
                                                    WPARAM w_param,
                                                    LPARAM l_param,
                                                    bool* handled) {
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   LRESULT ret = OnNCHitTest(
       gfx::Point(CR_GET_X_LPARAM(l_param), CR_GET_Y_LPARAM(l_param)));
-  *handled = IsMsgHandled();
+  *handled = !ref.get() || msg_handled_;
   return ret;
 }
 
@@ -1410,7 +1408,7 @@ LRESULT HWNDMessageHandler::DefWindowProcWithRedrawLock(UINT message,
   ScopedRedrawLock lock(this);
   // The Widget and HWND can be destroyed in the call to DefWindowProc, so use
   // the WeakPtrFactory to avoid unlocking (and crashing) after destruction.
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   LRESULT result = DefWindowProc(hwnd(), message, w_param, l_param);
   if (!ref)
     lock.CancelUnlockOperation();
@@ -1441,7 +1439,7 @@ void HWNDMessageHandler::ForceRedrawWindow(int attempts) {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&HWNDMessageHandler::ForceRedrawWindow,
-                       weak_factory_.GetWeakPtr(), attempts),
+                       msg_handler_weak_factory_.GetWeakPtr(), attempts),
         base::TimeDelta::FromMilliseconds(500));
     return;
   }
@@ -1708,7 +1706,7 @@ LRESULT HWNDMessageHandler::OnImeMessages(UINT message,
                                           WPARAM w_param,
                                           LPARAM l_param) {
   LRESULT result = 0;
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   const bool msg_handled =
       delegate_->HandleIMEMessage(message, w_param, l_param, &result);
   if (ref.get())
@@ -1749,7 +1747,7 @@ LRESULT HWNDMessageHandler::OnKeyEvent(UINT message,
   MSG msg = {
       hwnd(), message, w_param, l_param, static_cast<DWORD>(GetMessageTime())};
   ui::KeyEvent key(msg);
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   delegate_->HandleKeyEvent(&key);
   if (!ref)
     return 0;
@@ -2376,7 +2374,8 @@ void HWNDMessageHandler::OnSysCommand(UINT notification_code,
     // with the mouse/touch/keyboard, we flag as being in a size loop.
     if ((notification_code & sc_mask) == SC_SIZE)
       in_size_loop_ = true;
-    base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+    base::WeakPtr<HWNDMessageHandler> ref(
+        msg_handler_weak_factory_.GetWeakPtr());
 
     DefWindowProc(hwnd(), WM_SYSCOMMAND, notification_code,
                   MAKELPARAM(point.x(), point.y()));
@@ -2455,7 +2454,7 @@ LRESULT HWNDMessageHandler::OnTouchEvent(UINT message,
         base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
             FROM_HERE,
             base::BindOnce(&HWNDMessageHandler::ResetTouchDownContext,
-                           weak_factory_.GetWeakPtr()),
+                           msg_handler_weak_factory_.GetWeakPtr()),
             base::TimeDelta::FromMilliseconds(kTouchDownContextResetTimeout));
       } else {
         if (input[i].dwFlags & TOUCHEVENTF_MOVE) {
@@ -2488,8 +2487,9 @@ LRESULT HWNDMessageHandler::OnTouchEvent(UINT message,
     // events on windows don't fire if we enter a modal loop in the context of
     // a touch event.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&HWNDMessageHandler::HandleTouchEvents,
-                                  weak_factory_.GetWeakPtr(), touch_events));
+        FROM_HERE,
+        base::BindOnce(&HWNDMessageHandler::HandleTouchEvents,
+                       msg_handler_weak_factory_.GetWeakPtr(), touch_events));
   }
   CloseTouchInputHandle(reinterpret_cast<HTOUCHINPUT>(l_param));
   SetMsgHandled(FALSE);
@@ -2596,7 +2596,7 @@ void HWNDMessageHandler::OnWindowPosChanging(WINDOWPOS* window_pos) {
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE,
             base::BindOnce(&HWNDMessageHandler::StopIgnoringPosChanges,
-                           weak_factory_.GetWeakPtr()));
+                           msg_handler_weak_factory_.GetWeakPtr()));
       }
       last_monitor_ = monitor;
       last_monitor_rect_ = monitor_rect;
@@ -2678,7 +2678,7 @@ void HWNDMessageHandler::OnSessionChange(WPARAM status_code) {
 }
 
 void HWNDMessageHandler::HandleTouchEvents(const TouchEvents& touch_events) {
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   for (size_t i = 0; i < touch_events.size() && ref; ++i) {
     ui::TouchEvent* touch_event = const_cast<ui::TouchEvent*>(&touch_events[i]);
     delegate_->HandleTouchEvent(touch_event);
@@ -2802,7 +2802,7 @@ LRESULT HWNDMessageHandler::HandleMouseEventInternal(UINT message,
 
   // There are cases where the code handling the message destroys the window,
   // so use the weak ptr to check if destruction occured or not.
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   bool handled = delegate_->HandleMouseEvent(&event);
 
   if (!ref.get())
@@ -2861,7 +2861,7 @@ LRESULT HWNDMessageHandler::HandlePointerEventTypeTouch(UINT message,
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&HWNDMessageHandler::ResetTouchDownContext,
-                       weak_factory_.GetWeakPtr()),
+                       msg_handler_weak_factory_.GetWeakPtr()),
         base::TimeDelta::FromMilliseconds(kTouchDownContextResetTimeout));
   }
 
@@ -2901,7 +2901,7 @@ LRESULT HWNDMessageHandler::HandlePointerEventTypeTouch(UINT message,
 
   // There are cases where the code handling the message destroys the
   // window, so use the weak ptr to check if destruction occurred or not.
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   delegate_->HandleTouchEvent(&event);
 
   if (event_type == ui::ET_TOUCH_RELEASED)
@@ -2935,7 +2935,7 @@ LRESULT HWNDMessageHandler::HandlePointerEventTypePen(UINT message,
 
   // There are cases where the code handling the message destroys the
   // window, so use the weak ptr to check if destruction occured or not.
-  base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr());
+  base::WeakPtr<HWNDMessageHandler> ref(msg_handler_weak_factory_.GetWeakPtr());
   if (event) {
     if (event->IsTouchEvent()) {
       delegate_->HandleTouchEvent(event->AsTouchEvent());

ui/views/win/hwnd_message_handler.h

@@ -27,6 +27,7 @@
 #include "ui/events/event.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/sequential_id_generator.h"
+#include "ui/gfx/win/msg_util.h"
 #include "ui/gfx/win/window_impl.h"
 #include "ui/views/views_export.h"
 #include "ui/views/win/pen_event_processor.h"
@@ -65,54 +66,6 @@ const int WM_NCUAHDRAWFRAME = 0xAF;
 // WM_WINDOWPOSCHANGED won't be received.
 const int WM_WINDOWSIZINGFINISHED = WM_USER;
 
-// IsMsgHandled() and BEGIN_SAFE_MSG_MAP_EX are a modified version of
-// BEGIN_MSG_MAP_EX. The main difference is it uses a WeakPtrFactory member
-// (|weak_factory|) that is used in _ProcessWindowMessage() and changing
-// IsMsgHandled() from a member function to a define that checks if the weak
-// factory is still valid in addition to the member. Together these allow for
-// |this| to be deleted during dispatch.
-#define IsMsgHandled() !ref.get() || msg_handled_
-
-#define BEGIN_SAFE_MSG_MAP_EX(weak_factory) \
- private: \
-  BOOL msg_handled_; \
-\
- public: \
-  /* "handled" management for cracked handlers */ \
-  void SetMsgHandled(BOOL handled) { \
-    msg_handled_ = handled; \
-  } \
-  BOOL ProcessWindowMessage(HWND hwnd, \
-                            UINT msg, \
-                            WPARAM w_param, \
-                            LPARAM l_param, \
-                            LRESULT& l_result, \
-                            DWORD msg_map_id = 0) override { \
-    base::WeakPtr<HWNDMessageHandler> ref(weak_factory.GetWeakPtr()); \
-    BOOL old_msg_handled = msg_handled_; \
-    BOOL ret = _ProcessWindowMessage(hwnd, msg, w_param, l_param, l_result, \
-                                     msg_map_id); \
-    if (ref.get()) \
-      msg_handled_ = old_msg_handled; \
-    return ret; \
-  } \
-  BOOL _ProcessWindowMessage(HWND hWnd, \
-                             UINT uMsg, \
-                             WPARAM wParam, \
-                             LPARAM lParam, \
-                             LRESULT& lResult, \
-                             DWORD dwMsgMapID) { \
-    base::WeakPtr<HWNDMessageHandler> ref(weak_factory.GetWeakPtr()); \
-    BOOL bHandled = TRUE; \
-    hWnd; \
-    uMsg; \
-    wParam; \
-    lParam; \
-    lResult; \
-    bHandled; \
-    switch(dwMsgMapID) { \
-      case 0:
-
 // An object that handles messages for a HWND that implements the views
 // "Custom Frame" look. The purpose of this class is to isolate the windows-
 // specific message handling from the code that wraps it. It is intended to be
@@ -360,11 +313,10 @@ class VIEWS_EXPORT HWNDMessageHandler : public gfx::WindowImpl,
 
   // Message Handlers ----------------------------------------------------------
 
-  BEGIN_SAFE_MSG_MAP_EX(weak_factory_)
+  CR_BEGIN_MSG_MAP_EX(HWNDMessageHandler)
     // Range handlers must go first!
     CR_MESSAGE_RANGE_HANDLER_EX(WM_MOUSEFIRST, WM_MOUSELAST, OnMouseRange)
-    CR_MESSAGE_RANGE_HANDLER_EX(WM_NCMOUSEMOVE,
-                                WM_NCXBUTTONDBLCLK,
+    CR_MESSAGE_RANGE_HANDLER_EX(WM_NCMOUSEMOVE, WM_NCXBUTTONDBLCLK,
                                 OnMouseRange)
 
     // CustomFrameWindow hacks
@@ -771,15 +723,15 @@ class VIEWS_EXPORT HWNDMessageHandler : public gfx::WindowImpl,
   static base::LazyInstance<FullscreenWindowMonitorMap>::DestructorAtExit
       fullscreen_monitor_map_;
 
-  // The WeakPtrFactories below must occur last in the class definition so they
-  // get destroyed last.
+  // The WeakPtrFactories below (one inside the
+  // CR_MSG_MAP_CLASS_DECLARATIONS macro and autohide_factory_) must
+  // occur last in the class definition so they get destroyed last.
+
+  CR_MSG_MAP_CLASS_DECLARATIONS(HWNDMessageHandler)
 
   // The factory used to lookup appbar autohide edges.
   base::WeakPtrFactory<HWNDMessageHandler> autohide_factory_;
 
-  // The factory used with BEGIN_SAFE_MSG_MAP_EX.
-  base::WeakPtrFactory<HWNDMessageHandler> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(HWNDMessageHandler);
 };