Skip to content

Commit a956353

Browse files
whsiehwhsieh
committed
[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: Canonical link: https://commits.webkit.org/227130@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@264376 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent b754647 commit a956353

6 files changed

Lines changed: 143 additions & 5 deletions

File tree

‎Source/WebKit/ChangeLog‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,44 @@
1+
2020-07-14 Wenson Hsieh <[email protected]>
2+
3+
[iOS] The completion handler in -handleKeyWebEvent:withCompletionHandler: is sometimes never called
4+
https://bugs.webkit.org/show_bug.cgi?id=214295
5+
<rdar://problem/60539389>
6+
7+
Reviewed by Devin Rousso.
8+
9+
This is a speculative fix for <rdar://problem/60539389>, wherein hardware key commands seemingly stop working in
10+
a web page that is (presumably) otherwise responsive. It's possible that the bug exercises a scenario in which
11+
the completion handler in `-[WKContentView handleKeyWebEvent:withCompletionHandler:]` is never invoked, which
12+
subsequently leads to the keyboard task queue being backed up with key events.
13+
14+
This can happen in several ways. For instance, if the web process is swapped or terminates in the middle of
15+
handling a key event, the key event queue will be cleared, but the UI process will still retain the (uncalled)
16+
completion handler for that key event. Additionally, `WebPageProxy::handleKeyboardEvent` may not even have
17+
attempted to propagate the event to the web process, in which case we shouldn't be saving the completion handler
18+
and waiting for a response.
19+
20+
Test: KeyboardInputTests.HandleKeyEventsInCrashedOrUninitializedWebProcess
21+
KeyboardInputTests.HandleKeyEventsWhileSwappingWebProcess
22+
23+
* UIProcess/WebPageProxy.cpp:
24+
(WebKit::WebPageProxy::handleKeyboardEvent):
25+
26+
Make this return a `bool` indicating whether the key event was sent to the web process. If not, then we
27+
should immediately invoke the completion handler in -handleKeyWebEvent:withCompletionHandler: below, instead of
28+
stashing the Objective-C block and waiting for a response from the web process (which is presumably not
29+
running).
30+
31+
* UIProcess/WebPageProxy.h:
32+
* UIProcess/ios/WKContentViewInteraction.mm:
33+
(-[WKContentView cleanUpInteraction]):
34+
(-[WKContentView _cancelPendingKeyEventHandler]):
35+
36+
When the web process terminates or swaps in the middle of handling a key event, go ahead and invoke the key
37+
event completion handler early with the queued event, since we aren't going to receive a response from the web
38+
process anyways.
39+
40+
(-[WKContentView handleKeyWebEvent:withCompletionHandler:]):
41+
142
2020-07-14 Simon Fraser <[email protected]>
243

344
Flashes of incorrect scroll position when zooming on quip

‎Source/WebKit/UIProcess/WebPageProxy.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2748,10 +2748,10 @@ const NativeWebKeyboardEvent& WebPageProxy::firstQueuedKeyEvent() const
27482748
return m_keyEventQueue.first();
27492749
}
27502750

2751-
void WebPageProxy::handleKeyboardEvent(const NativeWebKeyboardEvent& event)
2751+
bool WebPageProxy::handleKeyboardEvent(const NativeWebKeyboardEvent& event)
27522752
{
27532753
if (!hasRunningProcess())
2754-
return;
2754+
return false;
27552755

27562756
LOG(KeyHandling, "WebPageProxy::handleKeyboardEvent: %s", webKeyboardEventTypeString(event.type()));
27572757

@@ -2763,6 +2763,8 @@ void WebPageProxy::handleKeyboardEvent(const NativeWebKeyboardEvent& event)
27632763
LOG(KeyHandling, " UI process: sent keyEvent from handleKeyboardEvent");
27642764
send(Messages::WebPage::KeyEvent(event));
27652765
}
2766+
2767+
return true;
27662768
}
27672769

27682770
WebPreferencesStore WebPageProxy::preferencesStore() const

‎Source/WebKit/UIProcess/WebPageProxy.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -937,7 +937,7 @@ class WebPageProxy : public API::ObjectImpl<API::Object::Type::Page>
937937
void handleWheelEvent(const NativeWebWheelEvent&);
938938

939939
bool isProcessingKeyboardEvents() const;
940-
void handleKeyboardEvent(const NativeWebKeyboardEvent&);
940+
bool handleKeyboardEvent(const NativeWebKeyboardEvent&);
941941
#if PLATFORM(WIN)
942942
void dispatchPendingCharEvents(const NativeWebKeyboardEvent&);
943943
#endif

‎Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1062,6 +1062,20 @@ - (void)cleanUpInteraction
10621062

10631063
[self _resetPanningPreventionFlags];
10641064
[self _handleDOMPasteRequestWithResult:WebCore::DOMPasteAccessResponse::DeniedForGesture];
1065+
[self _cancelPendingKeyEventHandler];
1066+
}
1067+
1068+
- (void)_cancelPendingKeyEventHandler
1069+
{
1070+
if (!_page)
1071+
return;
1072+
1073+
ASSERT_IMPLIES(_keyWebEventHandler, _page->hasQueuedKeyEvent());
1074+
if (!_page->hasQueuedKeyEvent())
1075+
return;
1076+
1077+
if (auto keyEventHandler = std::exchange(_keyWebEventHandler, nil))
1078+
keyEventHandler(_page->firstQueuedKeyEvent().nativeEvent(), NO);
10651079
}
10661080

10671081
- (void)_removeDefaultGestureRecognizers
@@ -5346,8 +5360,10 @@ - (void)handleKeyWebEvent:(::WebEvent *)theEvent withCompletionHandler:(void (^)
53465360
return;
53475361
}
53485362
#endif
5349-
_keyWebEventHandler = makeBlockPtr(completionHandler);
5350-
_page->handleKeyboardEvent(WebKit::NativeWebKeyboardEvent(theEvent, HandledByInputMethod::No));
5363+
if (_page->handleKeyboardEvent(WebKit::NativeWebKeyboardEvent(theEvent, HandledByInputMethod::No)))
5364+
_keyWebEventHandler = makeBlockPtr(completionHandler);
5365+
else
5366+
completionHandler(theEvent, NO);
53515367
}
53525368

53535369
- (void)_didHandleKeyEvent:(::WebEvent *)event eventWasHandled:(BOOL)eventWasHandled

‎Tools/ChangeLog‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,15 @@
1+
2020-07-14 Wenson Hsieh <[email protected]>
2+
3+
[iOS] The completion handler in -handleKeyWebEvent:withCompletionHandler: is sometimes never called
4+
https://bugs.webkit.org/show_bug.cgi?id=214295
5+
<rdar://problem/60539389>
6+
7+
Reviewed by Devin Rousso.
8+
9+
Add API tests to exercise the corner cases described in the WebKit ChangeLog.
10+
11+
* TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:
12+
113
2020-07-14 Aakash Jain <[email protected]>
214

315
[build.webkit.org] watchos should build both arm64_32 and armv7k architectures

‎Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,14 @@
3030
#import "PlatformUtilities.h"
3131
#import "TestCocoa.h"
3232
#import "TestInputDelegate.h"
33+
#import "TestNavigationDelegate.h"
34+
#import "TestProtocol.h"
3335
#import "TestWKWebView.h"
3436
#import "UIKitSPI.h"
3537
#import "UserInterfaceSwizzler.h"
38+
#import <WebKit/WKProcessPoolPrivate.h>
3639
#import <WebKit/WKWebViewPrivate.h>
40+
#import <WebKit/_WKProcessPoolConfiguration.h>
3741
#import <WebKitLegacy/WebEvent.h>
3842
#import <cmath>
3943

@@ -368,6 +372,69 @@ - (UIView *)inputAccessoryView
368372
TestWebKitAPI::Util::run(&doneWaiting);
369373
}
370374

375+
TEST(KeyboardInputTests, HandleKeyEventsInCrashedOrUninitializedWebProcess)
376+
{
377+
auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
378+
auto contentView = [webView textInputContentView];
379+
{
380+
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]);
381+
bool doneWaiting = false;
382+
[webView synchronouslyLoadHTMLString:@"<body></body>"];
383+
[webView evaluateJavaScript:@"while (1);" completionHandler:nil];
384+
[contentView handleKeyWebEvent:keyDownEvent.get() withCompletionHandler:[&](WebEvent *event, BOOL handled) {
385+
EXPECT_TRUE([event isEqual:keyDownEvent.get()]);
386+
EXPECT_FALSE(handled);
387+
doneWaiting = true;
388+
}];
389+
[webView _killWebContentProcessAndResetState];
390+
TestWebKitAPI::Util::run(&doneWaiting);
391+
}
392+
{
393+
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]);
394+
bool doneWaiting = false;
395+
[webView _close];
396+
[contentView handleKeyWebEvent:keyUpEvent.get() withCompletionHandler:[&](WebEvent *event, BOOL handled) {
397+
EXPECT_TRUE([event isEqual:keyUpEvent.get()]);
398+
EXPECT_FALSE(handled);
399+
doneWaiting = true;
400+
}];
401+
TestWebKitAPI::Util::run(&doneWaiting);
402+
}
403+
}
404+
405+
TEST(KeyboardInputTests, HandleKeyEventsWhileSwappingWebProcess)
406+
{
407+
[TestProtocol registerWithScheme:@"https"];
408+
409+
auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
410+
[processPoolConfiguration setProcessSwapsOnNavigation:YES];
411+
auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
412+
auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
413+
[configuration setProcessPool:processPool.get()];
414+
415+
auto navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
416+
auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
417+
[webView setNavigationDelegate:navigationDelegate.get()];
418+
[webView loadHTMLString:@"<body>webkit.org</body>" baseURL:[NSURL URLWithString:@"https://webkit.org"]];
419+
[navigationDelegate waitForDidFinishNavigation];
420+
421+
[webView loadHTMLString:@"<body>apple.com</body>" baseURL:[NSURL URLWithString:@"https://apple.com"]];
422+
[navigationDelegate waitForDidStartProvisionalNavigation];
423+
424+
bool done = false;
425+
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]);
426+
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.01 * NSEC_PER_SEC)), dispatch_get_main_queue(), [keyEvent, webView, &done] {
427+
[[webView textInputContentView] handleKeyWebEvent:keyEvent.get() withCompletionHandler:[keyEvent, &done](WebEvent *event, BOOL handled) {
428+
EXPECT_TRUE([event isEqual:keyEvent.get()]);
429+
EXPECT_FALSE(handled);
430+
done = true;
431+
}];
432+
});
433+
434+
[navigationDelegate waitForDidFinishNavigation];
435+
TestWebKitAPI::Util::run(&done);
436+
}
437+
371438
TEST(KeyboardInputTests, CaretSelectionRectAfterRestoringFirstResponderWithRetainActiveFocusedState)
372439
{
373440
// 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.

0 commit comments

Comments
 (0)