[tbb-commits] [tor-browser] 21/37: Bug 1763634 - part 2: Make `BrowserParent` verify whether replied keyboard events are what it sent. r=smaug, a=RyanVM

gitolite role git at cupani.torproject.org
Wed Jun 22 18:27:30 UTC 2022


This is an automated email from the git hooks/post-receive script.

richard pushed a commit to branch tor-browser-91.11.0esr-11.5-1
in repository tor-browser.

commit 930671a4e229c7be850a74656c0a92ba7b41b0ce
Author: Masayuki Nakano <masayuki at d-toybox.com>
AuthorDate: Mon Jun 6 07:42:50 2022 +0000

    Bug 1763634 - part 2: Make `BrowserParent` verify whether replied keyboard events are what it sent. r=smaug, a=RyanVM
    
    For making this, `BrowserParent` (temporarily) store sent keyboard events which
    wants reply from the remote process.  Then, when it gets a reply event received,
    it compares whether the event data is broken or not.
    
    The different point from the patch for mozilla-central is, there was no
    `nsID::GenerateUUID` because it was introduced in 95 (bug 1723674).  Instead,
    this patch uses `nsIUUIDGenerator::GenerateUUIDInPlace`.  It looks slower
    since it requires to get the service.  However, it must be fine because this
    is a reply for a users' key operation, i.e., not a hot path.
    
    Differential Revision: https://phabricator.services.mozilla.com/D148242
    
    Depends on D148241
---
 dom/ipc/BrowserChild.cpp  | 80 ++++++++++++++++++++++++++---------------------
 dom/ipc/BrowserChild.h    |  4 +--
 dom/ipc/BrowserParent.cpp | 61 ++++++++++++++++++++++++++++++++----
 dom/ipc/BrowserParent.h   | 20 +++++++++++-
 dom/ipc/PBrowser.ipdl     | 24 ++++++++++++--
 widget/BasicEvents.h      |  7 +++--
 6 files changed, 147 insertions(+), 49 deletions(-)

diff --git a/dom/ipc/BrowserChild.cpp b/dom/ipc/BrowserChild.cpp
index 9f1bccda2efef..32c397826906c 100644
--- a/dom/ipc/BrowserChild.cpp
+++ b/dom/ipc/BrowserChild.cpp
@@ -2105,68 +2105,78 @@ void BrowserChild::UpdateRepeatedKeyEventEndTime(
 }
 
 mozilla::ipc::IPCResult BrowserChild::RecvRealKeyEvent(
-    const WidgetKeyboardEvent& aEvent) {
-  if (SkipRepeatedKeyEvent(aEvent)) {
-    return IPC_OK();
-  }
-
-  MOZ_ASSERT(
-      aEvent.mMessage != eKeyPress || aEvent.AreAllEditCommandsInitialized(),
-      "eKeyPress event should have native key binding information");
+    const WidgetKeyboardEvent& aEvent, const nsID& aUUID) {
+  MOZ_ASSERT_IF(aEvent.mMessage == eKeyPress,
+                aEvent.AreAllEditCommandsInitialized());
 
   // If content code called preventDefault() on a keydown event, then we don't
   // want to process any following keypress events.
-  if (aEvent.mMessage == eKeyPress && mIgnoreKeyPressEvent) {
-    return IPC_OK();
-  }
+  const bool isPrecedingKeyDownEventConsumed =
+      aEvent.mMessage == eKeyPress && mIgnoreKeyPressEvent;
 
   WidgetKeyboardEvent localEvent(aEvent);
   localEvent.mWidget = mPuppetWidget;
   localEvent.mUniqueId = aEvent.mUniqueId;
-  nsEventStatus status = DispatchWidgetEventViaAPZ(localEvent);
 
-  // Update the end time of the possible repeated event so that we can skip
-  // some incoming events in case event handling took long time.
-  UpdateRepeatedKeyEventEndTime(localEvent);
+  if (!SkipRepeatedKeyEvent(aEvent) && !isPrecedingKeyDownEventConsumed) {
+    nsEventStatus status = DispatchWidgetEventViaAPZ(localEvent);
 
-  if (aEvent.mMessage == eKeyDown) {
-    mIgnoreKeyPressEvent = status == nsEventStatus_eConsumeNoDefault;
-  }
+    // Update the end time of the possible repeated event so that we can skip
+    // some incoming events in case event handling took long time.
+    UpdateRepeatedKeyEventEndTime(localEvent);
 
-  if (localEvent.mFlags.mIsSuppressedOrDelayed) {
-    localEvent.PreventDefault();
-  }
+    if (aEvent.mMessage == eKeyDown) {
+      mIgnoreKeyPressEvent = status == nsEventStatus_eConsumeNoDefault;
+    }
+
+    if (localEvent.mFlags.mIsSuppressedOrDelayed) {
+      localEvent.PreventDefault();
+    }
 
-  // If a response is desired from the content process, resend the key event.
-  if (aEvent.WantReplyFromContentProcess()) {
     // If the event's default isn't prevented but the status is no default,
     // That means that the event was consumed by EventStateManager or something
     // which is not a usual event handler.  In such case, prevent its default
     // as a default handler.  For example, when an eKeyPress event matches
-    // with a content accesskey, and it's executed, peventDefault() of the
+    // with a content accesskey, and it's executed, preventDefault() of the
     // event won't be called but the status is set to "no default".  Then,
     // the event shouldn't be handled by nsMenuBarListener in the main process.
     if (!localEvent.DefaultPrevented() &&
         status == nsEventStatus_eConsumeNoDefault) {
       localEvent.PreventDefault();
     }
-    // This is an ugly hack, mNoRemoteProcessDispatch is set to true when the
-    // event's PreventDefault() or StopScrollProcessForwarding() is called.
-    // And then, it'll be checked by ParamTraits<mozilla::WidgetEvent>::Write()
-    // whether the event is being sent to remote process unexpectedly.
-    // However, unfortunately, it cannot check the destination.  Therefore,
-    // we need to clear the flag explicitly here because ParamTraits should
-    // keep checking the flag for avoiding regression.
-    localEvent.mFlags.mNoRemoteProcessDispatch = false;
-    SendReplyKeyEvent(localEvent);
+
+    MOZ_DIAGNOSTIC_ASSERT(!localEvent.PropagationStopped());
+  }
+  // The keyboard event which we ignore should not be handled in the main
+  // process for shortcut key handling.  For notifying if we skipped it, we can
+  // use "stop propagation" flag here because it must be cleared by
+  // `EventTargetChainItem` if we've dispatched it.
+  else {
+    localEvent.StopPropagation();
   }
 
+  // If we don't need to send a rely for the given keyboard event, we do nothing
+  // anymore here.
+  if (!aEvent.WantReplyFromContentProcess()) {
+    return IPC_OK();
+  }
+
+  // This is an ugly hack, mNoRemoteProcessDispatch is set to true when the
+  // event's PreventDefault() or StopScrollProcessForwarding() is called.
+  // And then, it'll be checked by ParamTraits<mozilla::WidgetEvent>::Write()
+  // whether the event is being sent to remote process unexpectedly.
+  // However, unfortunately, it cannot check the destination.  Therefore,
+  // we need to clear the flag explicitly here because ParamTraits should
+  // keep checking the flag for avoiding regression.
+  localEvent.mFlags.mNoRemoteProcessDispatch = false;
+  SendReplyKeyEvent(localEvent, aUUID);
+
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult BrowserChild::RecvNormalPriorityRealKeyEvent(
-    const WidgetKeyboardEvent& aEvent) {
-  return RecvRealKeyEvent(aEvent);
+    const WidgetKeyboardEvent& aEvent, const nsID& aUUID) {
+  return RecvRealKeyEvent(aEvent, aUUID);
 }
 
 mozilla::ipc::IPCResult BrowserChild::RecvCompositionEvent(
diff --git a/dom/ipc/BrowserChild.h b/dom/ipc/BrowserChild.h
index 2f13bc5998c88..161488020fc1d 100644
--- a/dom/ipc/BrowserChild.h
+++ b/dom/ipc/BrowserChild.h
@@ -352,10 +352,10 @@ class BrowserChild final : public nsMessageManagerScriptExecutor,
                                             nsIContentSecurityPolicy* aCsp);
 
   mozilla::ipc::IPCResult RecvRealKeyEvent(
-      const mozilla::WidgetKeyboardEvent& aEvent);
+      const mozilla::WidgetKeyboardEvent& aEvent, const nsID& aUUID);
 
   mozilla::ipc::IPCResult RecvNormalPriorityRealKeyEvent(
-      const mozilla::WidgetKeyboardEvent& aEvent);
+      const mozilla::WidgetKeyboardEvent& aEvent, const nsID& aUUID);
 
   mozilla::ipc::IPCResult RecvMouseWheelEvent(
       const mozilla::WidgetWheelEvent& aEvent, const ScrollableLayerGuid& aGuid,
diff --git a/dom/ipc/BrowserParent.cpp b/dom/ipc/BrowserParent.cpp
index 7c19df6195b73..096a7cae418c9 100644
--- a/dom/ipc/BrowserParent.cpp
+++ b/dom/ipc/BrowserParent.cpp
@@ -87,6 +87,7 @@
 #include "nsLayoutUtils.h"
 #include "nsQueryActor.h"
 #include "nsSHistory.h"
+#include "nsIUUIDGenerator.h"
 #include "nsViewManager.h"
 #include "nsVariant.h"
 #include "nsIWidget.h"
@@ -2020,13 +2021,24 @@ void BrowserParent::SendRealKeyEvent(WidgetKeyboardEvent& aEvent) {
   } else {
     aEvent.PreventNativeKeyBindings();
   }
-  DebugOnly<bool> ret =
+  SentKeyEventData sendKeyEventData{
+      aEvent.mKeyCode,      aEvent.mCharCode,      aEvent.mPseudoCharCode,
+      aEvent.mKeyNameIndex, aEvent.mCodeNameIndex, aEvent.mModifiers};
+  nsCOMPtr<nsIUUIDGenerator> uuidGen =
+      do_GetService("@mozilla.org/uuid-generator;1");
+  MOZ_RELEASE_ASSERT(uuidGen);
+  MOZ_ALWAYS_SUCCEEDS(uuidGen->GenerateUUIDInPlace(&sendKeyEventData.mUUID));
+  const bool ok =
       Manager()->IsInputPriorityEventEnabled()
-          ? PBrowserParent::SendRealKeyEvent(aEvent)
-          : PBrowserParent::SendNormalPriorityRealKeyEvent(aEvent);
+          ? PBrowserParent::SendRealKeyEvent(aEvent, sendKeyEventData.mUUID)
+          : PBrowserParent::SendNormalPriorityRealKeyEvent(
+                aEvent, sendKeyEventData.mUUID);
 
-  NS_WARNING_ASSERTION(ret, "PBrowserParent::SendRealKeyEvent() failed");
-  MOZ_ASSERT(!ret || aEvent.HasBeenPostedToRemoteProcess());
+  NS_WARNING_ASSERTION(ok, "PBrowserParent::SendRealKeyEvent() failed");
+  MOZ_ASSERT(!ok || aEvent.HasBeenPostedToRemoteProcess());
+  if (ok && aEvent.IsWaitingReplyFromRemoteProcess()) {
+    mWaitingReplyKeyboardEvents.AppendElement(sendKeyEventData);
+  }
 }
 
 void BrowserParent::SendRealTouchEvent(WidgetTouchEvent& aEvent) {
@@ -2636,9 +2648,46 @@ void BrowserParent::StopIMEStateManagement() {
 }
 
 mozilla::ipc::IPCResult BrowserParent::RecvReplyKeyEvent(
-    const WidgetKeyboardEvent& aEvent) {
+    const WidgetKeyboardEvent& aEvent, const nsID& aUUID) {
   NS_ENSURE_TRUE(mFrameElement, IPC_OK());
 
+  // First, verify aEvent is what we've sent to a remote process.
+  Maybe<size_t> index = [&]() -> Maybe<size_t> {
+    for (const size_t i : IntegerRange(mWaitingReplyKeyboardEvents.Length())) {
+      const SentKeyEventData& data = mWaitingReplyKeyboardEvents[i];
+      if (data.mUUID.Equals(aUUID)) {
+        if (NS_WARN_IF(data.mKeyCode != aEvent.mKeyCode) ||
+            NS_WARN_IF(data.mCharCode != aEvent.mCharCode) ||
+            NS_WARN_IF(data.mPseudoCharCode != aEvent.mPseudoCharCode) ||
+            NS_WARN_IF(data.mKeyNameIndex != aEvent.mKeyNameIndex) ||
+            NS_WARN_IF(data.mCodeNameIndex != aEvent.mCodeNameIndex) ||
+            NS_WARN_IF(data.mModifiers != aEvent.mModifiers)) {
+          // Got different event data from what we stored before dispatching an
+          // event with the ID.
+          return Nothing();
+        }
+        return Some(i);
+      }
+    }
+    // No entry found.
+    return Nothing();
+  }();
+  if (MOZ_UNLIKELY(index.isNothing())) {
+    return IPC_FAIL(this, "Bogus reply keyboard event");
+  }
+  // Don't discard the older keyboard events because the order may be changed if
+  // the remote process has a event listener which takes too long time and while
+  // the freezing, user may switch the tab, or if the remote process sends
+  // synchronous XMLHttpRequest.
+  mWaitingReplyKeyboardEvents.RemoveElementAt(*index);
+
+  // If the event propagation was stopped by the child, it means that the event
+  // was ignored in the child.  In the case, we should ignore it too because the
+  // focused web app didn't have a chance to prevent its default.
+  if (aEvent.PropagationStopped()) {
+    return IPC_OK();
+  }
+
   WidgetKeyboardEvent localEvent(aEvent);
   localEvent.MarkAsHandledInRemoteProcess();
 
diff --git a/dom/ipc/BrowserParent.h b/dom/ipc/BrowserParent.h
index 80e4d055e26c0..c05b4322b319e 100644
--- a/dom/ipc/BrowserParent.h
+++ b/dom/ipc/BrowserParent.h
@@ -27,6 +27,7 @@
 #include "nsIDOMEventListener.h"
 #include "nsIRemoteTab.h"
 #include "nsIWidget.h"
+#include "nsTArray.h"
 #include "nsWeakReference.h"
 
 class imgIContainer;
@@ -268,7 +269,8 @@ class BrowserParent final : public PBrowserParent,
 
   mozilla::ipc::IPCResult RecvEvent(const RemoteDOMEvent& aEvent);
 
-  mozilla::ipc::IPCResult RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent);
+  mozilla::ipc::IPCResult RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent,
+                                            const nsID& aUUID);
 
   mozilla::ipc::IPCResult RecvAccessKeyNotHandled(
       const WidgetKeyboardEvent& aEvent);
@@ -888,6 +890,22 @@ class BrowserParent final : public PBrowserParent,
   Maybe<LayoutDeviceToLayoutDeviceMatrix4x4> mChildToParentConversionMatrix;
   Maybe<ScreenRect> mRemoteDocumentRect;
 
+  // mWaitingReplyKeyboardEvents stores keyboard events which are sent from
+  // SendRealKeyEvent and the event will be back as a reply event.  They are
+  // removed when RecvReplyKeyEvent receives corresponding event or newer event.
+  // Note that reply event will be used for handling non-reserved shortcut keys.
+  // Therefore, we need to store only important data for GlobalKeyHandler.
+  struct SentKeyEventData {
+    uint32_t mKeyCode;
+    uint32_t mCharCode;
+    uint32_t mPseudoCharCode;
+    KeyNameIndex mKeyNameIndex;
+    CodeNameIndex mCodeNameIndex;
+    Modifiers mModifiers;
+    nsID mUUID;
+  };
+  nsTArray<SentKeyEventData> mWaitingReplyKeyboardEvents;
+
   nsIntRect mRect;
   ScreenIntSize mDimensions;
   hal::ScreenOrientation mOrientation;
diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
index 9750219fa46ac..4601d583bfcd6 100644
--- a/dom/ipc/PBrowser.ipdl
+++ b/dom/ipc/PBrowser.ipdl
@@ -44,6 +44,7 @@ include "mozilla/layers/LayersMessageUtils.h";
 include "mozilla/ipc/TransportSecurityInfoUtils.h";
 include "mozilla/ipc/URIUtils.h";
 
+using struct nsID from "nsID.h";
 using mozilla::gfx::Matrix4x4 from "mozilla/gfx/Matrix.h";
 using mozilla::gfx::MaybeMatrix4x4 from "mozilla/gfx/Matrix.h";
 using mozilla::gfx::SurfaceFormat from "mozilla/gfx/Types.h";
@@ -463,7 +464,16 @@ parent:
 
     async __delete__();
 
-    async ReplyKeyEvent(WidgetKeyboardEvent event);
+    /**
+     * Send a reply of keyboard event to the parent.  Then, parent can consider
+     * whether the event should kick a shortcut key or ignore.
+     *
+     * @param aEvent    The event which was sent from the parent and handled
+     *                  in a remote process.
+     * @param aUUI      The UUID which was generated when aEvent was sent to
+     *                  a remote process.
+     */
+    async ReplyKeyEvent(WidgetKeyboardEvent aEvent, nsID aUUID);
 
     /**
      * Retrieves edit commands for the key combination represented by aEvent.
@@ -780,9 +790,17 @@ child:
                                                       ScrollableLayerGuid aGuid,
                                                       uint64_t aInputBlockId);
 
+    /**
+     * Send a keyboard event which reporesents a user input to a remote process.
+     *
+     * @param aEvent    The event which user typed a key.
+     * @param aUUID     A UUID which is generated in the parent at sending it.
+     *                  This must be specified when the child sends a reply
+     *                  event to the parent.
+     */
     [Priority=input]
-    async RealKeyEvent(WidgetKeyboardEvent event);
-    async NormalPriorityRealKeyEvent(WidgetKeyboardEvent event);
+    async RealKeyEvent(WidgetKeyboardEvent aEvent, nsID aUUID);
+    async NormalPriorityRealKeyEvent(WidgetKeyboardEvent aEvent, nsID aUUID);
 
     [Priority=input]
     async MouseWheelEvent(WidgetWheelEvent event,
diff --git a/widget/BasicEvents.h b/widget/BasicEvents.h
index 52fc2009f5730..0077194895f9d 100644
--- a/widget/BasicEvents.h
+++ b/widget/BasicEvents.h
@@ -322,10 +322,13 @@ struct BaseEventFlags {
   inline void ResetCrossProcessDispatchingState() {
     MOZ_ASSERT(!IsCrossProcessForwardingStopped());
     mPostedToRemoteProcess = false;
-    // Ignore propagation state in the different process if it's marked as
+    // Ignore propagation state in the remote process if it's marked as
     // "waiting reply from remote process" because the process needs to
     // stop propagation in the process until receiving a reply event.
-    if (IsWaitingReplyFromRemoteProcess()) {
+    // Note that the propagation stopped flag is important for the reply event
+    // handler in the main process because it's used for making whether it's
+    // ignored by the remote process or not.
+    if (!XRE_IsParentProcess() && IsWaitingReplyFromRemoteProcess()) {
       mPropagationStopped = mImmediatePropagationStopped = false;
     }
     // mDispatchedAtLeastOnce indicates the state in current process.

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.


More information about the tbb-commits mailing list