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@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 ParamTraitsmozilla::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 ParamTraitsmozilla::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.