Skip to content

Commit

Permalink
[iOS] Crash when closing and removing a WKWebView with an active find…
Browse files Browse the repository at this point in the history
… interaction

https://bugs.webkit.org/show_bug.cgi?id=261623
rdar://115543506

Reviewed by Wenson Hsieh.

In 267443@main, `WKWebView` began driving find overlay display logic from the
UI process, rather than the Web process. Specifically, the `CALayer` representing
the overlay is directly manipulated from `WKWebView`, rather than `PageOverlay`.

`-[WKWebView _layerForFindOverlay]` can be used to retrieve the current find
overlay layer, using the `DrawingAreaProxy` and a layer identifier.

The crashing scenario is as follows:
1. Begin a find interaction. This creates a find overlay layer, referenced as `_perProcessState.committedFindLayerID`.
2. Close the `WKWebView` using `-[WKWebView _close]`. This results in the `DrawingAreaProxy` being nulled out.
3. Remove the `WKWebView` from the view hierarchy. This triggers the end of the find interaction.
4. Since the find interaction is ending, the web view attempts to hide the overlay layer.
5. Null dereference of `_page->drawingArea()` as current layer is retrieved prior to hiding.

The underlying assumption that was incorrect here was that `_perProcessState`
gets reset when the `WKWebView` is closed. That would fix the crash, since
`committedFindLayerID` would be unavailable. However, this state is not reset,
presumably since the overall web view state no longer has useful meaning.

To fix, simply null-check the `DrawingAreaProxy` as is done in other parts of
the code. `committedFindLayerID` does not need to be reset, as a new search
cannot be initiated with a closed web view.

* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView _layerForFindOverlay]):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:
(TEST):

Canonical link: https://commits.webkit.org/268044@main
  • Loading branch information
pxlcoder committed Sep 16, 2023
1 parent 5ecf788 commit 4473b14
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
5 changes: 4 additions & 1 deletion Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm
Original file line number Diff line number Diff line change
Expand Up @@ -4392,7 +4392,10 @@ - (CALayer *)_layerForFindOverlay
if (!_page || !_perProcessState.committedFindLayerID)
return nil;

return downcast<WebKit::RemoteLayerTreeDrawingAreaProxy>(*_page->drawingArea()).remoteLayerTreeHost().layerForID(_perProcessState.committedFindLayerID);
if (auto* drawingArea = _page->drawingArea())
return downcast<WebKit::RemoteLayerTreeDrawingAreaProxy>(*drawingArea).remoteLayerTreeHost().layerForID(_perProcessState.committedFindLayerID);

return nil;
}

#endif // HAVE(UIFINDINTERACTION)
Expand Down
23 changes: 23 additions & 0 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,29 @@ static void testPerformTextSearchWithQueryStringInWebView(WKWebView *webView, NS
EXPECT_EQ(overlayCount(webView.get()), 1U);
}

TEST(WebKit, FindOverlayCloseWebViewCrash)
{
auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 200, 200)]);
[webView setFindInteractionEnabled:YES];

NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"lots-of-text" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
[webView loadRequest:request];
[webView _test_waitForDidFinishNavigation];

auto *findInteraction = [webView findInteraction];
[findInteraction presentFindNavigatorShowingReplace:NO];

// Wait for two presentation updates, as the document overlay root layer is
// created lazily.
[webView waitForNextPresentationUpdate];
[webView waitForNextPresentationUpdate];

EXPECT_EQ(overlayCount(webView.get()), 1U);

[webView _close];
[webView removeFromSuperview];
}

TEST(WebKit, FindOverlaySPI)
{
auto findDelegate = adoptNS([[TestFindDelegate alloc] init]);
Expand Down

0 comments on commit 4473b14

Please sign in to comment.