From 1078a6796e5a9158a096cf3b9677e62d57905bed Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Fri, 20 Sep 2024 08:12:33 +0000 Subject: [PATCH] Bug 1863611 Make `EventStateManager` send content command events to focused remote process r=smaug Currently, `EventStateManager` handles some content commands only in the parent process. Therefore, if `eSetSelection` event or something is being handled in the focused remote process, the command may be disabled yet. Avoiding the race conditions, we should handle the content command events in the focused process. Differential Revision: https://phabricator.services.mozilla.com/D222789 --- dom/events/EventStateManager.cpp | 39 +++++++++++-- dom/ipc/BrowserChild.cpp | 14 +++++ dom/ipc/BrowserChild.h | 6 ++ dom/ipc/BrowserParent.cpp | 42 +++++++++++++- dom/ipc/BrowserParent.h | 5 +- dom/ipc/PBrowser.ipdl | 4 ++ widget/ContentCache.cpp | 38 +++++++++++++ widget/ContentCache.h | 13 ++++- widget/EventMessageList.h | 2 + widget/MiscEvents.h | 11 ++++ widget/tests/browser/browser.toml | 2 + ...tCommand_immediately_after_setSelection.js | 55 +++++++++++++++++++ 12 files changed, 220 insertions(+), 11 deletions(-) create mode 100644 widget/tests/browser/browser_test_contentCommand_immediately_after_setSelection.js diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp index c371596b09ba..dd4e43a41e5b 100644 --- a/dom/events/EventStateManager.cpp +++ b/dom/events/EventStateManager.cpp @@ -6747,24 +6747,31 @@ nsresult EventStateManager::DoContentCommandEvent( nsCOMPtr root = window->GetTopWindowRoot(); NS_ENSURE_TRUE(root, NS_ERROR_FAILURE); const char* cmd; + bool maybeNeedToHandleInRemote = false; switch (aEvent->mMessage) { case eContentCommandCut: cmd = "cmd_cut"; + maybeNeedToHandleInRemote = true; break; case eContentCommandCopy: cmd = "cmd_copy"; + maybeNeedToHandleInRemote = true; break; case eContentCommandPaste: cmd = "cmd_paste"; + maybeNeedToHandleInRemote = true; break; case eContentCommandDelete: cmd = "cmd_delete"; + maybeNeedToHandleInRemote = true; break; case eContentCommandUndo: cmd = "cmd_undo"; + maybeNeedToHandleInRemote = true; break; case eContentCommandRedo: cmd = "cmd_redo"; + maybeNeedToHandleInRemote = true; break; case eContentCommandPasteTransferable: cmd = "cmd_pasteTransferable"; @@ -6775,6 +6782,20 @@ nsresult EventStateManager::DoContentCommandEvent( default: return NS_ERROR_NOT_IMPLEMENTED; } + if (XRE_IsParentProcess() && maybeNeedToHandleInRemote) { + if (BrowserParent* remote = BrowserParent::GetFocused()) { + if (!aEvent->mOnlyEnabledCheck) { + remote->SendSimpleContentCommandEvent(*aEvent); + } + // XXX The command may be disabled in the parent process. Perhaps, we + // should set actual enabled state in the parent process here and there + // should be another bool flag which indicates whether the content is sent + // to a remote process. + aEvent->mIsEnabled = true; + aEvent->mSucceeded = true; + return NS_OK; + } + } // If user tries to do something, user must try to do it in visible window. // So, let's retrieve controller of visible window. nsCOMPtr controller; @@ -6864,8 +6885,12 @@ nsresult EventStateManager::DoContentCommandInsertTextEvent( if (XRE_IsParentProcess()) { // Handle it in focused content process if there is. if (BrowserParent* remote = BrowserParent::GetFocused()) { - remote->SendInsertText(aEvent->mString.ref()); - aEvent->mIsEnabled = true; // XXX it can be a lie... + if (!aEvent->mOnlyEnabledCheck) { + remote->SendInsertText(*aEvent); + } + // XXX The remote process may be not editable right now. Therefore, this + // may be different from actual state in the remote process. + aEvent->mIsEnabled = true; aEvent->mSucceeded = true; return NS_OK; } @@ -6901,10 +6926,12 @@ nsresult EventStateManager::DoContentCommandReplaceTextEvent( if (XRE_IsParentProcess()) { // Handle it in focused content process if there is. if (BrowserParent* remote = BrowserParent::GetFocused()) { - Unused << remote->SendReplaceText( - aEvent->mSelection.mReplaceSrcString, aEvent->mString.ref(), - aEvent->mSelection.mOffset, aEvent->mSelection.mPreventSetSelection); - aEvent->mIsEnabled = true; // XXX it can be a lie... + if (!aEvent->mOnlyEnabledCheck) { + Unused << remote->SendReplaceText(*aEvent); + } + // XXX The remote process may be not editable right now. Therefore, this + // may be different from actual state in the remote process. + aEvent->mIsEnabled = true; aEvent->mSucceeded = true; return NS_OK; } diff --git a/dom/ipc/BrowserChild.cpp b/dom/ipc/BrowserChild.cpp index e67867d6f69d..0cd7c975f4af 100644 --- a/dom/ipc/BrowserChild.cpp +++ b/dom/ipc/BrowserChild.cpp @@ -2312,6 +2312,20 @@ mozilla::ipc::IPCResult BrowserChild::RecvNormalPrioritySelectionEvent( return RecvSelectionEvent(aEvent); } +mozilla::ipc::IPCResult BrowserChild::RecvSimpleContentCommandEvent( + const EventMessage& aMessage) { + WidgetContentCommandEvent localEvent(true, aMessage, mPuppetWidget); + DispatchWidgetEventViaAPZ(localEvent); + Unused << SendOnEventNeedingAckHandled(aMessage, 0u); + return IPC_OK(); +} + +mozilla::ipc::IPCResult +BrowserChild::RecvNormalPrioritySimpleContentCommandEvent( + const EventMessage& aMessage) { + return RecvSimpleContentCommandEvent(aMessage); +} + mozilla::ipc::IPCResult BrowserChild::RecvInsertText( const nsAString& aStringToInsert) { // Use normal event path to reach focused document. diff --git a/dom/ipc/BrowserChild.h b/dom/ipc/BrowserChild.h index 143bc14c7364..94d7987fbbb7 100644 --- a/dom/ipc/BrowserChild.h +++ b/dom/ipc/BrowserChild.h @@ -400,6 +400,12 @@ class BrowserChild final : public nsMessageManagerScriptExecutor, mozilla::ipc::IPCResult RecvNormalPrioritySelectionEvent( const mozilla::WidgetSelectionEvent& aEvent); + mozilla::ipc::IPCResult RecvSimpleContentCommandEvent( + const mozilla::EventMessage& aMessage); + + mozilla::ipc::IPCResult RecvNormalPrioritySimpleContentCommandEvent( + const mozilla::EventMessage& aMessage); + mozilla::ipc::IPCResult RecvInsertText(const nsAString& aStringToInsert); mozilla::ipc::IPCResult RecvUpdateRemoteStyle( diff --git a/dom/ipc/BrowserParent.cpp b/dom/ipc/BrowserParent.cpp index f00946e8a935..2cb0e32fdd11 100644 --- a/dom/ipc/BrowserParent.cpp +++ b/dom/ipc/BrowserParent.cpp @@ -3294,13 +3294,49 @@ bool BrowserParent::SendSelectionEvent(WidgetSelectionEvent& aEvent) { return true; } -bool BrowserParent::SendInsertText(const nsString& aStringToInsert) { +bool BrowserParent::SendSimpleContentCommandEvent( + const mozilla::WidgetContentCommandEvent& aEvent) { + MOZ_ASSERT(aEvent.mMessage != eContentCommandInsertText); + MOZ_ASSERT(aEvent.mMessage != eContentCommandReplaceText); + MOZ_ASSERT(aEvent.mMessage != eContentCommandPasteTransferable); + MOZ_ASSERT(aEvent.mMessage != eContentCommandLookUpDictionary); + MOZ_ASSERT(aEvent.mMessage != eContentCommandScroll); + if (mIsDestroyed) { return false; } + mContentCache.OnContentCommandEvent(aEvent); return Manager()->IsInputPriorityEventEnabled() - ? PBrowserParent::SendInsertText(aStringToInsert) - : PBrowserParent::SendNormalPriorityInsertText(aStringToInsert); + ? PBrowserParent::SendSimpleContentCommandEvent(aEvent.mMessage) + : PBrowserParent::SendNormalPrioritySimpleContentCommandEvent( + aEvent.mMessage); +} + +bool BrowserParent::SendInsertText(const WidgetContentCommandEvent& aEvent) { + if (mIsDestroyed) { + return false; + } + mContentCache.OnContentCommandEvent(aEvent); + return Manager()->IsInputPriorityEventEnabled() + ? PBrowserParent::SendInsertText(aEvent.mString.ref()) + : PBrowserParent::SendNormalPriorityInsertText( + aEvent.mString.ref()); +} + +bool BrowserParent::SendReplaceText(const WidgetContentCommandEvent& aEvent) { + if (mIsDestroyed) { + return false; + } + mContentCache.OnContentCommandEvent(aEvent); + return Manager()->IsInputPriorityEventEnabled() + ? PBrowserParent::SendReplaceText( + aEvent.mSelection.mReplaceSrcString, aEvent.mString.ref(), + aEvent.mSelection.mOffset, + aEvent.mSelection.mPreventSetSelection) + : PBrowserParent::SendNormalPriorityReplaceText( + aEvent.mSelection.mReplaceSrcString, aEvent.mString.ref(), + aEvent.mSelection.mOffset, + aEvent.mSelection.mPreventSetSelection); } bool BrowserParent::SendPasteTransferable(IPCTransferable&& aTransferable) { diff --git a/dom/ipc/BrowserParent.h b/dom/ipc/BrowserParent.h index 38f24d5767a7..58dee684f464 100644 --- a/dom/ipc/BrowserParent.h +++ b/dom/ipc/BrowserParent.h @@ -613,7 +613,10 @@ class BrowserParent final : public PBrowserParent, bool HandleQueryContentEvent(mozilla::WidgetQueryContentEvent& aEvent); - bool SendInsertText(const nsString& aStringToInsert); + bool SendSimpleContentCommandEvent( + const mozilla::WidgetContentCommandEvent& aEvent); + bool SendInsertText(const mozilla::WidgetContentCommandEvent& aEvent); + bool SendReplaceText(const mozilla::WidgetContentCommandEvent& aEvent); bool SendPasteTransferable(IPCTransferable&& aTransferable); diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl index d8deff3ad6c7..712883d796ff 100644 --- a/dom/ipc/PBrowser.ipdl +++ b/dom/ipc/PBrowser.ipdl @@ -849,6 +849,10 @@ child: [Priority=input] async SelectionEvent(WidgetSelectionEvent event); async NormalPrioritySelectionEvent(WidgetSelectionEvent event); + + [Priority=input] async SimpleContentCommandEvent(EventMessage message); + async NormalPrioritySimpleContentCommandEvent(EventMessage message); + /** * Dispatch eContentCommandInsertText event in the remote process. */ diff --git a/widget/ContentCache.cpp b/widget/ContentCache.cpp index b6093fb3f78f..c2c62ef1336d 100644 --- a/widget/ContentCache.cpp +++ b/widget/ContentCache.cpp @@ -16,6 +16,7 @@ #include "mozilla/IMEStateManager.h" #include "mozilla/IntegerPrintfMacros.h" #include "mozilla/Logging.h" +#include "mozilla/MiscEvents.h" #include "mozilla/RefPtr.h" #include "mozilla/TextComposition.h" #include "mozilla/dom/BrowserParent.h" @@ -1419,6 +1420,28 @@ void ContentCacheInParent::OnSelectionEvent( mPendingSetSelectionEventNeedingAck++; } +void ContentCacheInParent::OnContentCommandEvent( + const WidgetContentCommandEvent& aContentCommandEvent) { + MOZ_LOG(sContentCacheLog, LogLevel::Info, + ("0x%p OnContentCommandEvent(aEvent={ " + "mMessage=%s, mString=\"%s\", mSelection={ mReplaceSrcString=\"%s\" " + "mOffset=%u, mPreventSetSelection=%s }, mOnlyEnabledCheck=%s })", + this, ToChar(aContentCommandEvent.mMessage), + ToString(aContentCommandEvent.mString).c_str(), + ToString(aContentCommandEvent.mSelection.mReplaceSrcString).c_str(), + aContentCommandEvent.mSelection.mOffset, + GetBoolName(aContentCommandEvent.mSelection.mPreventSetSelection), + GetBoolName(aContentCommandEvent.mOnlyEnabledCheck))); + + MOZ_ASSERT(!aContentCommandEvent.mOnlyEnabledCheck); + +#if MOZ_DIAGNOSTIC_ASSERT_ENABLED && !defined(FUZZING_SNAPSHOT) + mDispatchedEventMessages.AppendElement(aContentCommandEvent.mMessage); +#endif // MOZ_DIAGNOSTIC_ASSERT_ENABLED + + mPendingContentCommandEventNeedingAck++; +} + void ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget* aWidget, EventMessage aMessage, uint32_t aCompositionId) { @@ -1588,6 +1611,21 @@ void ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget* aWidget, } else { mPendingSetSelectionEventNeedingAck--; } + } else if (aMessage >= eContentCommandEventFirst && + aMessage <= eContentCommandEventLast) { + if (NS_WARN_IF(!mPendingContentCommandEventNeedingAck)) { +#if MOZ_DIAGNOSTIC_ASSERT_ENABLED && !defined(FUZZING_SNAPSHOT) + nsAutoCString info( + "\nThere is no pending content command events but received from the " + "remote child\n\n"); + AppendEventMessageLog(info); + CrashReporter::AppendAppNotesToCrashReport(info); + MOZ_DIAGNOSTIC_ASSERT( + false, "No pending event message but received unexpected event"); +#endif // #if MOZ_DIAGNOSTIC_ASSERT_ENABLED + } else { + mPendingContentCommandEventNeedingAck--; + } } if (!PendingEventsNeedingAck()) { diff --git a/widget/ContentCache.h b/widget/ContentCache.h index 3f8b48425ede..a5d2962c4a27 100644 --- a/widget/ContentCache.h +++ b/widget/ContentCache.h @@ -423,6 +423,13 @@ class ContentCacheInParent final : public ContentCache { */ void OnSelectionEvent(const WidgetSelectionEvent& aSelectionEvent); + /** + * OnContentCommandEvent() should called before sending a content command + * event. + */ + void OnContentCommandEvent( + const WidgetContentCommandEvent& aContentCommandEvent); + /** * OnEventNeedingAckHandled() should be called after the child process * handles a sent event which needs acknowledging. @@ -489,7 +496,8 @@ class ContentCacheInParent final : public ContentCache { // sent to the remote process, but we've not verified that the remote process // finished handling it. [[nodiscard]] uint32_t PendingEventsNeedingAck() const { - uint32_t ret = mPendingSetSelectionEventNeedingAck; + uint32_t ret = mPendingSetSelectionEventNeedingAck + + mPendingContentCommandEventNeedingAck; for (const HandlingCompositionData& data : mHandlingCompositions) { ret += data.mPendingEventsNeedingAck; } @@ -599,6 +607,9 @@ class ContentCacheInParent final : public ContentCache { // process finished handling the events. Note that eSetSelection may be // dispatched without composition. Therefore, we need to count it with this. uint32_t mPendingSetSelectionEventNeedingAck = 0u; + // Increased when sending WidgetContentCommandEvent events and decreased when + // the remote process finished handling them. + uint32_t mPendingContentCommandEventNeedingAck = 0u; // mPendingCommitLength is commit string length of the first pending // composition. This is used by relative offset query events when querying // new composition start offset. diff --git a/widget/EventMessageList.h b/widget/EventMessageList.h index 7d7402f89300..e73f9604ad07 100644 --- a/widget/EventMessageList.h +++ b/widget/EventMessageList.h @@ -358,6 +358,8 @@ NS_EVENT_MESSAGE(eContentCommandLookUpDictionary) // scrollable ancestor element can only be scrolled vertically, and horizontal // scrolling is requested using this event, no scrolling will occur. NS_EVENT_MESSAGE(eContentCommandScroll) +NS_EVENT_MESSAGE_FIRST_LAST(eContentCommandEvent, eContentCommandCut, + eContentCommandScroll) // Event to gesture notification NS_EVENT_MESSAGE(eGestureNotify) diff --git a/widget/MiscEvents.h b/widget/MiscEvents.h index 685c4260215c..6fbfb2eacdee 100644 --- a/widget/MiscEvents.h +++ b/widget/MiscEvents.h @@ -78,9 +78,20 @@ class WidgetContentCommandEvent : public WidgetGUIEvent { bool mPreventSetSelection = false; // [in] } mSelection; + // If set to true, the event checks whether the command is enabled in the + // process or not without executing the command. I.e., if it's in the parent + // process when a remote process has focus, mIsEnabled may be different from + // the latest state of the command in the remote process. bool mOnlyEnabledCheck; // [in] bool mSucceeded; // [out] + + // If the command is enabled, set to true. Otherwise, false. This is set + // synchronously in the process. If it's in the parent process when a remote + // process has focus, this returns the command state in the parent process + // which may be different from the remote process. + // XXX When mOnlyEnabledCheck is set to true, this may be always set to true + // even when the command is disabled in the parent process. bool mIsEnabled; // [out] void AssignContentCommandEventData(const WidgetContentCommandEvent& aEvent, diff --git a/widget/tests/browser/browser.toml b/widget/tests/browser/browser.toml index 0224d3f6bd0e..aec117580403 100644 --- a/widget/tests/browser/browser.toml +++ b/widget/tests/browser/browser.toml @@ -19,6 +19,8 @@ skip-if = [ ["browser_test_consumed_alt_char_on_windows.js"] skip-if = ["os != 'win'"] +["browser_test_contentCommand_immediately_after_setSelection.js"] + ["browser_test_fullscreen_size.js"] ["browser_test_ime_state_after_body_removed_and_reconnected_in_designMode.js"] diff --git a/widget/tests/browser/browser_test_contentCommand_immediately_after_setSelection.js b/widget/tests/browser/browser_test_contentCommand_immediately_after_setSelection.js new file mode 100644 index 000000000000..9c6229ffb946 --- /dev/null +++ b/widget/tests/browser/browser_test_contentCommand_immediately_after_setSelection.js @@ -0,0 +1,55 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +add_task(async function () { + await BrowserTestUtils.withNewTab( + "data:text/html,", + async function (browser) { + await SimpleTest.promiseFocus(browser); + + for (const test of [ + { command: "cut" }, + { command: "delete" }, + { command: "insertText", data: "B" }, + ]) { + // ab[]c + await SpecialPowers.spawn(browser, [], () => { + const textarea = content.document.querySelector("textarea"); + textarea.value = "abc"; + textarea.focus(); + textarea.selectionStart = "ab".length; + textarea.selectionEnd = textarea.selectionStart; + content.wrappedJSObject.promiseInput = new Promise(resolve => + textarea.addEventListener("input", () => resolve(textarea.value), { + once: true, + }) + ); + }); + + // a[b]c + window.windowUtils.sendSelectionSetEvent("a".length, "b".length); + // a[]c or a[B]c + window.windowUtils.sendContentCommandEvent( + test.command, + null, + test.data + ); + + info("Waiting for input event on