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 draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 02 Feb 2017 22:43:20 +0900
changeset 469575 2434eb787bb3837f0efa84a4e4a74fb006cbe45e
parent 469566 d0f35dfb7d194054c7885971815664ed04edfe03
child 469576 14e91c31f2a055bc4440a8edb92c1761d3566048
child 469628 1c137fabe9f319a633f735a20923c365ccf6c8ca
push id43780
push usermasayuki@d-toybox.com
push dateThu, 02 Feb 2017 13:51:09 +0000
reviewersm_kato
bugs1335670
milestone54.0a1
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 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,