Bug 237835 - [iOS] Indefinite hang when printing using a UIPrintPageRenderer
Summary: [iOS] Indefinite hang when printing using a UIPrintPageRenderer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: Other
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-14 10:00 PDT by Aditya Keerthi
Modified: 2022-03-15 10:40 PDT (History)
10 users (show)

See Also:

Attachments
Patch (19.17 KB, patch)
2022-03-14 10:04 PDT, Aditya Keerthi 		no flags Details | Formatted Diff | Diff
Patch for landing (21.05 KB, patch)
2022-03-14 13:30 PDT, Aditya Keerthi 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2022-03-14 10:00:49 PDT 
...
Comment 1 Aditya Keerthi 2022-03-14 10:01:04 PDT 
rdar://90002387
Comment 2 Aditya Keerthi 2022-03-14 10:04:03 PDT 
Created attachment 454602 [details]
Patch
Comment 3 Devin Rousso 2022-03-14 10:54:37 PDT 
Comment on attachment 454602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454602&action=review

r=me, thanks for fixing!

> Source/WebKit/UIProcess/ios/WKContentView.mm:692
> +    if (_pdfPrintCallbackID) {

NIT: Do we need to check for `_pdfPrintCallbackID` here?  I feel like the inner `if` is enough.

> Source/WebKit/UIProcess/ios/WKContentView.mm:932
> +        _pdfPrintCompletionSemaphore = makeUnique<BinarySemaphore>();

NIT: should we `ASSERT(!_pdfPrintCompletionSemaphore);`?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewPrintFormatter.mm:99
> +TEST(WKWebView, PrintToPDF)

Is there a way for us to test the non-main-thread path as well?
Comment 4 Aditya Keerthi 2022-03-14 13:30:24 PDT 
Created attachment 454618 [details]
Patch for landing
Comment 5 Aditya Keerthi 2022-03-14 13:32:42 PDT 
(In reply to Devin Rousso from comment #3)
> Comment on attachment 454602 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454602&action=review
> 
> r=me, thanks for fixing!

Thanks for the review!
 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:692
> > +    if (_pdfPrintCallbackID) {
> 
> NIT: Do we need to check for `_pdfPrintCallbackID` here?  I feel like the
> inner `if` is enough.

Removed, the inner `if` is enough due to the null check.
 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:932
> > +        _pdfPrintCompletionSemaphore = makeUnique<BinarySemaphore>();
> 
> NIT: should we `ASSERT(!_pdfPrintCompletionSemaphore);`?

Added an assertion.
 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewPrintFormatter.mm:99
> > +TEST(WKWebView, PrintToPDF)
> 
> Is there a way for us to test the non-main-thread path as well?

Added a test which exercises the non-main-thread path using UIPrintInteractionController.
Comment 6 EWS 2022-03-15 10:40:40 PDT 
Committed r291304 (248443@main): <https://commits.webkit.org/248443@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454618 [details].