Bug 243264 - Retain cycle after snapshotting a WKWebView displaying a PDF file
Summary: Retain cycle after snapshotting a WKWebView displaying a PDF file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: PDF (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-07-27 13:44 PDT by Ali Juma
Modified: 2022-08-05 07:38 PDT (History)
6 users (show)

See Also:


Attachments
ViewController reproducing the leak (1.57 KB, text/x-objcsrc)
2022-07-27 13:44 PDT, Ali Juma
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2022-07-27 13:44:36 PDT
Created attachment 461260 [details]
ViewController reproducing the leak

When a third-party embedder (that is, any embedder without the com.apple.QuartzCore.global-capture entitlement) calls -[WKWebView takeSnapshotWithConfiguration:completionHandler:], this creates a retain cycle that isn't broken until the WKWebView is navigated away. If, instead, the WKWebView is released by the embedder in this state, the cycle remains forever, and the WKWebView is leaked.

This seems to be a combination of two bugs:
1) In -[WKWebView takeSnapshotWithConfiguration:completionHandler:], the completion handler constructed on this line: https://github.com/WebKit/WebKit/blob/a790b3beee524f688f96f721372dee80a6235e7f/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm#L1247 captures strongSelf without seeming to need it (it's not used in the body of that completion handler).
2) For third-party embedders (for whom +[WKPDFView web_requiresCustomSnapshotting] is YES), the callback from (1) ends up getting passed to PDFHostViewController in -[WKPDFView web_snapshotRectInContentViewCoordinates], and for reasons that aren't clear (hard to tell without source for PDFKit), the callback isn't released even after it is called.

The result is we have a cycle:
1) WKWebView retains WKPDFView (through _customContentView)
2) WKPDFView retains PDFHostViewController (through _hostViewController)
3) PDFHostViewController retains the callback
4) The callback retains WKWebView (because of the strongSelf mentioned earlier)

I've confirmed using Xcode's memory graph that the WKWebView remains alive even if the embedder releases it and removes it from the view hierarchy.

In addition to leaking memory, this is also web-observable if another page has an opener to the leaked WKWebView, since window.closed remains false even though the tab has actually been closed. This reproduces in multiple 3rd-party browsers on iOS (Chrome, Firefox, Brave, Edge).

The simplest fix would be to change https://github.com/WebKit/WebKit/blob/a790b3beee524f688f96f721372dee80a6235e7f/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm#L1247 so that strongSelf isn't captured in the completionHandler constructed at the end of that line.

An alternative would be to figure out why PDFHostViewController isn't releasing the callback, but that's outside the scope of WebKit.

I've attached a simple example of constructing a WKWebView and taking a snapshot, which includes a comment at the point where we can look a the memory graph to see the leak.
Comment 1 Ali Juma 2022-07-29 12:38:17 PDT
Pull request: https://github.com/WebKit/WebKit/pull/2859
Comment 2 EWS 2022-07-29 14:27:23 PDT
Committed 252966@main (b16d7efbd2b6): <https://commits.webkit.org/252966@main>

Reviewed commits have been landed. Closing PR #2859 and removing active labels.
Comment 3 Radar WebKit Bug Importer 2022-07-29 14:28:16 PDT
<rdar://problem/97805012>
Comment 4 Ali Juma 2022-08-05 07:38:04 PDT
I've also filed FB11080384 for having this fix cherry-picked to a release branch.