- Revision
- 264376
- Author
- wenson_hs...@apple.com
- Date
- 2020-07-14 14:39:12 -0700 (Tue, 14 Jul 2020)
Log Message
[iOS] The completion handler in -handleKeyWebEvent:withCompletionHandler: is sometimes never called
https://bugs.webkit.org/show_bug.cgi?id=214295
<rdar://problem/60539389>
Reviewed by Devin Rousso.
Source/WebKit:
This is a speculative fix for <rdar://problem/60539389>, wherein hardware key commands seemingly stop working in
a web page that is (presumably) otherwise responsive. It's possible that the bug exercises a scenario in which
the completion handler in `-[WKContentView handleKeyWebEvent:withCompletionHandler:]` is never invoked, which
subsequently leads to the keyboard task queue being backed up with key events.
This can happen in several ways. For instance, if the web process is swapped or terminates in the middle of
handling a key event, the key event queue will be cleared, but the UI process will still retain the (uncalled)
completion handler for that key event. Additionally, `WebPageProxy::handleKeyboardEvent` may not even have
attempted to propagate the event to the web process, in which case we shouldn't be saving the completion handler
and waiting for a response.
Test: KeyboardInputTests.HandleKeyEventsInCrashedOrUninitializedWebProcess
KeyboardInputTests.HandleKeyEventsWhileSwappingWebProcess
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::handleKeyboardEvent):
Make this return a `bool` indicating whether the key event was sent to the web process. If not, then we
should immediately invoke the completion handler in -handleKeyWebEvent:withCompletionHandler: below, instead of
stashing the Objective-C block and waiting for a response from the web process (which is presumably not
running).
* UIProcess/WebPageProxy.h:
* UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView cleanUpInteraction]):
(-[WKContentView _cancelPendingKeyEventHandler]):
When the web process terminates or swaps in the middle of handling a key event, go ahead and invoke the key
event completion handler early with the queued event, since we aren't going to receive a response from the web
process anyways.
(-[WKContentView handleKeyWebEvent:withCompletionHandler:]):
Tools:
Add API tests to exercise the corner cases described in the WebKit ChangeLog.
* TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (264375 => 264376)
--- trunk/Source/WebKit/ChangeLog 2020-07-14 21:28:45 UTC (rev 264375)
+++ trunk/Source/WebKit/ChangeLog 2020-07-14 21:39:12 UTC (rev 264376)
@@ -1,3 +1,44 @@
+2020-07-14 Wenson Hsieh <wenson_hs...@apple.com>
+
+ [iOS] The completion handler in -handleKeyWebEvent:withCompletionHandler: is sometimes never called
+ https://bugs.webkit.org/show_bug.cgi?id=214295
+ <rdar://problem/60539389>
+
+ Reviewed by Devin Rousso.
+
+ This is a speculative fix for <rdar://problem/60539389>, wherein hardware key commands seemingly stop working in
+ a web page that is (presumably) otherwise responsive. It's possible that the bug exercises a scenario in which
+ the completion handler in `-[WKContentView handleKeyWebEvent:withCompletionHandler:]` is never invoked, which
+ subsequently leads to the keyboard task queue being backed up with key events.
+
+ This can happen in several ways. For instance, if the web process is swapped or terminates in the middle of
+ handling a key event, the key event queue will be cleared, but the UI process will still retain the (uncalled)
+ completion handler for that key event. Additionally, `WebPageProxy::handleKeyboardEvent` may not even have
+ attempted to propagate the event to the web process, in which case we shouldn't be saving the completion handler
+ and waiting for a response.
+
+ Test: KeyboardInputTests.HandleKeyEventsInCrashedOrUninitializedWebProcess
+ KeyboardInputTests.HandleKeyEventsWhileSwappingWebProcess
+
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::handleKeyboardEvent):
+
+ Make this return a `bool` indicating whether the key event was sent to the web process. If not, then we
+ should immediately invoke the completion handler in -handleKeyWebEvent:withCompletionHandler: below, instead of
+ stashing the Objective-C block and waiting for a response from the web process (which is presumably not
+ running).
+
+ * UIProcess/WebPageProxy.h:
+ * UIProcess/ios/WKContentViewInteraction.mm:
+ (-[WKContentView cleanUpInteraction]):
+ (-[WKContentView _cancelPendingKeyEventHandler]):
+
+ When the web process terminates or swaps in the middle of handling a key event, go ahead and invoke the key
+ event completion handler early with the queued event, since we aren't going to receive a response from the web
+ process anyways.
+
+ (-[WKContentView handleKeyWebEvent:withCompletionHandler:]):
+
2020-07-14 Simon Fraser <simon.fra...@apple.com>
Flashes of incorrect scroll position when zooming on quip
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (264375 => 264376)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2020-07-14 21:28:45 UTC (rev 264375)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2020-07-14 21:39:12 UTC (rev 264376)
@@ -2748,10 +2748,10 @@
return m_keyEventQueue.first();
}
-void WebPageProxy::handleKeyboardEvent(const NativeWebKeyboardEvent& event)
+bool WebPageProxy::handleKeyboardEvent(const NativeWebKeyboardEvent& event)
{
if (!hasRunningProcess())
- return;
+ return false;
LOG(KeyHandling, "WebPageProxy::handleKeyboardEvent: %s", webKeyboardEventTypeString(event.type()));
@@ -2763,6 +2763,8 @@
LOG(KeyHandling, " UI process: sent keyEvent from handleKeyboardEvent");
send(Messages::WebPage::KeyEvent(event));
}
+
+ return true;
}
WebPreferencesStore WebPageProxy::preferencesStore() const
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.h (264375 => 264376)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.h 2020-07-14 21:28:45 UTC (rev 264375)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.h 2020-07-14 21:39:12 UTC (rev 264376)
@@ -937,7 +937,7 @@
void handleWheelEvent(const NativeWebWheelEvent&);
bool isProcessingKeyboardEvents() const;
- void handleKeyboardEvent(const NativeWebKeyboardEvent&);
+ bool handleKeyboardEvent(const NativeWebKeyboardEvent&);
#if PLATFORM(WIN)
void dispatchPendingCharEvents(const NativeWebKeyboardEvent&);
#endif
Modified: trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (264375 => 264376)
--- trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2020-07-14 21:28:45 UTC (rev 264375)
+++ trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2020-07-14 21:39:12 UTC (rev 264376)
@@ -1062,8 +1062,22 @@
[self _resetPanningPreventionFlags];
[self _handleDOMPasteRequestWithResult:WebCore::DOMPasteAccessResponse::DeniedForGesture];
+ [self _cancelPendingKeyEventHandler];
}
+- (void)_cancelPendingKeyEventHandler
+{
+ if (!_page)
+ return;
+
+ ASSERT_IMPLIES(_keyWebEventHandler, _page->hasQueuedKeyEvent());
+ if (!_page->hasQueuedKeyEvent())
+ return;
+
+ if (auto keyEventHandler = std::exchange(_keyWebEventHandler, nil))
+ keyEventHandler(_page->firstQueuedKeyEvent().nativeEvent(), NO);
+}
+
- (void)_removeDefaultGestureRecognizers
{
#if ENABLE(IOS_TOUCH_EVENTS)
@@ -5346,8 +5360,10 @@
return;
}
#endif
- _keyWebEventHandler = makeBlockPtr(completionHandler);
- _page->handleKeyboardEvent(WebKit::NativeWebKeyboardEvent(theEvent, HandledByInputMethod::No));
+ if (_page->handleKeyboardEvent(WebKit::NativeWebKeyboardEvent(theEvent, HandledByInputMethod::No)))
+ _keyWebEventHandler = makeBlockPtr(completionHandler);
+ else
+ completionHandler(theEvent, NO);
}
- (void)_didHandleKeyEvent:(::WebEvent *)event eventWasHandled:(BOOL)eventWasHandled
Modified: trunk/Tools/ChangeLog (264375 => 264376)
--- trunk/Tools/ChangeLog 2020-07-14 21:28:45 UTC (rev 264375)
+++ trunk/Tools/ChangeLog 2020-07-14 21:39:12 UTC (rev 264376)
@@ -1,3 +1,15 @@
+2020-07-14 Wenson Hsieh <wenson_hs...@apple.com>
+
+ [iOS] The completion handler in -handleKeyWebEvent:withCompletionHandler: is sometimes never called
+ https://bugs.webkit.org/show_bug.cgi?id=214295
+ <rdar://problem/60539389>
+
+ Reviewed by Devin Rousso.
+
+ Add API tests to exercise the corner cases described in the WebKit ChangeLog.
+
+ * TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:
+
2020-07-14 Aakash Jain <aakash_j...@apple.com>
[build.webkit.org] watchos should build both arm64_32 and armv7k architectures
Modified: trunk/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm (264375 => 264376)
--- trunk/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm 2020-07-14 21:28:45 UTC (rev 264375)
+++ trunk/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm 2020-07-14 21:39:12 UTC (rev 264376)
@@ -30,10 +30,14 @@
#import "PlatformUtilities.h"
#import "TestCocoa.h"
#import "TestInputDelegate.h"
+#import "TestNavigationDelegate.h"
+#import "TestProtocol.h"
#import "TestWKWebView.h"
#import "UIKitSPI.h"
#import "UserInterfaceSwizzler.h"
+#import <WebKit/WKProcessPoolPrivate.h>
#import <WebKit/WKWebViewPrivate.h>
+#import <WebKit/_WKProcessPoolConfiguration.h>
#import <WebKitLegacy/WebEvent.h>
#import <cmath>
@@ -368,6 +372,69 @@
TestWebKitAPI::Util::run(&doneWaiting);
}
+TEST(KeyboardInputTests, HandleKeyEventsInCrashedOrUninitializedWebProcess)
+{
+ auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
+ auto contentView = [webView textInputContentView];
+ {
+ auto keyDownEvent = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyDown timeStamp:CFAbsoluteTimeGetCurrent() characters:@"a" charactersIgnoringModifiers:@"a" modifiers:0 isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:65 isTabKey:NO]);
+ bool doneWaiting = false;
+ [webView synchronouslyLoadHTMLString:@"<body></body>"];
+ [webView evaluateJavaScript:@"while (1);" completionHandler:nil];
+ [contentView handleKeyWebEvent:keyDownEvent.get() withCompletionHandler:[&](WebEvent *event, BOOL handled) {
+ EXPECT_TRUE([event isEqual:keyDownEvent.get()]);
+ EXPECT_FALSE(handled);
+ doneWaiting = true;
+ }];
+ [webView _killWebContentProcessAndResetState];
+ TestWebKitAPI::Util::run(&doneWaiting);
+ }
+ {
+ auto keyUpEvent = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyUp timeStamp:CFAbsoluteTimeGetCurrent() characters:@"a" charactersIgnoringModifiers:@"a" modifiers:0 isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:65 isTabKey:NO]);
+ bool doneWaiting = false;
+ [webView _close];
+ [contentView handleKeyWebEvent:keyUpEvent.get() withCompletionHandler:[&](WebEvent *event, BOOL handled) {
+ EXPECT_TRUE([event isEqual:keyUpEvent.get()]);
+ EXPECT_FALSE(handled);
+ doneWaiting = true;
+ }];
+ TestWebKitAPI::Util::run(&doneWaiting);
+ }
+}
+
+TEST(KeyboardInputTests, HandleKeyEventsWhileSwappingWebProcess)
+{
+ [TestProtocol registerWithScheme:@"https"];
+
+ auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+ [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+ auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+ auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+ [configuration setProcessPool:processPool.get()];
+
+ auto navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
+ auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+ [webView setNavigationDelegate:navigationDelegate.get()];
+ [webView loadHTMLString:@"<body>webkit.org</body>" baseURL:[NSURL URLWithString:@"https://webkit.org"]];
+ [navigationDelegate waitForDidFinishNavigation];
+
+ [webView loadHTMLString:@"<body>apple.com</body>" baseURL:[NSURL URLWithString:@"https://apple.com"]];
+ [navigationDelegate waitForDidStartProvisionalNavigation];
+
+ bool done = false;
+ auto keyEvent = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyDown timeStamp:CFAbsoluteTimeGetCurrent() characters:@"a" charactersIgnoringModifiers:@"a" modifiers:0 isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:65 isTabKey:NO]);
+ dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.01 * NSEC_PER_SEC)), dispatch_get_main_queue(), [keyEvent, webView, &done] {
+ [[webView textInputContentView] handleKeyWebEvent:keyEvent.get() withCompletionHandler:[keyEvent, &done](WebEvent *event, BOOL handled) {
+ EXPECT_TRUE([event isEqual:keyEvent.get()]);
+ EXPECT_FALSE(handled);
+ done = true;
+ }];
+ });
+
+ [navigationDelegate waitForDidFinishNavigation];
+ TestWebKitAPI::Util::run(&done);
+}
+
TEST(KeyboardInputTests, CaretSelectionRectAfterRestoringFirstResponderWithRetainActiveFocusedState)
{
// This difference in caret width is due to the fact that we don't zoom in to the input field on iPad, but do on iPhone.
