Bug 1369419 GetMessage() and PeekMessage() shouldn't be used directly as far as possible r?jimm draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 02 Jun 2017 12:02:35 +0900
changeset 588191 ba07335f2d4fe9835441a54276cb4a3d81030cb1
parent 587037 e85da080583ef7fa858949ff5f3cb472afa8b1a7
child 631505 b75bb6fee721a5eb038290225c42df1b40d72a1b
push id61960
push usermasayuki@d-toybox.com
push dateFri, 02 Jun 2017 11:02:43 +0000
reviewersjimm
bugs1369419
milestone55.0a1
Bug 1369419 GetMessage() and PeekMessage() shouldn't be used directly as far as possible r?jimm In TSF mode, application should retrieve messages with ITfMessagePump::GetMessage() or ITfMessagePump::PeekMessage() since TSF/TIP may handle the message before or after the host application handles it. This patch rewrites the API users with WinUtils::(Get|Peek)Message() which use ITfMessagePump if it's available. MozReview-Commit-ID: LwHIgp7SxLH
dom/gamepad/windows/WindowsGamepad.cpp
ipc/glue/WindowsMessageLoop.cpp
mozglue/misc/StackWalk.cpp
widget/windows/WinUtils.cpp
widget/windows/WinUtils.h
widget/windows/nsWindow.cpp
xpcom/threads/HangMonitor.cpp
--- a/dom/gamepad/windows/WindowsGamepad.cpp
+++ b/dom/gamepad/windows/WindowsGamepad.cpp
@@ -14,16 +14,17 @@
 #include <hidsdi.h>
 #include <stdio.h>
 #include <xinput.h>
 
 #include "nsIComponentManager.h"
 #include "nsITimer.h"
 #include "nsTArray.h"
 #include "nsThreadUtils.h"
+#include "WinUtils.h"
 
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/Services.h"
 
 #include "mozilla/ipc/BackgroundParent.h"
 #include "mozilla/dom/GamepadPlatformService.h"
 
 namespace {
@@ -331,17 +332,17 @@ private:
 
 HWND sHWnd = nullptr;
 
 static void
 DirectInputMessageLoopOnceCallback(nsITimer *aTimer, void* aClosure)
 {
   MOZ_ASSERT(NS_GetCurrentThread() == gMonitorThread);
   MSG msg;
-  while (PeekMessageW(&msg, sHWnd, 0, 0, PM_REMOVE) > 0) {
+  while (widget::WinUtils::PeekMessage(&msg, sHWnd, 0, 0, PM_REMOVE) > 0) {
     TranslateMessage(&msg);
     DispatchMessage(&msg);
   }
   aTimer->Cancel();
   if (!sIsShutdown) {
     aTimer->InitWithFuncCallback(DirectInputMessageLoopOnceCallback,
                                  nullptr, kWindowsGamepadPollInterval,
                                  nsITimer::TYPE_ONE_SHOT);
--- a/ipc/glue/WindowsMessageLoop.cpp
+++ b/ipc/glue/WindowsMessageLoop.cpp
@@ -18,16 +18,17 @@
 
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/ipc/ProtocolUtils.h"
 #include "mozilla/PaintTracker.h"
 
 using namespace mozilla;
 using namespace mozilla::ipc;
 using namespace mozilla::ipc::windows;
+using namespace mozilla::widget;
 
 /**
  * The Windows-only code below exists to solve a general problem with deadlocks
  * that we experience when sending synchronous IPC messages to processes that
  * contain native windows (i.e. HWNDs). Windows (the OS) sends synchronous
  * messages between parent and child HWNDs in multiple circumstances (e.g.
  * WM_PARENTNOTIFY, WM_NCACTIVATE, etc.), even when those HWNDs are controlled
  * by different threads or different processes. Thus we can very easily end up
@@ -814,17 +815,17 @@ MessageChannel::SpinInternalEventLoop()
     {
       MonitorAutoLock lock(*mMonitor);
       if (!Connected()) {
         return;
       }
     }
 
     // Retrieve window or thread messages
-    if (PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE)) {
+    if (WinUtils::PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
       // The child UI should have been destroyed before the app is closed, in
       // which case, we should never get this here.
       if (msg.message == WM_QUIT) {
           NS_ERROR("WM_QUIT received in SpinInternalEventLoop!");
       } else {
           TranslateMessage(&msg);
           ::DispatchMessageW(&msg);
           return;
@@ -904,22 +905,22 @@ NeuteredWindowRegion::PumpOnce()
 {
   if (!gWindowHook) {
     // This should be a no-op if nothing has been neutered.
     return;
   }
 
   MSG msg = {0};
   // Pump any COM messages so that we don't hang due to STA marshaling.
-  if (gCOMWindow && ::PeekMessageW(&msg, gCOMWindow, 0, 0, PM_REMOVE)) {
+  if (gCOMWindow && WinUtils::PeekMessage(&msg, gCOMWindow, 0, 0, PM_REMOVE)) {
       ::TranslateMessage(&msg);
       ::DispatchMessageW(&msg);
   }
   // Expunge any nonqueued messages on the current thread.
-  ::PeekMessageW(&msg, nullptr, 0, 0, PM_NOREMOVE);
+  WinUtils::PeekMessage(&msg, nullptr, 0, 0, PM_NOREMOVE);
 }
 
 DeneuteredWindowRegion::DeneuteredWindowRegion(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL)
   : mReneuter(gWindowHook != NULL)
 {
   MOZ_GUARD_OBJECT_NOTIFIER_INIT;
   if (mReneuter) {
     StopNeutering();
@@ -1130,28 +1131,28 @@ MessageChannel::WaitForSyncNotify(bool a
       // "nonqueued" messages that are pending before returning. If we have
       // "nonqueued" messages pending then we should have switched out all the
       // window procedures above. In that case this PeekMessage call won't
       // actually cause any mozilla code (or plugin code) to run.
 
       // We have to manually pump all COM messages *after* looking at the queue
       // queue status but before yielding our thread below.
       if (gCOMWindow) {
-        if (PeekMessageW(&msg, gCOMWindow, 0, 0, PM_REMOVE)) {
+        if (WinUtils::PeekMessage(&msg, gCOMWindow, 0, 0, PM_REMOVE)) {
           TranslateMessage(&msg);
           ::DispatchMessageW(&msg);
         }
       }
 
       // If the following PeekMessage call fails to return a message for us (and
       // returns false) and we didn't run any "nonqueued" messages then we must
       // have woken up for a message designated for a window in another thread.
       // If we loop immediately then we could enter a tight loop, so we'll give
       // up our time slice here to let the child process its message.
-      if (!PeekMessageW(&msg, nullptr, 0, 0, PM_NOREMOVE) &&
+      if (!WinUtils::PeekMessage(&msg, nullptr, 0, 0, PM_NOREMOVE) &&
           !haveSentMessagesPending) {
         // Message was for child, we should wait a bit.
         SwitchToThread();
       }
     }
   }
 
   if (timerId) {
@@ -1257,25 +1258,25 @@ MessageChannel::WaitForInterruptNotify()
     }
 
     // See MessageChannel's WaitFor*Notify for details.
     bool haveSentMessagesPending =
       (HIWORD(GetQueueStatus(QS_SENDMESSAGE)) & QS_SENDMESSAGE) != 0;
 
     // Run all COM messages *after* looking at the queue status.
     if (gCOMWindow) {
-        if (PeekMessageW(&msg, gCOMWindow, 0, 0, PM_REMOVE)) {
+        if (WinUtils::PeekMessage(&msg, gCOMWindow, 0, 0, PM_REMOVE)) {
             TranslateMessage(&msg);
             ::DispatchMessageW(&msg);
         }
     }
 
     // PeekMessage markes the messages as "old" so that they don't wake up
     // MsgWaitForMultipleObjects every time.
-    if (!PeekMessageW(&msg, nullptr, 0, 0, PM_NOREMOVE) &&
+    if (!WinUtils::PeekMessage(&msg, nullptr, 0, 0, PM_NOREMOVE) &&
         !haveSentMessagesPending) {
       // Message was for child, we should wait a bit.
       SwitchToThread();
     }
   }
 
   if (timerId) {
     KillTimer(nullptr, timerId);
--- a/mozglue/misc/StackWalk.cpp
+++ b/mozglue/misc/StackWalk.cpp
@@ -604,16 +604,19 @@ WalkStackMain64(struct WalkStackData* aD
 }
 
 static unsigned int WINAPI
 WalkStackThread(void* aData)
 {
   BOOL msgRet;
   MSG msg;
 
+  // XXX This is external linkage code.  Therefore, we cannot use WinUtils.
+  //     So, calls ::PeekMessage() and ::GetMessage() directly here.
+
   // Call PeekMessage to force creation of a message queue so that
   // other threads can safely post events to us.
   ::PeekMessage(&msg, nullptr, WM_USER, WM_USER, PM_NOREMOVE);
 
   // and tell the thread that created us that we're ready.
   HANDLE readyEvent = (HANDLE)aData;
   ::SetEvent(readyEvent);
 
--- a/widget/windows/WinUtils.cpp
+++ b/widget/windows/WinUtils.cpp
@@ -679,50 +679,54 @@ static Atomic<bool> sAPCPending;
 void
 WinUtils::SetAPCPending()
 {
   sAPCPending = true;
 }
 #endif // ACCESSIBILITY
 
 /* static */
-bool
+BOOL
 WinUtils::PeekMessage(LPMSG aMsg, HWND aWnd, UINT aFirstMessage,
                       UINT aLastMessage, UINT aOption)
 {
 #ifdef ACCESSIBILITY
   if (NS_IsMainThread() && sAPCPending.exchange(false)) {
     while (sNtTestAlert() != STATUS_SUCCESS) ;
   }
 #endif
 #ifdef NS_ENABLE_TSF
   RefPtr<ITfMessagePump> msgPump = TSFTextStore::GetMessagePump();
   if (msgPump) {
     BOOL ret = FALSE;
     HRESULT hr = msgPump->PeekMessageW(aMsg, aWnd, aFirstMessage, aLastMessage,
                                        aOption, &ret);
-    NS_ENSURE_TRUE(SUCCEEDED(hr), false);
+    if (NS_WARN_IF(FAILED(hr))) {
+      return FALSE; // When PeekMessage() fails, returns FALSE.
+    }
     return ret;
   }
 #endif // #ifdef NS_ENABLE_TSF
   return ::PeekMessageW(aMsg, aWnd, aFirstMessage, aLastMessage, aOption);
 }
 
 /* static */
-bool
+BOOL
 WinUtils::GetMessage(LPMSG aMsg, HWND aWnd, UINT aFirstMessage,
                      UINT aLastMessage)
 {
 #ifdef NS_ENABLE_TSF
   RefPtr<ITfMessagePump> msgPump = TSFTextStore::GetMessagePump();
   if (msgPump) {
     BOOL ret = FALSE;
     HRESULT hr = msgPump->GetMessageW(aMsg, aWnd, aFirstMessage, aLastMessage,
                                       &ret);
-    NS_ENSURE_TRUE(SUCCEEDED(hr), false);
+    if (NS_WARN_IF(FAILED(hr))) {
+      return -1; // When GetMessage() fails, returns -1.
+    }
     return ret;
   }
 #endif // #ifdef NS_ENABLE_TSF
   return ::GetMessageW(aMsg, aWnd, aFirstMessage, aLastMessage);
 }
 
 #if defined(ACCESSIBILITY)
 static DWORD
--- a/widget/windows/WinUtils.h
+++ b/widget/windows/WinUtils.h
@@ -228,19 +228,19 @@ public:
   static void LogW(const wchar_t *fmt, ...);
 
   /**
    * PeekMessage() and GetMessage() are wrapper methods for PeekMessageW(),
    * GetMessageW(), ITfMessageMgr::PeekMessageW() and
    * ITfMessageMgr::GetMessageW().
    * Don't call the native APIs directly.  You MUST use these methods instead.
    */
-  static bool PeekMessage(LPMSG aMsg, HWND aWnd, UINT aFirstMessage,
+  static BOOL PeekMessage(LPMSG aMsg, HWND aWnd, UINT aFirstMessage,
                           UINT aLastMessage, UINT aOption);
-  static bool GetMessage(LPMSG aMsg, HWND aWnd, UINT aFirstMessage,
+  static BOOL GetMessage(LPMSG aMsg, HWND aWnd, UINT aFirstMessage,
                          UINT aLastMessage);
 
   /**
    * Wait until a message is ready to be processed.
    * Prefer using this method to directly calling ::WaitMessage since
    * ::WaitMessage will wait if there is an unread message in the queue.
    * That can cause freezes until another message enters the queue if the
    * message is marked read by a call to PeekMessage which the caller is
--- a/widget/windows/nsWindow.cpp
+++ b/widget/windows/nsWindow.cpp
@@ -3370,17 +3370,17 @@ FullscreenTransitionThreadProc(LPVOID lp
                  data->mBounds.width, data->mBounds.height, 0);
   data->mWnd = wnd;
   ::ReleaseSemaphore(data->mSemaphore, 1, nullptr);
   // The initialization data may no longer be valid
   // after we release the semaphore.
   data = nullptr;
 
   MSG msg;
-  while (::GetMessageW(&msg, nullptr, 0, 0)) {
+  while (WinUtils::GetMessage(&msg, nullptr, 0, 0)) {
     ::TranslateMessage(&msg);
     ::DispatchMessage(&msg);
   }
   ::ShowCursor(true);
   ::DestroyWindow(wnd);
   return 0;
 }
 
--- a/xpcom/threads/HangMonitor.cpp
+++ b/xpcom/threads/HangMonitor.cpp
@@ -22,16 +22,17 @@
 #include "GeckoProfiler.h"
 
 #ifdef MOZ_CRASHREPORTER
 #include "nsExceptionHandler.h"
 #endif
 
 #ifdef XP_WIN
 #include <windows.h>
+#include "WinUtils.h"
 #endif
 
 #if defined(MOZ_GECKO_PROFILER) && defined(MOZ_PROFILING) && defined(XP_WIN)
   #define REPORT_CHROME_HANGS
 #endif
 
 namespace mozilla {
 namespace HangMonitor {
@@ -344,22 +345,25 @@ IsUIMessageWaiting()
 {
 #ifndef XP_WIN
   return false;
 #else
 #define NS_WM_IMEFIRST WM_IME_SETCONTEXT
 #define NS_WM_IMELAST  WM_IME_KEYUP
   BOOL haveUIMessageWaiting = FALSE;
   MSG msg;
-  haveUIMessageWaiting |= ::PeekMessageW(&msg, nullptr, WM_KEYFIRST,
-                                         WM_IME_KEYLAST, PM_NOREMOVE);
-  haveUIMessageWaiting |= ::PeekMessageW(&msg, nullptr, NS_WM_IMEFIRST,
-                                         NS_WM_IMELAST, PM_NOREMOVE);
-  haveUIMessageWaiting |= ::PeekMessageW(&msg, nullptr, WM_MOUSEFIRST,
-                                         WM_MOUSELAST, PM_NOREMOVE);
+  haveUIMessageWaiting |=
+    widget::WinUtils::PeekMessage(&msg, nullptr, WM_KEYFIRST,
+                                  WM_IME_KEYLAST, PM_NOREMOVE);
+  haveUIMessageWaiting |=
+    widget::WinUtils::PeekMessage(&msg, nullptr, NS_WM_IMEFIRST,
+                                  NS_WM_IMELAST, PM_NOREMOVE);
+  haveUIMessageWaiting |=
+    widget::WinUtils::PeekMessage(&msg, nullptr, WM_MOUSEFIRST,
+                                  WM_MOUSELAST, PM_NOREMOVE);
   return haveUIMessageWaiting;
 #endif
 }
 
 void
 NotifyActivity(ActivityType aActivityType)
 {
   MOZ_ASSERT(NS_IsMainThread(),