Bug 1336028 NativeKey::GetFollowingCharMessage() should take newer char message when found char message and removed message from the queue is different but their scancode indicates same physical key r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 02 Feb 2017 23:28:48 +0900
changeset 332350 b3d2a4cdecd15085d59d2d5fc7661392fb771787
parent 332349 26268dace54f614d06554596c3c8967b957418db
child 332351 28bb04d0338d2741bbc951566f156744a50060bc
push id31303
push usercbook@mozilla.com
push dateFri, 03 Feb 2017 12:23:51 +0000
treeherdermozilla-central@28bb04d0338d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1336028
milestone54.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 1336028 NativeKey::GetFollowingCharMessage() should take newer char message when found char message and removed message from the queue is different but their scancode indicates same physical key r=m_kato NativeKey::GetFollowingCharMessage() may remove a char message which is different from previously found message in the queue because hacky keyboard layout or utility can overwrite the wParam when it's removed from the queue. Now, we should assume that newer message, i.e., actually removed from the queue, is the expected message by the user. See bug 1336028 comment 0 for the actual scenarios which are collected by crash reports. https://bugzilla.mozilla.org/show_bug.cgi?id=1336028#c0 MozReview-Commit-ID: 9ZgukHH1vfi
widget/windows/KeyboardLayout.cpp
widget/windows/KeyboardLayout.h
--- a/widget/windows/KeyboardLayout.cpp
+++ b/widget/windows/KeyboardLayout.cpp
@@ -2783,16 +2783,32 @@ NativeKey::MayBeSameCharMessage(const MS
   static const LPARAM kScanCodeMask = 0x00FF0000;
   return
     aCharMsg1.message == aCharMsg2.message &&
     aCharMsg1.wParam == aCharMsg2.wParam &&
     (aCharMsg1.lParam & ~kScanCodeMask) == (aCharMsg2.lParam & ~kScanCodeMask);
 }
 
 bool
+NativeKey::IsSamePhysicalKeyMessage(const MSG& aKeyOrCharMsg1,
+                                    const MSG& aKeyOrCharMsg2) const
+{
+  if (NS_WARN_IF(aKeyOrCharMsg1.message < WM_KEYFIRST) ||
+      NS_WARN_IF(aKeyOrCharMsg1.message > WM_KEYLAST) ||
+      NS_WARN_IF(aKeyOrCharMsg2.message < WM_KEYFIRST) ||
+      NS_WARN_IF(aKeyOrCharMsg2.message > WM_KEYLAST)) {
+    return false;
+  }
+  return WinUtils::GetScanCode(aKeyOrCharMsg1.lParam) ==
+           WinUtils::GetScanCode(aKeyOrCharMsg2.lParam) &&
+         WinUtils::IsExtendedScanCode(aKeyOrCharMsg1.lParam) ==
+           WinUtils::IsExtendedScanCode(aKeyOrCharMsg2.lParam);
+}
+
+bool
 NativeKey::GetFollowingCharMessage(MSG& aCharMsg)
 {
   MOZ_ASSERT(IsKeyDownMessage());
   MOZ_ASSERT(!IsKeyMessageOnPlugin());
 
   aCharMsg.message = WM_NULL;
 
   if (mFakeCharMsgs) {
@@ -3033,74 +3049,90 @@ NativeKey::GetFollowingCharMessage(MSG& 
       MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
         ("%p   NativeKey::GetFollowingCharMessage(), WARNING, succeeded to "
          "remove a char message, but the removed message's wParam is 0, "
          "removedMsg=%s",
          this, ToString(removedMsg).get()));
       return false;
     }
 
+    // This is normal case.
+    if (MayBeSameCharMessage(removedMsg, nextKeyMsg)) {
+      aCharMsg = removedMsg;
+      MOZ_LOG(sNativeKeyLogger, LogLevel::Verbose,
+        ("%p   NativeKey::GetFollowingCharMessage(), succeeded to retrieve "
+         "next char message, aCharMsg=%s",
+         this, ToString(aCharMsg).get()));
+      return true;
+    }
+
+    // Even if removed message is different char message from the found char
+    // message, when the scan code is same, we can assume that the message
+    // is overwritten by somebody who hooks API.  See bug 1336028 comment 0 for
+    // the possible scenarios.
+    if (IsCharMessage(removedMsg) &&
+        IsSamePhysicalKeyMessage(removedMsg, nextKeyMsg)) {
+      aCharMsg = removedMsg;
+      MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
+        ("%p   NativeKey::GetFollowingCharMessage(), WARNING, succeeded to "
+         "remove a char message, but the removed message was changed from "
+         "the found message except their scancode, aCharMsg=%s, "
+         "nextKeyMsg(the found message)=%s",
+         this, ToString(aCharMsg).get(), ToString(nextKeyMsg).get()));
+      return true;
+    }
+
     // NOTE: Although, we don't know when this case occurs, the scan code value
     //       in lParam may be changed from 0 to something.  The changed value
     //       is different from the scan code of handling keydown message.
-    if (!MayBeSameCharMessage(removedMsg, nextKeyMsg)) {
-      MOZ_LOG(sNativeKeyLogger, LogLevel::Error,
-        ("%p   NativeKey::GetFollowingCharMessage(), FAILED, removed message "
-         "is really different from what we have already found, removedMsg=%s, "
-         "nextKeyMsg=%s",
-         this, ToString(removedMsg).get(), ToString(nextKeyMsg).get()));
+    MOZ_LOG(sNativeKeyLogger, LogLevel::Error,
+      ("%p   NativeKey::GetFollowingCharMessage(), FAILED, removed message "
+       "is really different from what we have already found, removedMsg=%s, "
+       "nextKeyMsg=%s",
+       this, ToString(removedMsg).get(), ToString(nextKeyMsg).get()));
 #ifdef MOZ_CRASHREPORTER
-      nsPrintfCString info("\nPeekMessage() removed unexpcted char message! "
-                           "\nActive keyboard layout=0x%08X (%s), "
-                           "\nHandling message: %s, InSendMessageEx()=%s, "
-                           "\nFound message: %s, "
-                           "\nRemoved message: %s, ",
-                           KeyboardLayout::GetActiveLayout(),
-                           KeyboardLayout::GetActiveLayoutName().get(),
-                           ToString(mMsg).get(),
-                           GetResultOfInSendMessageEx().get(),
-                           ToString(nextKeyMsg).get(),
-                           ToString(removedMsg).get());
+    nsPrintfCString info("\nPeekMessage() removed unexpcted char message! "
+                         "\nActive keyboard layout=0x%08X (%s), "
+                         "\nHandling message: %s, InSendMessageEx()=%s, "
+                         "\nFound message: %s, "
+                         "\nRemoved message: %s, ",
+                         KeyboardLayout::GetActiveLayout(),
+                         KeyboardLayout::GetActiveLayoutName().get(),
+                         ToString(mMsg).get(),
+                         GetResultOfInSendMessageEx().get(),
+                         ToString(nextKeyMsg).get(),
+                         ToString(removedMsg).get());
+    CrashReporter::AppendAppNotesToCrashReport(info);
+    // What's the next key message?
+    MSG nextKeyMsgAfter;
+    if (WinUtils::PeekMessage(&nextKeyMsgAfter, mMsg.hwnd,
+                              WM_KEYFIRST, WM_KEYLAST,
+                              PM_NOREMOVE | PM_NOYIELD)) {
+      nsPrintfCString info("\nNext key message after unexpected char message "
+                           "removed: %s, ",
+                           ToString(nextKeyMsgAfter).get());
       CrashReporter::AppendAppNotesToCrashReport(info);
-      // What's the next key message?
-      MSG nextKeyMsgAfter;
-      if (WinUtils::PeekMessage(&nextKeyMsgAfter, mMsg.hwnd,
-                                WM_KEYFIRST, WM_KEYLAST,
-                                PM_NOREMOVE | PM_NOYIELD)) {
-        nsPrintfCString info("\nNext key message after unexpected char message "
-                             "removed: %s, ",
-                             ToString(nextKeyMsgAfter).get());
-        CrashReporter::AppendAppNotesToCrashReport(info);
-      } else {
-        CrashReporter::AppendAppNotesToCrashReport(
-          NS_LITERAL_CSTRING("\nThere is no key message after unexpected char "
-                             "message removed, "));
-      }
-      // Another window has a key message?
-      MSG nextKeyMsgInAllWindows;
-      if (WinUtils::PeekMessage(&nextKeyMsgInAllWindows, 0,
-                                WM_KEYFIRST, WM_KEYLAST,
-                                PM_NOREMOVE | PM_NOYIELD)) {
-        nsPrintfCString info("\nNext key message in all windows: %s.",
-                             ToString(nextKeyMsgInAllWindows).get());
-        CrashReporter::AppendAppNotesToCrashReport(info);
-      } else {
-        CrashReporter::AppendAppNotesToCrashReport(
-          NS_LITERAL_CSTRING("\nThere is no key message in any windows."));
-      }
+    } else {
+      CrashReporter::AppendAppNotesToCrashReport(
+        NS_LITERAL_CSTRING("\nThere is no key message after unexpected char "
+                           "message removed, "));
+    }
+    // Another window has a key message?
+    if (WinUtils::PeekMessage(&nextKeyMsgInAllWindows, 0,
+                              WM_KEYFIRST, WM_KEYLAST,
+                              PM_NOREMOVE | PM_NOYIELD)) {
+      nsPrintfCString info("\nNext key message in all windows: %s.",
+                           ToString(nextKeyMsgInAllWindows).get());
+      CrashReporter::AppendAppNotesToCrashReport(info);
+    } else {
+      CrashReporter::AppendAppNotesToCrashReport(
+        NS_LITERAL_CSTRING("\nThere is no key message in any windows."));
+    }
 #endif // #ifdef MOZ_CRASHREPORTER
-      MOZ_CRASH("PeekMessage() removed unexpected message");
-    }
-
-    aCharMsg = removedMsg;
-    MOZ_LOG(sNativeKeyLogger, LogLevel::Verbose,
-      ("%p   NativeKey::GetFollowingCharMessage(), succeeded to retrieve next "
-       "char message, aCharMsg=%s",
-       this, ToString(aCharMsg).get()));
-    return true;
+    MOZ_CRASH("PeekMessage() removed unexpected message");
   }
   MOZ_LOG(sNativeKeyLogger, LogLevel::Error,
     ("%p   NativeKey::GetFollowingCharMessage(), FAILED, removed messages "
      "are all WM_NULL, nextKeyMsg=%s",
      this, ToString(nextKeyMsg).get()));
 #ifdef MOZ_CRASHREPORTER
   nsPrintfCString info("\nWe lost following char message! "
                        "\nActive keyboard layout=0x%08X (%s), "
--- a/widget/windows/KeyboardLayout.h
+++ b/widget/windows/KeyboardLayout.h
@@ -508,16 +508,18 @@ private:
   {
     return IsSysCharMessage(aMSG.message);
   }
   bool IsSysCharMessage(UINT aMessage) const
   {
     return (aMessage == WM_SYSCHAR || aMessage == WM_SYSDEADCHAR);
   }
   bool MayBeSameCharMessage(const MSG& aCharMsg1, const MSG& aCharMsg2) const;
+  bool IsSamePhysicalKeyMessage(const MSG& aKeyOrCharMsg1,
+                                const MSG& aKeyOrCharMsg2) const;
   bool IsFollowedByPrintableCharMessage() const;
   bool IsFollowedByPrintableCharOrSysCharMessage() const;
   bool IsFollowedByDeadCharMessage() const;
   bool IsKeyMessageOnPlugin() const
   {
     return (mMsg.message == MOZ_WM_KEYDOWN ||
             mMsg.message == MOZ_WM_KEYUP);
   }