[iOS] Layout tests sometimes throw an exception under checkForOutstandingCallbacks
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 29 Dec 2019 02:05:09 +0000 (02:05 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 29 Dec 2019 02:05:09 +0000 (02:05 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@253929 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp
Tools/TestRunnerShared/UIScriptContext/UIScriptController.h
Tools/WebKitTestRunner/ios/HIDEventGenerator.h
Tools/WebKitTestRunner/ios/HIDEventGenerator.mm
Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h
Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm

index d9f41df..f0dae3f 100644 (file)
@@ -1,3 +1,64 @@
+2019-12-28  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [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  <antti@apple.com>
 
         Allow disabling internal and experimental features in run-webkit-tests
index 5a84da8..97191a0 100644 (file)
@@ -52,7 +52,7 @@ UIScriptContext::UIScriptContext(UIScriptContextDelegate& delegate)
 
 UIScriptContext::~UIScriptContext()
 {
-    m_controller->checkForOutstandingCallbacks();
+    m_controller->waitForOutstandingCallbacks();
     m_controller->contextDestroyed();
 }
 
index 586b2e4..d1a3a7a 100644 (file)
@@ -61,7 +61,7 @@ public:
     void notImplemented() const { RELEASE_ASSERT_NOT_REACHED(); }
 
     void contextDestroyed();
-    virtual void checkForOutstandingCallbacks() { /* notImplemented(); */ }
+    virtual void waitForOutstandingCallbacks() { /* notImplemented(); */ }
 
     void makeWindowObject(JSContextRef, JSObjectRef windowObject, JSValueRef* exception);
 
index 08fb5c7..24850f9 100644 (file)
@@ -104,7 +104,7 @@ RetainPtr<IOHIDEventRef> createHIDKeyEvent(NSString *, uint64_t timestamp, bool
 - (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;
index 76c5e16..bc3b635 100644 (file)
@@ -151,15 +151,11 @@ static void delayBetweenMove(int eventIndex, double elapsed)
     }   
 }
 
-@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 @@ static void delayBetweenMove(int eventIndex, double elapsed)
     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 @@ static InterpolationType interpolationFromString(NSString *string)
     }
 }
 
-- (BOOL)checkForOutstandingCallbacks
+- (BOOL)hasOutstandingCallbacks
 {
-    return !([_eventCallbacks count] > 0);
+    return [_eventCallbacks count];
 }
 
 static inline bool shouldWrapWithShiftKeyEventForCharacter(NSString *key)
index 335e6f0..9b1731b 100644 (file)
@@ -48,7 +48,7 @@ public:
     {
     }
 
-    void checkForOutstandingCallbacks() override;
+    void waitForOutstandingCallbacks() override;
     void doAfterPresentationUpdate(JSValueRef) override;
     void doAfterNextStablePresentationUpdate(JSValueRef) override;
     void ensurePositionInformationIsUpToDateAt(long x, long y, JSValueRef) override;
index aadfc8c..d7cdce7 100644 (file)
@@ -112,10 +112,15 @@ Ref<UIScriptController> UIScriptController::create(UIScriptContext& context)
     return adoptRef(*new UIScriptControllerIOS(context));
 }
 
-void UIScriptControllerIOS::checkForOutstandingCallbacks()
-{
-    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?"];
+void UIScriptControllerIOS::waitForOutstandingCallbacks()
+{
+    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)