Bug 236811 - [iOS] Safari can sometimes hang while printing due to sync IPC
Summary: [iOS] Safari can sometimes hang while printing due to sync IPC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-17 16:43 PST by Devin Rousso
Modified: 2022-02-18 16:45 PST (History)
8 users (show)

See Also:


Attachments
Patch (24.69 KB, patch)
2022-02-17 17:17 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.12 KB, patch)
2022-02-17 18:16 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (21.29 KB, patch)
2022-02-17 22:22 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[fast-cq] Patch (22.38 KB, patch)
2022-02-18 13:32 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2022-02-17 16:43:05 PST
.
Comment 1 Devin Rousso 2022-02-17 16:46:55 PST
<rdar://problem/56425783>
Comment 2 Devin Rousso 2022-02-17 17:17:11 PST
Created attachment 452450 [details]
Patch
Comment 3 Tim Horton 2022-02-17 17:49:09 PST
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.
Comment 4 Devin Rousso 2022-02-17 17:51:15 PST
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.
Comment 5 Devin Rousso 2022-02-17 18:16:05 PST
Created attachment 452462 [details]
Patch
Comment 6 Aditya Keerthi 2022-02-17 18:44:11 PST
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.
Comment 7 Devin Rousso 2022-02-17 22:22:54 PST
Created attachment 452481 [details]
Patch
Comment 8 Devin Rousso 2022-02-18 13:32:06 PST
Created attachment 452574 [details]
[fast-cq] Patch
Comment 9 EWS 2022-02-18 16:45:29 PST
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].