Bug 1333459 - part4: Make EventStateManager resets "waiting reply from remote process" when the focused content isn't in remote process r?smaug draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 21 Jul 2017 17:22:08 +0900
changeset 613985 3807492e4156dfea9c42795709975df6bcdc38c7
parent 613984 f2e79675ac28c353b0933b8df5c6c45bc6173968
child 613986 715bbeb41d0e8a4b064e4e416b931977097f8b16
push id69882
push usermasayuki@d-toybox.com
push dateSun, 23 Jul 2017 15:20:52 +0000
reviewerssmaug
bugs1333459
milestone56.0a1
Bug 1333459 - part4: Make EventStateManager resets "waiting reply from remote process" when the focused content isn't in remote process r?smaug On macOS, we fall back eKeyPress event to native menu. Therefore, widget always requests a reply from remote process because it's difficult to check if the eKeyPress event will be sent to a remote process actually. If it's not sent to any remote processes, PresShell needs to dispatch the event into the DOM tree. Additionally, even if it's marked as "waiting reply from remote process", it needs to dispatch the DOM event in the main process first because we need to check if the key combination is reserved by chrome (if it's reserved, the eKeyPress event shouldn't be fired in the remote process). Therefore, this patch makes EventStateManager::PreHandleEvent() resets the state when focused content isn't in any remote processes and the event's propagation hasn't been stopped. Additionally, this patch makes PresShell::HandleEventInternal() checks WidgetEvent::PropgationStopped() with WidgetEvent::IsWaitingReplyFromRemoteProcess() before dispatching the event into the DOM tree. MozReview-Commit-ID: FmgL3rCuQ8y
dom/events/EventStateManager.cpp
layout/base/PresShell.cpp
widget/BasicEvents.h
widget/cocoa/TextInputHandler.mm
--- a/dom/events/EventStateManager.cpp
+++ b/dom/events/EventStateManager.cpp
@@ -809,16 +809,28 @@ EventStateManager::PreHandleEvent(nsPres
       //       TextComposition::IsComposing() is false even before
       //       compositionend if there is no composing string.
       //       And also don't expose other document's composition state.
       //       A native IME context is typically shared by multiple documents.
       //       So, don't use GetTextCompositionFor(nsIWidget*) here.
       RefPtr<TextComposition> composition =
         IMEStateManager::GetTextCompositionFor(aPresContext);
       aEvent->AsKeyboardEvent()->mIsComposing = !!composition;
+
+      // Widget may need to perform default action for specific keyboard
+      // event if it's not consumed.  In this case, widget has already marked
+      // the event as "waiting reply from remote process".  However, we need
+      // to reset it if the target (focused content) isn't in a remote process
+      // because PresShell needs to check if it's marked as so before
+      // dispatching events into the DOM tree.
+      if (aEvent->IsWaitingReplyFromRemoteProcess() &&
+          !aEvent->PropagationStopped() &&
+          !IsRemoteTarget(content)) {
+        aEvent->ResetWaitingReplyFromRemoteProcessState();
+      }
     }
     break;
   case eWheel:
   case eWheelOperationStart:
   case eWheelOperationEnd:
     {
       NS_ASSERTION(aEvent->IsTrusted(),
                    "Untrusted wheel event shouldn't be here");
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -8156,21 +8156,29 @@ PresShell::HandleEventInternal(WidgetEve
 
     // 2. Give event to the DOM for third party and JS use.
     if (NS_SUCCEEDED(rv)) {
       bool wasHandlingKeyBoardEvent =
         nsContentUtils::IsHandlingKeyBoardEvent();
       if (aEvent->mClass == eKeyboardEventClass) {
         nsContentUtils::SetIsHandlingKeyBoardEvent(true);
       }
-      // If EventStateManager or something wants reply from remote process,
-      // PresShell shouldn't dispatch the event into the DOM tree because they
-      // don't have a chance to stop propagation in the system event group.
+      // If EventStateManager or something wants reply from remote process and
+      // needs to win any other event listeners in chrome, the event is both
+      // stopped its propagation and marked as "waiting reply from remote
+      // process".  In this case, PresShell shouldn't dispatch the event into
+      // the DOM tree because they don't have a chance to stop propagation in
+      // the system event group.  On the other hand, if its propagation is not
+      // stopped, that means that the event may be reserved by chrome.  If it's
+      // reserved by chrome, the event shouldn't be sent to any remote
+      // processes.  In this case, PresShell needs to dispatch the event to
+      // the DOM tree for checking if it's reserved.
       if (aEvent->IsAllowedToDispatchDOMEvent() &&
-          !aEvent->IsWaitingReplyFromRemoteProcess()) {
+          !(aEvent->PropagationStopped() &&
+            aEvent->IsWaitingReplyFromRemoteProcess())) {
         MOZ_ASSERT(nsContentUtils::IsSafeToRunScript(),
           "Somebody changed aEvent to cause a DOM event!");
         nsPresShellEventCB eventCB(this);
         if (nsIFrame* target = GetCurrentEventFrame()) {
           if (target->OnlySystemGroupDispatch(aEvent->mMessage)) {
               aEvent->StopPropagation();
           }
         }
--- a/widget/BasicEvents.h
+++ b/widget/BasicEvents.h
@@ -235,25 +235,27 @@ public:
    * Return true if the event shouldn't be dispatched to remote process.
    */
   inline bool IsCrossProcessForwardingStopped() const
   {
     return mNoRemoteProcessDispatch;
   }
   /**
    * Mark the event as waiting reply from remote process.
+   * If the caller needs to win other keyboard event handlers in chrome,
+   * the caller should call StopPropagation() too.
+   * Otherwise, if the caller just needs to know if the event is consumed by
+   * either content or chrome, it should just call this because the event
+   * may be reserved by chrome and it needs to be dispatched into the DOM
+   * tree in chrome for checking if it's reserved before being sent to any
+   * remote processes.
    */
   inline void MarkAsWaitingReplyFromRemoteProcess()
   {
     MOZ_ASSERT(!mPostedToRemoteProcess);
-    // When this is called, it means that event handlers in this process need
-    // a reply from content in a remote process.  So, callers should stop
-    // propagation in this process first.
-    NS_ASSERTION(PropagationStopped(),
-                 "Why didn't you stop propagation in this process?");
     mNoRemoteProcessDispatch = false;
     mWantReplyFromContentProcess = true;
   }
   /**
    * Reset "waiting reply from remote process" state.  This is useful when
    * you dispatch a copy of an event coming from different process.
    */
   inline void ResetWaitingReplyFromRemoteProcessState()
--- a/widget/cocoa/TextInputHandler.mm
+++ b/widget/cocoa/TextInputHandler.mm
@@ -2810,17 +2810,22 @@ IMEInputHandler::WillDispatchKeyboardEve
 
   KeyEventState* currentKeyEvent = static_cast<KeyEventState*>(aData);
   NSEvent* nativeEvent = currentKeyEvent->mKeyEvent;
   nsAString* insertString = currentKeyEvent->mInsertString;
   if (aKeyboardEvent.mMessage == eKeyPress && aIndexOfKeypress == 0 &&
       (!insertString || insertString->IsEmpty())) {
     // Inform the child process that this is an event that we want a reply
     // from.
-    aKeyboardEvent.mFlags.mWantReplyFromContentProcess = true;
+    // XXX This should be called only when the target is a remote process.
+    //     However, it's difficult to check it under widget/.
+    //     So, let's do this here for now, then,
+    //     EventStateManager::PreHandleEvent() will reset the flags if
+    //     the event target isn't in remote process.
+    aKeyboardEvent.MarkAsWaitingReplyFromRemoteProcess();
   }
   if (KeyboardLayoutOverrideRef().mOverrideEnabled) {
     TISInputSourceWrapper tis;
     tis.InitByLayoutID(KeyboardLayoutOverrideRef().mKeyboardLayout, true);
     tis.WillDispatchKeyboardEvent(nativeEvent, insertString, aKeyboardEvent);
     return;
   }
   TISInputSourceWrapper::CurrentInputSource().