Bug 1369419 GetMessage() and PeekMessage() shouldn't be used directly as far as possible r=jimm
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 02 Jun 2017 12:02:35 +0900
changeset 410260 b404c31db6de9f6724ddd5a86e30acc07ba367f0
parent 410259 2ba8cf87141eb96731b220960c49edc9aa4f089b
child 410261 b609d94a575a85e6228f751893d6324683510670
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimm
bugs1369419
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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(),