[tor-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 tor-commits
mailing list