Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] Several FullscreenVideoTextRecognition API tests are flaky failures #2797

Merged
merged 1 commit into from
Jul 28, 2022

Conversation

whsieh
Copy link
Member

@whsieh whsieh commented Jul 28, 2022

d5d042e

[iOS] Several FullscreenVideoTextRecognition API tests are flaky failures
https://bugs.webkit.org/show_bug.cgi?id=243273
rdar://97567564

Reviewed by Tim Horton.

Address a couple sources of flaky crashes in these API tests on iOS:

1. It's possible for `-play` to trigger the video fullscreen presentation codepath (and thus, call
into `-presentViewController:animated:completion:` before the tests that call this method have
finished; in this scenario, the view controller presentation method isn't swizzled, so we end up
crashing in UIKit code due to lack of a UI application). Address this by pulling this logic out into
the common initializer of `FullscreenVideoTextRecognitionWebView` instead, to ensure that this call
into `UIViewController` remains swizzled throughout the entire duration of each test.

2. The swizzled implementation of `swizzledPresentViewController` are current unsafe, since nothing
guarantees that the completion handler is still alive by the time it is invoked on the next runloop
iteration. Address this by using `BlockPtr` to retain and release the completion block.

* Tools/TestWebKitAPI/Tests/WebKitCocoa/FullscreenVideoTextRecognition.mm:
(swizzledPresentViewController):
(-[FullscreenVideoTextRecognitionWebView initWithFrame:configuration:]):
(-[FullscreenVideoTextRecognitionWebView enterFullscreen]):

Canonical link: https://commits.webkit.org/252892@main

@whsieh whsieh self-assigned this Jul 28, 2022
@whsieh whsieh added Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases WebKit Nightly Build labels Jul 28, 2022
@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Jul 28, 2022
@whsieh
Copy link
Member Author

whsieh commented Jul 28, 2022

Thanks for the review!

…ures

https://bugs.webkit.org/show_bug.cgi?id=243273
rdar://97567564

Reviewed by Tim Horton.

Address a couple sources of flaky crashes in these API tests on iOS:

1. It's possible for `-play` to trigger the video fullscreen presentation codepath (and thus, call
into `-presentViewController:animated:completion:` before the tests that call this method have
finished; in this scenario, the view controller presentation method isn't swizzled, so we end up
crashing in UIKit code due to lack of a UI application). Address this by pulling this logic out into
the common initializer of `FullscreenVideoTextRecognitionWebView` instead, to ensure that this call
into `UIViewController` remains swizzled throughout the entire duration of each test.

2. The swizzled implementation of `swizzledPresentViewController` are current unsafe, since nothing
guarantees that the completion handler is still alive by the time it is invoked on the next runloop
iteration. Address this by using `BlockPtr` to retain and release the completion block.

* Tools/TestWebKitAPI/Tests/WebKitCocoa/FullscreenVideoTextRecognition.mm:
(swizzledPresentViewController):
(-[FullscreenVideoTextRecognitionWebView initWithFrame:configuration:]):
(-[FullscreenVideoTextRecognitionWebView enterFullscreen]):

Canonical link: https://commits.webkit.org/252892@main
@webkit-commit-queue
Copy link
Collaborator

Committed 252892@main (d5d042e): https://commits.webkit.org/252892@main

Reviewed commits have been landed. Closing PR #2797 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit d5d042e into WebKit:main Jul 28, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 28, 2022
@whsieh whsieh deleted the eng/243273 branch May 17, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
4 participants