Bug 1335670 NativeKey should dispatch consumed keydown event when it receives WM_NULL at removing WM_*CHAR from the queue and the original message has gone r=m_kato a=gchang
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 02 Feb 2017 22:43:20 +0900
changeset 375933 673d97776e64b8084064e59bca9cac760b7a11f9
parent 375932 2e221577245fb866c3221b1d3c63bd6166d24182
child 375934 0c836d66d8ab986fd1d7a8bb71cf8ec8240f48aa
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, gchang
bugs1335670
milestone53.0a2
Bug 1335670 NativeKey should dispatch consumed keydown event when it receives WM_NULL at removing WM_*CHAR from the queue and the original message has gone r=m_kato a=gchang Currently, NativeKey::GetFollowingCharMessage() tries 5 times to remove found char message from the queue. It was enough when we found this issue at developing Metrofox. However, this hack is not enough for some odd keyboard layouts because we see some crash reports which gives up to remove a char message from the queue because 5 WM_NULL messages are returned. For preventing this crash, we should check if there is the message which is trying to remove from the queue when NativeKey receives WM_NULL. Then, when there is no key message in the queue or next key message becomes non-char message,, NativeKey should dispatch consumed keydown event because we can assume that the key operation may have already been handled or canceled. Otherwise, NativeKey should retry to remove the message again (until 50 times!, it's just enough big magic number, there is no concrete reason). MozReview-Commit-ID: 1c6Y4OoQdrP
widget/windows/KeyboardLayout.cpp
--- a/widget/windows/KeyboardLayout.cpp
+++ b/widget/windows/KeyboardLayout.cpp
@@ -2831,17 +2831,17 @@ NativeKey::GetFollowingCharMessage(MSG& 
   mRemovingMsg = nextKeyMsg;
 
   mReceivedMsg = sEmptyMSG;
   AutoRestore<MSG> ensureToClearRecivedMsg(mReceivedMsg);
 
   // On Metrofox, PeekMessage() sometimes returns WM_NULL even if we specify
   // the message range.  So, if it returns WM_NULL, we should retry to get
   // the following char message it was found above.
-  for (uint32_t i = 0; i < 5; i++) {
+  for (uint32_t i = 0; i < 50; i++) {
     MSG removedMsg, nextKeyMsgInAllWindows;
     bool doCrash = false;
     if (!WinUtils::PeekMessage(&removedMsg, mMsg.hwnd,
                                nextKeyMsg.message, nextKeyMsg.message,
                                PM_REMOVE | PM_NOYIELD)) {
       // We meets unexpected case.  We should collect the message queue state
       // and crash for reporting the bug.
       doCrash = true;
@@ -2972,23 +2972,62 @@ NativeKey::GetFollowingCharMessage(MSG& 
       } else {
         CrashReporter::AppendAppNotesToCrashReport(
           NS_LITERAL_CSTRING("\nThere is no message in any window"));
       }
 #endif // #ifdef MOZ_CRASHREPORTER
       MOZ_CRASH("We lost the following char message");
     }
 
-    // Retry for the strange case.
+    // We're still not sure why ::PeekMessage() may return WM_NULL even with
+    // its first message and its last message are same message.  However,
+    // at developing Metrofox, we met this case even with usual keyboard
+    // layouts.  So, it might be possible in desktop application or it really
+    // occurs with some odd keyboard layouts which perhaps hook API.
     if (removedMsg.message == WM_NULL) {
       MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
         ("%p   NativeKey::GetFollowingCharMessage(), WARNING, failed to "
          "remove a char message, instead, removed WM_NULL message, ",
          "removedMsg=%s",
          this, ToString(removedMsg).get()));
+      // Check if there is the message which we're trying to remove.
+      MSG newNextKeyMsg;
+      if (!WinUtils::PeekMessage(&newNextKeyMsg, mMsg.hwnd,
+                                 WM_KEYFIRST, WM_KEYLAST,
+                                 PM_NOREMOVE | PM_NOYIELD)) {
+        // If there is no key message, we should mark this keydown as consumed
+        // because the key operation may have already been handled or canceled.
+        MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
+          ("%p   NativeKey::GetFollowingCharMessage(), WARNING, failed to "
+           "remove a char message because it's gone during removing it from "
+           "the queue, nextKeyMsg=%s",
+           this, ToString(nextKeyMsg).get()));
+        MOZ_ASSERT(!mCharMessageHasGone);
+        mFollowingCharMsgs.Clear();
+        mCharMessageHasGone = true;
+        return false;
+      }
+      if (!IsCharMessage(newNextKeyMsg)) {
+        // If next key message becomes a non-char message, we should mark this
+        // keydown as consumed because the key operation may have already been
+        // handled or canceled.
+        MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
+          ("%p   NativeKey::GetFollowingCharMessage(), WARNING, failed to "
+           "remove a char message because it's gone during removing it from "
+           "the queue, nextKeyMsg=%s, newNextKeyMsg=%s",
+           this, ToString(nextKeyMsg).get(), ToString(newNextKeyMsg).get()));
+        MOZ_ASSERT(!mCharMessageHasGone);
+        mFollowingCharMsgs.Clear();
+        mCharMessageHasGone = true;
+        return false;
+      }
+      MOZ_LOG(sNativeKeyLogger, LogLevel::Debug,
+        ("%p   NativeKey::GetFollowingCharMessage(), there is the message "
+         "which is being tried to be removed from the queue, trying again...",
+         this));
       continue;
     }
 
     // Typically, this case occurs with WM_DEADCHAR.  If the removed message's
     // wParam becomes 0, that means that the key event shouldn't cause text
     // input.  So, let's ignore the strange char message.
     if (removedMsg.message == nextKeyMsg.message && !removedMsg.wParam) {
       MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,