Title: [253929] trunk/Tools
- Revision
- 253929
- Author
- [email protected]
- Date
- 2019-12-28 18:05:09 -0800 (Sat, 28 Dec 2019)
Log Message
[iOS] Layout tests sometimes throw an exception under checkForOutstandingCallbacks
https://bugs.webkit.org/show_bug.cgi?id=205612
<rdar://problem/57789693>
Reviewed by Tim Horton.
On iOS, layout tests that synthesize HID events but end before WebKitTestRunnerApp finishes dequeueing and
handling those events occasionally cause the next test to crash with an Objective-C exception under
UIScriptControllerIOS::checkForOutstandingCallbacks. This happens when UIScriptContext is destroyed after a HID
marker event is dispatched, but before that HID marker event has been handled. (For clarity, the HID marker
event is a special vendor-defined event used by HIDEventGenerator to signify the end of a series of synthesized
HID events that were previously dispatched to the application).
This is typically fixed by ensuring that all iOS layout tests always wait for synthesized HID events to finish
before ending the test (i.e. by calling `testRunner.notifyDone()`). However, some tests that fall into this
category are imported: e.g. dom/events/document-level-touchmove-event-listener-passive-by-default.html in
web-platform-tests/, which does not wait for the swipe gesture to finish before completing. This current causes
us to dispatch the end of the gesture while the following test (dom/events/event-disabled-dynamic.html) begins.
While I wasn't able to trivially reproduce the exception locally, it was consistently reproducible by forcing a
50 ms `sleep` in -[HIDEventGenerator sendMarkerHIDEventWithCompletionBlock:], right before queueing the marker
event. This suggests that the crash is timing-dependent, and just seems to occasionally reproduce more
frequently in internal automation.
This test seems to be passing reliably in other engines (e.g. Chrome and Edge), so instead of trying to fix the
test to always wait for events to finish dispatching, we can address the issue by teaching WebKitTestRunner to
simply wait for outgoing marker events to finish dispatching before proceeding with the next test, rather than
crashing. This should not only fix the crash, but also address sporadic flakiness that may result from tests
that handle synthetic HID events that were dispatched by the previous test.
* TestRunnerShared/UIScriptContext/UIScriptContext.cpp:
(UIScriptContext::~UIScriptContext):
* TestRunnerShared/UIScriptContext/UIScriptController.h:
(WTR::UIScriptController::waitForOutstandingCallbacks):
(WTR::UIScriptController::checkForOutstandingCallbacks): Deleted.
Rename checkForOutstandingCallbacks to waitForOutstandingCallbacks, and make it wait up to a second for the
application to finish handling any outgoing marker HID event. In the event that the timeout is hit, we still
throw an Objective-C exception to avoid beginning the next test in an unpredictable state.
* WebKitTestRunner/ios/HIDEventGenerator.h:
* WebKitTestRunner/ios/HIDEventGenerator.mm:
(-[HIDEventGenerator init]):
Perform some minor cleanup here, by removing excess private category properties in HIDEventGenerator (including
-debugTouchViews, which was unused); also, change _eventCallbacks into a `RetainPtr`, so that we don't need to
worry about manually releasing it.
(-[HIDEventGenerator dealloc]): Deleted.
(-[HIDEventGenerator hasOutstandingCallbacks]):
(-[HIDEventGenerator checkForOutstandingCallbacks]): Deleted.
Rename -checkForOutstandingCallbacks to -hasOutstandingCallbacks, and flip the return result.
* WebKitTestRunner/ios/UIScriptControllerIOS.h:
* WebKitTestRunner/ios/UIScriptControllerIOS.mm:
(WTR::UIScriptControllerIOS::waitForOutstandingCallbacks):
(WTR::UIScriptControllerIOS::checkForOutstandingCallbacks): Deleted.
Modified Paths
Diff
Modified: trunk/Tools/ChangeLog (253928 => 253929)
--- trunk/Tools/ChangeLog 2019-12-29 00:19:44 UTC (rev 253928)
+++ trunk/Tools/ChangeLog 2019-12-29 02:05:09 UTC (rev 253929)
@@ -1,3 +1,64 @@
+2019-12-28 Wenson Hsieh <[email protected]>
+
+ [iOS] Layout tests sometimes throw an exception under checkForOutstandingCallbacks
+ https://bugs.webkit.org/show_bug.cgi?id=205612
+ <rdar://problem/57789693>
+
+ Reviewed by Tim Horton.
+
+ On iOS, layout tests that synthesize HID events but end before WebKitTestRunnerApp finishes dequeueing and
+ handling those events occasionally cause the next test to crash with an Objective-C exception under
+ UIScriptControllerIOS::checkForOutstandingCallbacks. This happens when UIScriptContext is destroyed after a HID
+ marker event is dispatched, but before that HID marker event has been handled. (For clarity, the HID marker
+ event is a special vendor-defined event used by HIDEventGenerator to signify the end of a series of synthesized
+ HID events that were previously dispatched to the application).
+
+ This is typically fixed by ensuring that all iOS layout tests always wait for synthesized HID events to finish
+ before ending the test (i.e. by calling `testRunner.notifyDone()`). However, some tests that fall into this
+ category are imported: e.g. dom/events/document-level-touchmove-event-listener-passive-by-default.html in
+ web-platform-tests/, which does not wait for the swipe gesture to finish before completing. This current causes
+ us to dispatch the end of the gesture while the following test (dom/events/event-disabled-dynamic.html) begins.
+
+ While I wasn't able to trivially reproduce the exception locally, it was consistently reproducible by forcing a
+ 50 ms `sleep` in -[HIDEventGenerator sendMarkerHIDEventWithCompletionBlock:], right before queueing the marker
+ event. This suggests that the crash is timing-dependent, and just seems to occasionally reproduce more
+ frequently in internal automation.
+
+ This test seems to be passing reliably in other engines (e.g. Chrome and Edge), so instead of trying to fix the
+ test to always wait for events to finish dispatching, we can address the issue by teaching WebKitTestRunner to
+ simply wait for outgoing marker events to finish dispatching before proceeding with the next test, rather than
+ crashing. This should not only fix the crash, but also address sporadic flakiness that may result from tests
+ that handle synthetic HID events that were dispatched by the previous test.
+
+ * TestRunnerShared/UIScriptContext/UIScriptContext.cpp:
+ (UIScriptContext::~UIScriptContext):
+ * TestRunnerShared/UIScriptContext/UIScriptController.h:
+ (WTR::UIScriptController::waitForOutstandingCallbacks):
+ (WTR::UIScriptController::checkForOutstandingCallbacks): Deleted.
+
+ Rename checkForOutstandingCallbacks to waitForOutstandingCallbacks, and make it wait up to a second for the
+ application to finish handling any outgoing marker HID event. In the event that the timeout is hit, we still
+ throw an Objective-C exception to avoid beginning the next test in an unpredictable state.
+
+ * WebKitTestRunner/ios/HIDEventGenerator.h:
+ * WebKitTestRunner/ios/HIDEventGenerator.mm:
+ (-[HIDEventGenerator init]):
+
+ Perform some minor cleanup here, by removing excess private category properties in HIDEventGenerator (including
+ -debugTouchViews, which was unused); also, change _eventCallbacks into a `RetainPtr`, so that we don't need to
+ worry about manually releasing it.
+
+ (-[HIDEventGenerator dealloc]): Deleted.
+ (-[HIDEventGenerator hasOutstandingCallbacks]):
+ (-[HIDEventGenerator checkForOutstandingCallbacks]): Deleted.
+
+ Rename -checkForOutstandingCallbacks to -hasOutstandingCallbacks, and flip the return result.
+
+ * WebKitTestRunner/ios/UIScriptControllerIOS.h:
+ * WebKitTestRunner/ios/UIScriptControllerIOS.mm:
+ (WTR::UIScriptControllerIOS::waitForOutstandingCallbacks):
+ (WTR::UIScriptControllerIOS::checkForOutstandingCallbacks): Deleted.
+
2019-12-28 Antti Koivisto <[email protected]>
Allow disabling internal and experimental features in run-webkit-tests
Modified: trunk/Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp (253928 => 253929)
--- trunk/Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp 2019-12-29 00:19:44 UTC (rev 253928)
+++ trunk/Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp 2019-12-29 02:05:09 UTC (rev 253929)
@@ -52,7 +52,7 @@
UIScriptContext::~UIScriptContext()
{
- m_controller->checkForOutstandingCallbacks();
+ m_controller->waitForOutstandingCallbacks();
m_controller->contextDestroyed();
}
Modified: trunk/Tools/TestRunnerShared/UIScriptContext/UIScriptController.h (253928 => 253929)
--- trunk/Tools/TestRunnerShared/UIScriptContext/UIScriptController.h 2019-12-29 00:19:44 UTC (rev 253928)
+++ trunk/Tools/TestRunnerShared/UIScriptContext/UIScriptController.h 2019-12-29 02:05:09 UTC (rev 253929)
@@ -61,7 +61,7 @@
void notImplemented() const { RELEASE_ASSERT_NOT_REACHED(); }
void contextDestroyed();
- virtual void checkForOutstandingCallbacks() { /* notImplemented(); */ }
+ virtual void waitForOutstandingCallbacks() { /* notImplemented(); */ }
void makeWindowObject(JSContextRef, JSObjectRef windowObject, JSValueRef* exception);
Modified: trunk/Tools/WebKitTestRunner/ios/HIDEventGenerator.h (253928 => 253929)
--- trunk/Tools/WebKitTestRunner/ios/HIDEventGenerator.h 2019-12-29 00:19:44 UTC (rev 253928)
+++ trunk/Tools/WebKitTestRunner/ios/HIDEventGenerator.h 2019-12-29 02:05:09 UTC (rev 253929)
@@ -104,7 +104,7 @@
- (void)sendEventStream:(NSDictionary *)eventInfo completionBlock:(void (^)(void))completionBlock;
- (void)markerEventReceived:(IOHIDEventRef)event;
-- (BOOL)checkForOutstandingCallbacks;
+- (BOOL)hasOutstandingCallbacks;
// Keyboard
- (void)keyPress:(NSString *)character completionBlock:(void (^)(void))completionBlock;
Modified: trunk/Tools/WebKitTestRunner/ios/HIDEventGenerator.mm (253928 => 253929)
--- trunk/Tools/WebKitTestRunner/ios/HIDEventGenerator.mm 2019-12-29 00:19:44 UTC (rev 253928)
+++ trunk/Tools/WebKitTestRunner/ios/HIDEventGenerator.mm 2019-12-29 02:05:09 UTC (rev 253929)
@@ -151,15 +151,11 @@
}
}
-@interface HIDEventGenerator ()
-@property (nonatomic, strong) NSMutableDictionary *eventCallbacks;
-@property (nonatomic, strong) NSArray<UIView *> *debugTouchViews;
-@end
-
@implementation HIDEventGenerator {
IOHIDEventSystemClientRef _ioSystemClient;
SyntheticEventDigitizerInfo _activePoints[HIDMaxTouchCount];
NSUInteger _activePointCount;
+ RetainPtr<NSMutableDictionary> _eventCallbacks;
}
+ (HIDEventGenerator *)sharedHIDEventGenerator
@@ -186,18 +182,11 @@
for (NSUInteger i = 0; i < HIDMaxTouchCount; ++i)
_activePoints[i].identifier = fingerIdentifiers[i];
- _eventCallbacks = [[NSMutableDictionary alloc] init];
+ _eventCallbacks = adoptNS([[NSMutableDictionary alloc] init]);
return self;
}
-- (void)dealloc
-{
- [_eventCallbacks release];
- [_debugTouchViews release];
- [super dealloc];
-}
-
- (void)_sendIOHIDKeyboardEvent:(uint64_t)timestamp usage:(uint32_t)usage isKeyDown:(bool)isKeyDown
{
RetainPtr<IOHIDEventRef> eventRef = adoptCF(IOHIDEventCreateKeyboardEvent(kCFAllocatorDefault,
@@ -800,9 +789,9 @@
}
}
-- (BOOL)checkForOutstandingCallbacks
+- (BOOL)hasOutstandingCallbacks
{
- return !([_eventCallbacks count] > 0);
+ return [_eventCallbacks count];
}
static inline bool shouldWrapWithShiftKeyEventForCharacter(NSString *key)
Modified: trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h (253928 => 253929)
--- trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h 2019-12-29 00:19:44 UTC (rev 253928)
+++ trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h 2019-12-29 02:05:09 UTC (rev 253929)
@@ -48,7 +48,7 @@
{
}
- void checkForOutstandingCallbacks() override;
+ void waitForOutstandingCallbacks() override;
void doAfterPresentationUpdate(JSValueRef) override;
void doAfterNextStablePresentationUpdate(JSValueRef) override;
void ensurePositionInformationIsUpToDateAt(long x, long y, JSValueRef) override;
Modified: trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm (253928 => 253929)
--- trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm 2019-12-29 00:19:44 UTC (rev 253928)
+++ trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm 2019-12-29 02:05:09 UTC (rev 253929)
@@ -112,10 +112,15 @@
return adoptRef(*new UIScriptControllerIOS(context));
}
-void UIScriptControllerIOS::checkForOutstandingCallbacks()
+void UIScriptControllerIOS::waitForOutstandingCallbacks()
{
- if (![[HIDEventGenerator sharedHIDEventGenerator] checkForOutstandingCallbacks])
- [NSException raise:@"WebKitTestRunnerTestProblem" format:@"The test completed before all synthesized events had been handled. Perhaps you're calling notifyDone() too early?"];
+ HIDEventGenerator *eventGenerator = HIDEventGenerator.sharedHIDEventGenerator;
+ NSDate *timeoutDate = [NSDate dateWithTimeIntervalSinceNow:1];
+ while (eventGenerator.hasOutstandingCallbacks) {
+ [NSRunLoop.currentRunLoop runMode:NSDefaultRunLoopMode beforeDate:timeoutDate];
+ if ([timeoutDate compare:NSDate.date] == NSOrderedAscending)
+ [NSException raise:@"WebKitTestRunnerTestProblem" format:@"The previous test completed before all synthesized events had been handled. Perhaps you're calling notifyDone() too early?"];
+ }
}
void UIScriptControllerIOS::doAfterPresentationUpdate(JSValueRef callback)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
