Bug 236811 – [iOS] Safari can sometimes hang while printing due to sync IPC

• Component: Printing
• Status: RESOLVED FIXED

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/56425783>

[Devin Rousso]

Created attachment 452450 [details]
Patch

[Tim Horton]

Comment on attachment 452450 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.h:1418
> +    uint64_t drawToPDFiOS(WebCore::FrameIdentifier, const PrintInfo&, size_t pageCount, CompletionHandler<void(const IPC::SharedBufferCopy&)>&&);

This name is not great (should be about printing, ideally, and not mention a platform). But you're just propagating it.

> Source/WebKit/UIProcess/_WKWebViewPrintFormatter.h:39
> +@property (nonatomic, readonly) BOOL requiresMainThread WK_API_AVAILABLE(ios(WK_IOS_TBA));

I don't think you need this.

[Devin Rousso]

Comment on attachment 452450 [details]
Patch

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

> Source/WebKit/UIProcess/_WKWebViewPrintFormatter.h:39
> +@property (nonatomic, readonly) BOOL requiresMainThread WK_API_AVAILABLE(ios(WK_IOS_TBA));

@Tim Horton pointed out that we don't need to do this to override the method.  I think I was in "C++ mode" when I wrote this part 😅  I will remove.

[Devin Rousso]

Created attachment 452462 [details]
Patch

[Aditya Keerthi]

Comment on attachment 452462 [details]
Patch

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

> Source/WebKit/ChangeLog:18
> +        When `requiresMainThread` is `NO`, use `WTF::BinarySemaphore` (and a wapper helper method)

"wapper"

> Source/WebKit/UIProcess/_WKWebViewPrintFormatter.mm:92
> +    [self._webView._printProvider _wk_calculatePageCountForPrintFormatter:self completionHandler:makeBlockPtr([&] (NSUInteger pageCount) {

Maybe we should move the synchronization (semaphore usage) into `_wk_calculatePageCountForPrintFormatter` in `WKContentView` and rename to make it clear the method is synchronous.

The completion handler here makes things a little confusing.

> Source/WebKit/UIProcess/_WKWebViewPrintFormatter.mm:93
> +        result = pageCount;

I think we should add an `ASSERT(isMainRunLoop());` here.

> Source/WebKit/UIProcess/_WKWebViewPrintFormatter.mm:126
> +        callOnMainRunLoopAndWait([&] {

This looks incorrect.

If we have a background thread, we also want to draw on the background thread. This is because `_printedDocument = self._webView._printProvider._wk_printedDocument;` can cause a hang as we wait for the PDF to be returned from the WebProcess, under `_waitForDrawToPDFCallback`.

We should call _internalDrawInRect:forPageAtIndex: on a background thread, but will need additional synchronization as the reply to `drawToPDFiOS` (which sets the printed document) would be received on the main thread.

> Source/WebKit/UIProcess/_WKWebViewPrintFormatterInternal.h:40
> +@property (nonatomic, readonly) BOOL _wk_requiresMainThread;

Maybe `_wk_printFormatterRequiresMainThread`? It's a little unclear in classes that implement, that the method is about printing.

[Devin Rousso]

Created attachment 452481 [details]
Patch

[Devin Rousso]

Created attachment 452574 [details]
[fast-cq] Patch

[EWS]

Committed r290186 (247514@main): <https://commits.webkit.org/247514@main>

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