Bug 1333459 - part4: Make EventStateManager resets "waiting reply from remote process" when the focused content isn't in remote process r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 21 Jul 2017 17:22:08 +0900
changeset 419176 57e0c8a166d2b378eed7c0008643ade6b184145f
parent 419175 73e6945ecfa74dff313c824201d4f06cc3e66424
child 419177 ec113974cdd9a44fa035a2c69678cad9fc28d35b
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1333459
milestone56.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 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().