[webkit-changes] [WebKit/WebKit] 160483: [macOS] Spurious ASSERT fails under WebCore::Dicti...

[Abrar Rahman Protyasha]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 16048339c64acda4ebd00a9caec15b9cc3b7ab89
      https://github.com/WebKit/WebKit/commit/16048339c64acda4ebd00a9caec15b9cc3b7ab89
  Author: Abrar Rahman Protyasha <a_protyasha at apple.com>
  Date:   2024-12-02 (Mon, 02 Dec 2024)

  Changed paths:
    M Source/WebCore/editing/cocoa/DictionaryLookup.mm
    M Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm
    M Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
    M Tools/TestWebKitAPI/Tests/WebKitCocoa/UnifiedPDFTests.mm
    A Tools/TestWebKitAPI/Tests/WebKitCocoa/metalSpecTOC.pdf
    M Tools/TestWebKitAPI/Tests/mac/ImmediateActionTests.mm
    A Tools/TestWebKitAPI/Tests/mac/WKWebViewForTestingImmediateActions.h
    A Tools/TestWebKitAPI/Tests/mac/WKWebViewForTestingImmediateActions.mm

  Log Message:
  -----------
  [macOS] Spurious ASSERT fails under WebCore::DictionaryLookup::stringForPDFSelection with empty selection
https://bugs.webkit.org/show_bug.cgi?id=283847
rdar://128668294

Reviewed by Wenson Hsieh.

On some documents, we frequently ASSERT during lookup hit tests under
DictionaryLookup::stringForPDFSelection when clicking on (essentially)
blank text. This is because selection extension code in that function
can also end up modifying a PDF selection to nil if the Reveal framework
says the extracted lookup text range under the selection is empty. This
is problematic because the plain text on the text range in question is
an empty string, rather than nil, so our isEqualToString: assertion
fails.

This specific failure mode is unexpected, so we deflake the assertion in
two ways:

1. When the PDF selection is nil, instead of an NSString equality check,
   we check if the plain text on the text range is empty. This is a
   string equality check where we ignore nullity on one side. We
   fallback to the full isEqualToString: check if the PDF selection is
   not nil.
2. Augment an early return in UnifiedPDFPlugin::textForImmediateActionHitTestAtPoint()
   with a check for an empty PDF selection. This way, we are not
   unnecessarily performing lookup on empty text ranges.

To add testing coverage, we move the WKWebViewForTestingImmediateActions
class into a separate namesake header, such that it can be used in the
Unified PDF test suite.

* Source/WebCore/editing/cocoa/DictionaryLookup.mm:
(WebCore::DictionaryLookup::stringForPDFSelection):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm:
(WebKit::UnifiedPDFPlugin::textForImmediateActionHitTestAtPoint):
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/UnifiedPDFTests.mm:
(TestWebKitAPI::UNIFIED_PDF_TEST):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/metalSpecTOC.pdf: Added.
* Tools/TestWebKitAPI/Tests/mac/ImmediateActionTests.mm:
(swizzledImmediateActionLocationInView): Deleted.
(-[WKWebViewForTestingImmediateActions _immediateActionAnimationControllerForHitTestResult:withType:userData:]): Deleted.
(-[WKWebViewForTestingImmediateActions immediateActionGesture]): Deleted.
(-[WKWebViewForTestingImmediateActions simulateImmediateAction:]): Deleted.
* Tools/TestWebKitAPI/Tests/mac/WKWebViewForTestingImmediateActions.h: Added.
* Tools/TestWebKitAPI/Tests/mac/WKWebViewForTestingImmediateActions.mm: Added.
(swizzledImmediateActionLocationInView):
(-[WKWebViewForTestingImmediateActions _immediateActionAnimationControllerForHitTestResult:withType:userData:]):
(-[WKWebViewForTestingImmediateActions immediateActionGesture]):
(-[WKWebViewForTestingImmediateActions simulateImmediateAction:]):

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



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications