Bug 1318265 - NativeKey shouldn't handle messages when mWidget has already been destroyed r=m_kato a=jcristau
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 17 Nov 2016 19:02:30 +0900
changeset 480357 afe0d94be5473f11a871efad31bb638c17ca3e8f
parent 480356 06eda6a9a51fa6a6d2d2e38b178d4bf009891a77
child 480358 462a50df3d934aaff8483f9aa0f6a231d91f6cae
push id44524
push usermartin.thomson@gmail.com
push dateWed, 08 Feb 2017 05:10:11 +0000
reviewersm_kato, jcristau
bugs1318265
milestone52.0
Bug 1318265 - NativeKey shouldn't handle messages when mWidget has already been destroyed r=m_kato a=jcristau When mWidget was already destroyed, anybody shouldn't dispatch WidgetEvent on it. Therefore, NativeKey::InitKeyEvent() has MOZ_CRASH() for detecting such dangerous bug and some users hit it. Each message handler of NativeKey should check if mWidget has already gone. Ideally, nsWindow shouldn't create NativeKey and try to handle the message with it. However, using NativeKey's message handlers can put some information to the log. Therefore, this patch doesn't touch nsWindow. MozReview-Commit-ID: 4k5VfaKHPgG
widget/windows/KeyboardLayout.cpp
--- a/widget/windows/KeyboardLayout.cpp
+++ b/widget/windows/KeyboardLayout.cpp
@@ -2099,16 +2099,24 @@ NativeKey::DispatchCommandEvent(uint32_t
      "result=%s, mWidget->Destroyed()=%s",
      this, GetBoolName(ok), GetBoolName(mWidget->Destroyed())));
   return ok;
 }
 
 bool
 NativeKey::HandleAppCommandMessage() const
 {
+  // If the widget has gone, we should do nothing.
+  if (mWidget->Destroyed()) {
+    MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
+      ("%p   NativeKey::HandleAppCommandMessage(), WARNING, not handled due to "
+       "destroyed the widget", this));
+    return false;
+  }
+
   // NOTE: Typical behavior of WM_APPCOMMAND caused by key is, WM_APPCOMMAND
   //       message is _sent_ first.  Then, the DefaultWndProc will _post_
   //       WM_KEYDOWN message and WM_KEYUP message if the keycode for the
   //       command is available (i.e., mVirtualKeyCode is not 0).
 
   // NOTE: IntelliType (Microsoft's keyboard utility software) always consumes
   //       WM_KEYDOWN and WM_KEYUP.
 
@@ -2218,16 +2226,17 @@ NativeKey::HandleAppCommandMessage() con
         contentCommandMessage = eContentCommandRedo;
         break;
       case APPCOMMAND_UNDO:
         contentCommandMessage = eContentCommandUndo;
         break;
     }
 
     if (contentCommandMessage) {
+      MOZ_ASSERT(!mWidget->Destroyed());
       WidgetContentCommandEvent contentCommandEvent(true, contentCommandMessage,
                                                     mWidget);
       MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
         ("%p   NativeKey::HandleAppCommandMessage(), dispatching %s event...",
          this, ToChar(contentCommandMessage)));
       mWidget->DispatchWindowEvent(&contentCommandEvent);
       MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
         ("%p   NativeKey::HandleAppCommandMessage(), dispatched %s event",
@@ -2246,16 +2255,17 @@ NativeKey::HandleAppCommandMessage() con
         ("%p   NativeKey::HandleAppCommandMessage(), doesn't dispatch content "
          "command event", this));
     }
   }
 
   // Dispatch a keyup event if the command is caused by pressing a key and
   // the key isn't mapped to a virtual keycode.
   if (dispatchKeyEvent && !mVirtualKeyCode) {
+    MOZ_ASSERT(!mWidget->Destroyed());
     nsresult rv = mDispatcher->BeginNativeInputTransaction();
     if (NS_WARN_IF(NS_FAILED(rv))) {
       MOZ_LOG(sNativeKeyLogger, LogLevel::Error,
         ("%p   NativeKey::HandleAppCommandMessage(), FAILED due to "
          "BeginNativeInputTransaction() failure", this));
       return true;
     }
     MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
@@ -2297,23 +2307,40 @@ NativeKey::HandleKeyDownMessage(bool* aE
       sDispatchedKeyOfAppCommand == mOriginalVirtualKeyCode) {
     // The multimedia key event has already been dispatch from
     // HandleAppCommandMessage().
     sDispatchedKeyOfAppCommand = 0;
     MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
       ("%p   NativeKey::HandleKeyDownMessage(), doesn't dispatch keydown "
        "event due to already dispatched from HandleAppCommandMessage(), ",
        this));
+    if (RedirectedKeyDownMessageManager::IsRedirectedMessage(mMsg)) {
+      RedirectedKeyDownMessageManager::Forget();
+    }
     return true;
   }
 
   if (IsReservedBySystem()) {
     MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
       ("%p   NativeKey::HandleKeyDownMessage(), doesn't dispatch keydown "
        "event because the key combination is reserved by the system", this));
+    if (RedirectedKeyDownMessageManager::IsRedirectedMessage(mMsg)) {
+      RedirectedKeyDownMessageManager::Forget();
+    }
+    return false;
+  }
+
+  // If the widget has gone, we should do nothing.
+  if (mWidget->Destroyed()) {
+    MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
+      ("%p   NativeKey::HandleKeyDownMessage(), WARNING, not handled due to "
+       "destroyed the widget", this));
+    if (RedirectedKeyDownMessageManager::IsRedirectedMessage(mMsg)) {
+      RedirectedKeyDownMessageManager::Forget();
+    }
     return false;
   }
 
   bool defaultPrevented = false;
   if (mFakeCharMsgs || IsKeyMessageOnPlugin() ||
       !RedirectedKeyDownMessageManager::IsRedirectedMessage(mMsg)) {
     nsresult rv = mDispatcher->BeginNativeInputTransaction();
     if (NS_WARN_IF(NS_FAILED(rv))) {
@@ -2427,16 +2454,18 @@ NativeKey::HandleKeyDownMessage(bool* aE
     // event already.
     if (aEventDispatched) {
       *aEventDispatched = true;
     }
   }
 
   RedirectedKeyDownMessageManager::Forget();
 
+  MOZ_ASSERT(!mWidget->Destroyed());
+
   // If the key was processed by IME, we shouldn't dispatch keypress event.
   if (mOriginalVirtualKeyCode == VK_PROCESSKEY) {
     MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
       ("%p   NativeKey::HandleKeyDownMessage(), not dispatching keypress "
        "event because the key was already handled by IME, defaultPrevented=%s",
        this, GetBoolName(defaultPrevented)));
     return defaultPrevented;
   }
@@ -2536,16 +2565,24 @@ NativeKey::HandleCharMessage(const MSG& 
   // eKeyPress event for it and passes the message to next wndproc.
   if (IsReservedBySystem()) {
     MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
       ("%p   NativeKey::HandleCharMessage(), doesn't dispatch keypress "
        "event because the key combination is reserved by the system", this));
     return false;
   }
 
+  // If the widget has gone, we should do nothing.
+  if (mWidget->Destroyed()) {
+    MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
+      ("%p   NativeKey::HandleCharMessage(), WARNING, not handled due to "
+       "destroyed the widget", this));
+    return false;
+  }
+
   // When a control key is inputted by a key, it should be handled without
   // WM_*CHAR messages at receiving WM_*KEYDOWN message.  So, when we receive
   // WM_*CHAR message directly, we see a control character here.
   if (IsControlCharMessage(aCharMsg)) {
     // In this case, we don't need to dispatch eKeyPress event because:
     // 1. We're the only browser which dispatches "keypress" event for
     //    non-printable characters (Although, both Chrome and Edge dispatch
     //    "keypress" event for some keys accidentally.  For example, "IntlRo"
@@ -2629,16 +2666,24 @@ NativeKey::HandleKeyUpMessage(bool* aEve
   // eKeyUp event for it and passes the message to next wndproc.
   if (IsReservedBySystem()) {
     MOZ_LOG(sNativeKeyLogger, LogLevel::Info,
       ("%p   NativeKey::HandleKeyUpMessage(), doesn't dispatch keyup "
        "event because the key combination is reserved by the system", this));
     return false;
   }
 
+  // If the widget has gone, we should do nothing.
+  if (mWidget->Destroyed()) {
+    MOZ_LOG(sNativeKeyLogger, LogLevel::Warning,
+      ("%p   NativeKey::HandleKeyUpMessage(), WARNING, not handled due to "
+       "destroyed the widget", this));
+    return false;
+  }
+
   nsresult rv = mDispatcher->BeginNativeInputTransaction();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     MOZ_LOG(sNativeKeyLogger, LogLevel::Error,
       ("%p   NativeKey::HandleKeyUpMessage(), FAILED due to "
        "BeginNativeInputTransaction() failure", this));
     return true;
   }
 
@@ -3193,16 +3238,17 @@ NativeKey::ComputeInputtingStringWithKey
   }
 }
 
 bool
 NativeKey::DispatchKeyPressEventsWithRetrievedCharMessages() const
 {
   MOZ_ASSERT(IsKeyDownMessage());
   MOZ_ASSERT(IsFollowedByPrintableCharOrSysCharMessage());
+  MOZ_ASSERT(!mWidget->Destroyed());
 
   nsresult rv = mDispatcher->BeginNativeInputTransaction();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     MOZ_LOG(sNativeKeyLogger, LogLevel::Error,
       ("%p   NativeKey::DispatchKeyPressEventsWithRetrievedCharMessages(), "
        "FAILED due to BeginNativeInputTransaction() failure", this));
     return true;
   }
@@ -3243,16 +3289,17 @@ NativeKey::DispatchKeyPressEventsWithRet
   return consumed;
 }
 
 bool
 NativeKey::DispatchKeyPressEventsWithoutCharMessage() const
 {
   MOZ_ASSERT(IsKeyDownMessage());
   MOZ_ASSERT(!mIsDeadKey || !mCommittedCharsAndModifiers.IsEmpty());
+  MOZ_ASSERT(!mWidget->Destroyed());
 
   nsresult rv = mDispatcher->BeginNativeInputTransaction();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     MOZ_LOG(sNativeKeyLogger, LogLevel::Error,
       ("%p   NativeKey::DispatchKeyPressEventsWithoutCharMessage(), FAILED due "
        "to BeginNativeInputTransaction() failure", this));
     return true;
   }