REGRESSION (271805@main): [iOS] Updating options in a visible <select…

…> menu can result in hangs https://bugs.webkit.org/show_bug.cgi?id=271138 rdar://124576235 Reviewed by Abrar Rahman Protyasha. 271805@main introduced logic that would update a visible <select> menu's options as they are changed. Currently, the logic sends an update every time an option is added or removed. This is problematic when options are added in a loop, as (n - 1) unnecessary IPC messages are sent, and O(n^2) menu items are constructed in the UI process. Additionally, there is further overhead from auto layout. Consequently, adding options in a loop can result in a hang when there is a visible <select> menu. Fix by adding a debouncing mechanism, so that changes to options can be coalesced into a single update. * LayoutTests/fast/forms/ios/select-option-removed-update.html: * LayoutTests/fast/forms/ios/select-option-update-1000-expected.txt: Added. * LayoutTests/fast/forms/ios/select-option-update-1000.html: Added. * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::close): (WebKit::WebPage::elementDidFocus): (WebKit::WebPage::focusedSelectElementDidChangeOptions): * Source/WebKit/WebProcess/WebPage/WebPage.h: Use a `DeferrableOneShotTimer` to ensure that any changes that happen within 100ms are coalesced into a single update. * Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm: (WebKit::WebPage::updateFocusedElementInformation): Canonical link: https://commits.webkit.org/276349@main
WebKit · Mar 19, 2024 · cfd8698 · cfd8698
1 parent 74283d1
commit cfd8698
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 6 deletions.
diff --git a/LayoutTests/fast/forms/ios/select-option-removed-update.html b/LayoutTests/fast/forms/ios/select-option-removed-update.html
@@ -25,7 +25,7 @@
     shouldBeTrue("areArraysEqual(items, " + '["Option 1", "Option 2", "Option 3"]' + ")");
 
     select.remove(0);
-    await UIHelper.ensurePresentationUpdate();
+    await UIHelper.delayFor(200);
 
     items = await UIHelper.selectMenuItems();
     shouldBeTrue("areArraysEqual(items, " + '["Option 2", "Option 3"]' + ")");

diff --git a/LayoutTests/fast/forms/ios/select-option-update-1000-expected.txt b/LayoutTests/fast/forms/ios/select-option-update-1000-expected.txt
@@ -0,0 +1,11 @@
+This test verifies that updating a <select> element's menu while it is visible does not result in a hang.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS items.length is 1
+PASS items.length is 1001
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/forms/ios/select-option-update-1000.html b/LayoutTests/fast/forms/ios/select-option-update-1000.html
@@ -0,0 +1,41 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+        <script src="../../../resources/js-test.js"></script>
+        <script src="../../../resources/ui-helper.js"></script>
+    </head>
+<body>
+<select id="select">
+    <option>1</option>
+</select>
+</body>
+<script>
+jsTestIsAsync = true;
+
+addEventListener("load", async () => {
+    description("This test verifies that updating a &lt;select&gt; element's menu while it is visible does not result in a hang.");
+
+    await UIHelper.activateElement(select);
+
+    items = await UIHelper.selectMenuItems();
+    shouldBeEqualToNumber("items.length", 1);
+
+    for (let i = 1; i <= 1000; i++) {
+        let option = document.createElement("option");
+        option.text = i + 1;
+        select.appendChild(option);
+    }
+
+    await UIHelper.delayFor(200);
+
+    items = await UIHelper.selectMenuItems();
+    shouldBeEqualToNumber("items.length", 1001);
+
+    await UIHelper.selectFormAccessoryPickerRow(1);
+    await UIHelper.waitForContextMenuToHide();
+
+    finishJSTest();
+});
+</script>
+</html>
diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
@@ -453,6 +453,10 @@ static const Seconds pageScrollHysteresisDuration { 300_ms };
 static const Seconds initialLayerVolatilityTimerInterval { 20_ms };
 static const Seconds maximumLayerVolatilityTimerInterval { 2_s };
 
+#if PLATFORM(IOS_FAMILY)
+static constexpr Seconds updateFocusedElementInformationDebounceInterval { 100_ms };
+#endif
+
 #define WEBPAGE_RELEASE_LOG(channel, fmt, ...) RELEASE_LOG(channel, "%p - [webPageID=%" PRIu64 "] WebPage::" fmt, this, m_identifier.toUInt64(), ##__VA_ARGS__)
 #define WEBPAGE_RELEASE_LOG_ERROR(channel, fmt, ...) RELEASE_LOG_ERROR(channel, "%p - [webPageID=%" PRIu64 "] WebPage::" fmt, this, m_identifier.toUInt64(), ##__VA_ARGS__)
 
@@ -581,6 +585,7 @@ WebPage::WebPage(PageIdentifier pageID, WebPageCreationParameters&& parameters)
     , m_deviceOrientation(parameters.deviceOrientation)
     , m_keyboardIsAttached(parameters.keyboardIsAttached)
     , m_canShowWhileLocked(parameters.canShowWhileLocked)
+    , m_updateFocusedElementInformationTimer(*this, &WebPage::updateFocusedElementInformation, updateFocusedElementInformationDebounceInterval)
 #endif
     , m_layerVolatilityTimer(*this, &WebPage::layerVolatilityTimerFired)
     , m_activityState(parameters.activityState)
@@ -1852,6 +1857,10 @@ void WebPage::close()
     m_textAutoSizingAdjustmentTimer.stop();
 #endif
 
+#if PLATFORM(IOS_FAMILY)
+    m_updateFocusedElementInformationTimer.stop();
+#endif
+
 #if ENABLE(CONTEXT_MENUS)
     m_contextMenuClient = makeUnique<API::InjectedBundle::PageContextMenuClient>();
 #endif
@@ -7077,6 +7086,10 @@ static bool isTextFormControlOrEditableContent(const WebCore::Element& element)
 
 void WebPage::elementDidFocus(Element& element, const FocusOptions& options)
 {
+#if PLATFORM(IOS_FAMILY)
+    m_updateFocusedElementInformationTimer.stop();
+#endif
+
     if (!shouldDispatchUpdateAfterFocusingElement(element)) {
         updateInputContextAfterBlurringAndRefocusingElementIfNeeded(element);
         m_focusedElement = &element;
@@ -7164,11 +7177,7 @@ void WebPage::focusedSelectElementDidChangeOptions(const WebCore::HTMLSelectElem
     if (m_focusedElement != &element)
         return;
 
-    auto information = focusedElementInformation();
-    if (!information)
-        return;
-
-    send(Messages::WebPageProxy::UpdateFocusedElementInformation(*information));
+    m_updateFocusedElementInformationTimer.restart();
 #else
     UNUSED_PARAM(element);
 #endif

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h
@@ -84,6 +84,7 @@
 #include <WebCore/SubstituteData.h>
 #include <WebCore/TextManipulationController.h>
 #include <WebCore/TextManipulationItem.h>
+#include <WebCore/Timer.h>
 #include <WebCore/UserActivity.h>
 #include <WebCore/UserMediaRequestIdentifier.h>
 #include <WebCore/UserScriptTypes.h>
@@ -892,6 +893,7 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
 
     void blurFocusedElement();
     void requestFocusedElementInformation(CompletionHandler<void(const std::optional<FocusedElementInformation>&)>&&);
+    void updateFocusedElementInformation();
     void selectWithGesture(const WebCore::IntPoint&, GestureType, GestureRecognizerState, bool isInteractingWithFocusedElement, CompletionHandler<void(const WebCore::IntPoint&, GestureType, GestureRecognizerState, OptionSet<SelectionFlags>)>&&);
     void updateSelectionWithTouches(const WebCore::IntPoint&, SelectionTouch, bool baseIsStart, CompletionHandler<void(const WebCore::IntPoint&, SelectionTouch, OptionSet<SelectionFlags>)>&&);
     void selectWithTwoTouches(const WebCore::IntPoint& from, const WebCore::IntPoint& to, GestureType, GestureRecognizerState, CompletionHandler<void(const WebCore::IntPoint&, GestureType, GestureRecognizerState, OptionSet<SelectionFlags>)>&&);
@@ -2573,6 +2575,8 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
     double m_lastTransactionPageScaleFactor { 0 };
     TransactionID m_lastTransactionIDWithScaleChange;
 
+    WebCore::DeferrableOneShotTimer m_updateFocusedElementInformationTimer;
+
     CompletionHandler<void(InteractionInformationAtPosition&&)> m_pendingSynchronousPositionInformationReply;
     std::optional<std::pair<TransactionID, double>> m_lastLayerTreeTransactionIdAndPageScaleBeforeScalingPage;
     bool m_sendAutocorrectionContextAfterFocusingElement { false };

diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
@@ -1015,6 +1015,20 @@ static void dispatchSyntheticMouseMove(LocalFrame& mainFrame, const WebCore::Flo
     completionHandler(information);
 }
 
+void WebPage::updateFocusedElementInformation()
+{
+    m_updateFocusedElementInformationTimer.stop();
+
+    if (!m_focusedElement)
+        return;
+
+    auto information = focusedElementInformation();
+    if (!information)
+        return;
+
+    send(Messages::WebPageProxy::UpdateFocusedElementInformation(*information));
+}
+
 #if ENABLE(DRAG_SUPPORT)
 void WebPage::requestDragStart(const IntPoint& clientPosition, const IntPoint& globalPosition, OptionSet<WebCore::DragSourceAction> allowedActionsMask)
 {