Bug 237835

Summary:

[iOS] Indefinite hang when printing using a UIPrintPageRenderer

Product:

WebKit

Reporter:

Aditya Keerthi <akeerthi>

Component:

Printing

Assignee:

Aditya Keerthi <akeerthi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, cmarcelo, ews-watchlist, hi, katherine_cheney, megan_gardner, thorton, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

iPhone / iPad

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none

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].