- Revision
- 276901
- Author
- [email protected]
- Date
- 2021-05-03 08:58:26 -0700 (Mon, 03 May 2021)
Log Message
[iOS] Crash when tapping fields on cycle.travel
https://bugs.webkit.org/show_bug.cgi?id=225259
<rdar://problem/77386417>
Reviewed by Wenson Hsieh.
Source/WebKit:
The From/To fields on cycle.travel have a click handler that presents
a popover. The popover is presented by changing its display property
from "none" to "block", and contains an input element with the "autofocus"
attribute. After changing the popover element's display property, the
page focuses another element by calling focus().
In WebKit, focusing an element sends an IPC message from the WebProcess
to the UIProcess, so that UIProcess knows of the focused element and
can present the necessary UI. All relevant information about the element
is encoded in a FocusedElementInformation struct, which is populated by
WebPage::getFocusedElementInformation.
Prior to populating the FocusedElementInformation,
getFocusedElementInformation performs a layout if needed,
so that the information sent to the UIProcess is up-to-date. In this
instance, a layout is needed after the popover element's display
property is changed. Then, since the page immediately focuses another
element after the display change, WebKit calls
getFocusedElementInformation prior to the layout being performed.
getFocusedElementInformation then sees that a layout is needed, and
triggers the pending layout.
Now, since since the popover contains an element with "autofocus",
another element is focused as a post-layout task. This means that the
layout triggered by getFocusedElementInformation changes the focused
element. r256401 added an early return in this case, to prevent a null
pointer deref.
An early return in getFocusedElementInformation leaves the passed-in
FocusedElementInformation in an inconsistent state. In particular,
the FocusedElementInformation's elementContext is left with invalid
identifiers. Currently, all callers of getFocusedElementInformation do
not check whether the information is in a valid state. Consequently,
the caller (in this case, WebPage::elementDidFocus) proceeds to send
an IPC message with the FocusedElementInformation as one of the
arguments. Then, a release assert is hit as sending IPC messages with
invalid arguments is unsafe.
To fix, make getFocusedElementInformation return an optional
FocusedElementInformation, rather than populating a parameter that
could be left in an invalid state. Then, any early returns can return
WTF::nullopt, to signify that we were unable to generate a valid
FocusedElementInformation. Finally, whenever the method returns
nullopt, we do not send the IPC message (or in another case, send a
message with nullopt).
Test: fast/forms/focus-change-after-layout-update-during-focus-crash.html
* UIProcess/WebPageProxy.h:
* UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView updateCurrentFocusedElementInformation:]):
* UIProcess/ios/WebPageProxyIOS.mm:
(WebKit::WebPageProxy::requestFocusedElementInformation):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::elementDidFocus):
* WebProcess/WebPage/WebPage.h:
* WebProcess/WebPage/WebPage.messages.in:
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::requestFocusedElementInformation):
(WebKit::WebPage::focusedElementInformation):
LayoutTests:
* fast/forms/focus-change-after-layout-update-during-focus-crash-expected.txt: Added.
* fast/forms/focus-change-after-layout-update-during-focus-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (276900 => 276901)
--- trunk/LayoutTests/ChangeLog 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/LayoutTests/ChangeLog 2021-05-03 15:58:26 UTC (rev 276901)
@@ -1,3 +1,14 @@
+2021-05-03 Aditya Keerthi <[email protected]>
+
+ [iOS] Crash when tapping fields on cycle.travel
+ https://bugs.webkit.org/show_bug.cgi?id=225259
+ <rdar://problem/77386417>
+
+ Reviewed by Wenson Hsieh.
+
+ * fast/forms/focus-change-after-layout-update-during-focus-crash-expected.txt: Added.
+ * fast/forms/focus-change-after-layout-update-during-focus-crash.html: Added.
+
2021-05-03 Chris Lord <[email protected]>
Decoding an SVG off the main thread causes a crash
Added: trunk/LayoutTests/fast/forms/focus-change-after-layout-update-during-focus-crash-expected.txt (0 => 276901)
--- trunk/LayoutTests/fast/forms/focus-change-after-layout-update-during-focus-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/focus-change-after-layout-update-during-focus-crash-expected.txt 2021-05-03 15:58:26 UTC (rev 276901)
@@ -0,0 +1,10 @@
+This test verifies that we don't crash when focusing an element that triggers a layout change, resulting in a change of focus.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Did not crash.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/forms/focus-change-after-layout-update-during-focus-crash.html (0 => 276901)
--- trunk/LayoutTests/fast/forms/focus-change-after-layout-update-during-focus-crash.html (rev 0)
+++ trunk/LayoutTests/fast/forms/focus-change-after-layout-update-during-focus-crash.html 2021-05-03 15:58:26 UTC (rev 276901)
@@ -0,0 +1,31 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <script src=""
+ <script src=""
+</head>
+<body>
+ <input id="input">
+ <input id="hidden" style="display: none" autofocus>
+</body>
+<script>
+jsTestIsAsync = true;
+
+addEventListener("load", async function() {
+ description("This test verifies that we don't crash when focusing an element that triggers a layout change, resulting in a change of focus.");
+ if (!window.testRunner)
+ return;
+
+ input.addEventListener("click", () => {
+ hidden.style.display = "block";
+ input.focus();
+
+ testPassed("Did not crash.");
+ finishJSTest();
+ });
+
+ UIHelper.activateElement(input);
+});
+
+</script>
+</html>
Modified: trunk/Source/WebKit/ChangeLog (276900 => 276901)
--- trunk/Source/WebKit/ChangeLog 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/ChangeLog 2021-05-03 15:58:26 UTC (rev 276901)
@@ -1,3 +1,72 @@
+2021-05-03 Aditya Keerthi <[email protected]>
+
+ [iOS] Crash when tapping fields on cycle.travel
+ https://bugs.webkit.org/show_bug.cgi?id=225259
+ <rdar://problem/77386417>
+
+ Reviewed by Wenson Hsieh.
+
+ The From/To fields on cycle.travel have a click handler that presents
+ a popover. The popover is presented by changing its display property
+ from "none" to "block", and contains an input element with the "autofocus"
+ attribute. After changing the popover element's display property, the
+ page focuses another element by calling focus().
+
+ In WebKit, focusing an element sends an IPC message from the WebProcess
+ to the UIProcess, so that UIProcess knows of the focused element and
+ can present the necessary UI. All relevant information about the element
+ is encoded in a FocusedElementInformation struct, which is populated by
+ WebPage::getFocusedElementInformation.
+
+ Prior to populating the FocusedElementInformation,
+ getFocusedElementInformation performs a layout if needed,
+ so that the information sent to the UIProcess is up-to-date. In this
+ instance, a layout is needed after the popover element's display
+ property is changed. Then, since the page immediately focuses another
+ element after the display change, WebKit calls
+ getFocusedElementInformation prior to the layout being performed.
+ getFocusedElementInformation then sees that a layout is needed, and
+ triggers the pending layout.
+
+ Now, since since the popover contains an element with "autofocus",
+ another element is focused as a post-layout task. This means that the
+ layout triggered by getFocusedElementInformation changes the focused
+ element. r256401 added an early return in this case, to prevent a null
+ pointer deref.
+
+ An early return in getFocusedElementInformation leaves the passed-in
+ FocusedElementInformation in an inconsistent state. In particular,
+ the FocusedElementInformation's elementContext is left with invalid
+ identifiers. Currently, all callers of getFocusedElementInformation do
+ not check whether the information is in a valid state. Consequently,
+ the caller (in this case, WebPage::elementDidFocus) proceeds to send
+ an IPC message with the FocusedElementInformation as one of the
+ arguments. Then, a release assert is hit as sending IPC messages with
+ invalid arguments is unsafe.
+
+ To fix, make getFocusedElementInformation return an optional
+ FocusedElementInformation, rather than populating a parameter that
+ could be left in an invalid state. Then, any early returns can return
+ WTF::nullopt, to signify that we were unable to generate a valid
+ FocusedElementInformation. Finally, whenever the method returns
+ nullopt, we do not send the IPC message (or in another case, send a
+ message with nullopt).
+
+ Test: fast/forms/focus-change-after-layout-update-during-focus-crash.html
+
+ * UIProcess/WebPageProxy.h:
+ * UIProcess/ios/WKContentViewInteraction.mm:
+ (-[WKContentView updateCurrentFocusedElementInformation:]):
+ * UIProcess/ios/WebPageProxyIOS.mm:
+ (WebKit::WebPageProxy::requestFocusedElementInformation):
+ * WebProcess/WebPage/WebPage.cpp:
+ (WebKit::WebPage::elementDidFocus):
+ * WebProcess/WebPage/WebPage.h:
+ * WebProcess/WebPage/WebPage.messages.in:
+ * WebProcess/WebPage/ios/WebPageIOS.mm:
+ (WebKit::WebPage::requestFocusedElementInformation):
+ (WebKit::WebPage::focusedElementInformation):
+
2021-05-03 Miguel Gomez <[email protected]>
[GTK][WPE] Properly reset WebKitWebView's responsiveness flag
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.h (276900 => 276901)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.h 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.h 2021-05-03 15:58:26 UTC (rev 276901)
@@ -1452,7 +1452,7 @@
void recordAutomaticNavigationSnapshot();
void suppressNextAutomaticNavigationSnapshot() { m_shouldSuppressNextAutomaticNavigationSnapshot = true; }
void recordNavigationSnapshot(WebBackForwardListItem&);
- void requestFocusedElementInformation(CompletionHandler<void(const FocusedElementInformation&)>&&);
+ void requestFocusedElementInformation(CompletionHandler<void(const Optional<FocusedElementInformation>&)>&&);
#if PLATFORM(COCOA) || PLATFORM(GTK)
RefPtr<ViewSnapshot> takeViewSnapshot(Optional<WebCore::IntRect>&&);
Modified: trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (276900 => 276901)
--- trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2021-05-03 15:58:26 UTC (rev 276901)
@@ -6532,13 +6532,13 @@
WeakObjCPtr<WKContentView> weakSelf { self };
auto identifierBeforeUpdate = _focusedElementInformation.focusedElementIdentifier;
_page->requestFocusedElementInformation([callback = WTFMove(callback), identifierBeforeUpdate, weakSelf] (auto& info) {
- if (!weakSelf || info.focusedElementIdentifier != identifierBeforeUpdate) {
+ if (!weakSelf || !info || info->focusedElementIdentifier != identifierBeforeUpdate) {
// If the focused element may have changed in the meantime, don't overwrite focused element information.
callback(false);
return;
}
- weakSelf.get()->_focusedElementInformation = info;
+ weakSelf.get()->_focusedElementInformation = info.value();
callback(true);
});
}
Modified: trunk/Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm (276900 => 276901)
--- trunk/Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm 2021-05-03 15:58:26 UTC (rev 276901)
@@ -149,7 +149,7 @@
return false;
}
-void WebPageProxy::requestFocusedElementInformation(CompletionHandler<void(const FocusedElementInformation&)>&& callback)
+void WebPageProxy::requestFocusedElementInformation(CompletionHandler<void(const Optional<FocusedElementInformation>&)>&& callback)
{
if (!hasRunningProcess())
return callback({ });
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp (276900 => 276901)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2021-05-03 15:58:26 UTC (rev 276901)
@@ -5946,13 +5946,15 @@
#endif
++m_currentFocusedElementIdentifier;
- FocusedElementInformation information;
- getFocusedElementInformation(information);
+ auto information = focusedElementInformation();
+ if (!information)
+ return;
+
RefPtr<API::Object> userData;
m_formClient->willBeginInputSession(this, &element, WebFrame::fromCoreFrame(*element.document().frame()), m_userIsInteracting, userData);
- send(Messages::WebPageProxy::ElementDidFocus(information, m_userIsInteracting, m_recentlyBlurredElement, m_lastActivityStateChanges, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+ send(Messages::WebPageProxy::ElementDidFocus(information.value(), m_userIsInteracting, m_recentlyBlurredElement, m_lastActivityStateChanges, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
#elif PLATFORM(MAC)
// FIXME: This can be unified with the iOS code above by bringing ElementDidFocus to macOS.
// This also doesn't take other noneditable controls into account, such as input type color.
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.h (276900 => 276901)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2021-05-03 15:58:26 UTC (rev 276901)
@@ -744,7 +744,7 @@
void inspectorNodeSearchEndedAtPosition(const WebCore::FloatPoint&);
void blurFocusedElement();
- void requestFocusedElementInformation(CompletionHandler<void(const FocusedElementInformation&)>&&);
+ void requestFocusedElementInformation(CompletionHandler<void(const Optional<FocusedElementInformation>&)>&&);
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>)>&&);
@@ -1477,7 +1477,7 @@
#if PLATFORM(IOS_FAMILY)
void updateViewportSizeForCSSViewportUnits();
- void getFocusedElementInformation(FocusedElementInformation&);
+ Optional<FocusedElementInformation> focusedElementInformation();
void platformInitializeAccessibility();
void generateSyntheticEditingCommand(SyntheticEditingCommandType);
void handleSyntheticClick(WebCore::Node& nodeRespondingToClick, const WebCore::FloatPoint& location, OptionSet<WebKit::WebEvent::Modifier>, WebCore::PointerID = WebCore::mousePointerID);
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in (276900 => 276901)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in 2021-05-03 15:58:26 UTC (rev 276901)
@@ -113,7 +113,7 @@
StoreSelectionForAccessibility(bool shouldStore)
StartAutoscrollAtPosition(WebCore::FloatPoint positionInWindow)
CancelAutoscroll()
- RequestFocusedElementInformation() -> (struct WebKit::FocusedElementInformation info) Async
+ RequestFocusedElementInformation() -> (Optional<WebKit::FocusedElementInformation> info) Async
HardwareKeyboardAvailabilityChanged(bool keyboardIsAttached)
SetIsShowingInputViewForFocusedElement(bool showingInputView)
UpdateSelectionWithDelta(int64_t locationDelta, int64_t lengthDelta) -> () Async
Modified: trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm (276900 => 276901)
--- trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2021-05-03 15:54:53 UTC (rev 276900)
+++ trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2021-05-03 15:58:26 UTC (rev 276901)
@@ -954,13 +954,13 @@
nodeRespondingToDoubleClick->document().frame()->eventHandler().handleMouseReleaseEvent(PlatformMouseEvent(roundedAdjustedPoint, roundedAdjustedPoint, LeftButton, PlatformEvent::MouseReleased, 2, shiftKey, ctrlKey, altKey, metaKey, WallTime::now(), 0, WebCore::OneFingerTap));
}
-void WebPage::requestFocusedElementInformation(CompletionHandler<void(const FocusedElementInformation&)>&& completionHandler)
+void WebPage::requestFocusedElementInformation(CompletionHandler<void(const Optional<FocusedElementInformation>&)>&& completionHandler)
{
- FocusedElementInformation info;
+ Optional<FocusedElementInformation> information;
if (m_focusedElement)
- getFocusedElementInformation(info);
+ information = focusedElementInformation();
- completionHandler(info);
+ completionHandler(information);
}
#if ENABLE(DRAG_SUPPORT)
@@ -3145,11 +3145,11 @@
completionHandler();
}
-void WebPage::getFocusedElementInformation(FocusedElementInformation& information)
+Optional<FocusedElementInformation> WebPage::focusedElementInformation()
{
RefPtr<Document> document = m_page->focusController().focusedOrMainFrame().document();
if (!document || !document->view())
- return;
+ return WTF::nullopt;
auto focusedElement = m_focusedElement.copyRef();
bool willLayout = document->view()->needsLayout();
@@ -3157,7 +3157,7 @@
// Layout may have detached the document or caused a change of focus.
if (!document->view() || focusedElement != m_focusedElement)
- return;
+ return WTF::nullopt;
if (willLayout)
sendEditorStateUpdate();
@@ -3164,6 +3164,8 @@
else
scheduleFullEditorStateUpdate();
+ FocusedElementInformation information;
+
information.lastInteractionLocation = m_lastInteractionLocation;
if (auto elementContext = contextForElement(*focusedElement))
information.elementContext = WTFMove(*elementContext);
@@ -3349,6 +3351,8 @@
information.shouldAvoidResizingWhenInputViewBoundsChange = quirks.shouldAvoidResizingWhenInputViewBoundsChange();
information.shouldAvoidScrollingWhenFocusedContentIsVisible = quirks.shouldAvoidScrollingWhenFocusedContentIsVisible();
information.shouldUseLegacySelectPopoverDismissalBehaviorInDataActivation = quirks.shouldUseLegacySelectPopoverDismissalBehaviorInDataActivation();
+
+ return information;
}
void WebPage::autofillLoginCredentials(const String& username, const String& password)
