Changeset 249584 in webkit


Ignore:
Timestamp:
Sep 6, 2019 11:46:14 AM (5 years ago)
Author:
timothy_horton@apple.com
Message:

Marking up a note on iOS results in a PDF with no contents
https://bugs.webkit.org/show_bug.cgi?id=201530
<rdar://problem/53686019>

Reviewed by Andy Estes.

Source/WebKit:

  • Platform/IPC/Connection.cpp:

(IPC::Connection::Connection):
(IPC::Connection::waitForMessage):
(IPC::Connection::connectionDidClose):

  • Platform/IPC/Connection.h:

If the main thread is blocked when the Web Content process dies, and
something eventually calls waitForAndDispatchImmediately without
returning control to the main run loop, we will wait for the full timeout,
because a) the code to mark the connection invalid is dispatched
to the main thread, and b) the secondary thread that is informed of
the Web Content process dying did not yet have a "waiting for" message
to mark as interrupted (because it wasn't waiting yet).

Fix this race by adding a bit that is set under the waitForMessage lock
on the secondary thread when the connection is invalidated, identically
to m_shouldWaitForSyncReplies, which solves the same problem for sync
messages.

Read the new bit when we are about to start waiting, and bail if it is set.
It's OK to not read it inside the loop because we are guaranteed to have
waitForMessage set at that point, so the normal interruption bit will work.

  • UIProcess/ios/WKContentView.mm:

(-[WKContentView _processDidExit]):
Reset _isPrintingToPDF; the Web Content process is never going to get
back to us if it crashes.

(-[WKContentView _wk_pageCountForPrintFormatter:]):
Do not bail from starting a printing operation if one is already occurring.
This fixes the original bug, because Markup ends up invalidating the page
count at least one extra time before asking for the printed document.
Instead of maintaining the fragile requirement that you cannot recompute
the page count while printing, just let it happen. In order to make this
work safely, synchronously wait for the previous printed result before
continuing with the next print.

We could do more coalescing here if need be, but calls to -_recalcPageCount
are not high in volume.

Tools:

  • TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
  • TestWebKitAPI/Tests/WebKitCocoa/WKWebViewPrintFormatter.mm:

Add some tests for WKWebViewPrintFormatter; specifically that it is
possible to _recalcPageCount twice in quick succession, and that
we don't hang if we start painting the printed content immediately
after a Web Content process crash.

Location:
trunk
Files:
1 added
6 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r249580 r249584  
     12019-09-06  Tim Horton  <timothy_horton@apple.com>
     2
     3        Marking up a note on iOS results in a PDF with no contents
     4        https://bugs.webkit.org/show_bug.cgi?id=201530
     5        <rdar://problem/53686019>
     6
     7        Reviewed by Andy Estes.
     8
     9        * Platform/IPC/Connection.cpp:
     10        (IPC::Connection::Connection):
     11        (IPC::Connection::waitForMessage):
     12        (IPC::Connection::connectionDidClose):
     13        * Platform/IPC/Connection.h:
     14        If the main thread is blocked when the Web Content process dies, and
     15        something eventually calls waitForAndDispatchImmediately without
     16        returning control to the main run loop, we will wait for the full timeout,
     17        because a) the code to mark the connection invalid is dispatched
     18        to the main thread, and b) the secondary thread that is informed of
     19        the Web Content process dying did not yet have a "waiting for" message
     20        to mark as interrupted (because it wasn't waiting yet).
     21
     22        Fix this race by adding a bit that is set under the waitForMessage lock
     23        on the secondary thread when the connection is invalidated, identically
     24        to m_shouldWaitForSyncReplies, which solves the same problem for sync
     25        messages.
     26
     27        Read the new bit when we are about to start waiting, and bail if it is set.
     28        It's OK to not read it inside the loop because we are guaranteed to have
     29        waitForMessage set at that point, so the normal interruption bit will work.
     30
     31        * UIProcess/ios/WKContentView.mm:
     32        (-[WKContentView _processDidExit]):
     33        Reset _isPrintingToPDF; the Web Content process is never going to get
     34        back to us if it crashes.
     35
     36        (-[WKContentView _wk_pageCountForPrintFormatter:]):
     37        Do not bail from starting a printing operation if one is already occurring.
     38        This fixes the original bug, because Markup ends up invalidating the page
     39        count at least one extra time before asking for the printed document.
     40        Instead of maintaining the fragile requirement that you cannot recompute
     41        the page count while printing, just let it happen. In order to make this
     42        work safely, synchronously wait for the previous printed result before
     43        continuing with the next print.
     44
     45        We could do more coalescing here if need be, but calls to -_recalcPageCount
     46        are not high in volume.
     47
    1482019-09-06  Alex Christensen  <achristensen@webkit.org>
    249
  • trunk/Source/WebKit/Platform/IPC/Connection.cpp

    r248846 r249584  
    268268    , m_didReceiveInvalidMessage(false)
    269269    , m_shouldWaitForSyncReplies(true)
     270    , m_shouldWaitForMessages(true)
    270271{
    271272    ASSERT(RunLoop::isMain());
     
    487488        ASSERT(!m_waitingForMessage);
    488489        if (m_waitingForMessage)
     490            return nullptr;
     491
     492        // If the connection is already invalidated, don't even start waiting.
     493        // Once m_waitingForMessage is set, messageWaitingInterrupted will cover this instead.
     494        if (!m_shouldWaitForMessages)
    489495            return nullptr;
    490496
     
    784790    {
    785791        std::lock_guard<Lock> lock(m_waitForMessageMutex);
     792
     793        ASSERT(m_shouldWaitForMessages);
     794        m_shouldWaitForMessages = false;
     795
    786796        if (m_waitingForMessage)
    787797            m_waitingForMessage->messageWaitingInterrupted = true;
  • trunk/Source/WebKit/Platform/IPC/Connection.h

    r249379 r249584  
    353353    Lock m_syncReplyStateMutex;
    354354    bool m_shouldWaitForSyncReplies;
     355    bool m_shouldWaitForMessages;
    355356    struct PendingSyncReply;
    356357    Vector<PendingSyncReply> m_pendingSyncReplies;
  • trunk/Source/WebKit/UIProcess/ios/WKContentView.mm

    r249492 r249584  
    555555    [self _removeVisibilityPropagationView];
    556556#endif
     557
     558    _isPrintingToPDF = NO;
    557559}
    558560
     
    698700{
    699701    if (_isPrintingToPDF)
    700         return 0;
     702        [self _waitForDrawToPDFCallback];
    701703
    702704    WebCore::FrameIdentifier frameID;
     
    741743}
    742744
     745- (BOOL)_waitForDrawToPDFCallback
     746{
     747    if (!_page->process().connection()->waitForAndDispatchImmediately<Messages::WebPageProxy::DrawToPDFCallback>(_page->webPageID(), Seconds::infinity())) {
     748        ASSERT_NOT_REACHED();
     749        return false;
     750    }
     751    ASSERT(!_isPrintingToPDF);
     752    return true;
     753}
     754
    743755- (CGPDFDocumentRef)_wk_printedDocument
    744756{
    745757    if (_isPrintingToPDF) {
    746         if (!_page->process().connection()->waitForAndDispatchImmediately<Messages::WebPageProxy::DrawToPDFCallback>(_page->webPageID(), Seconds::infinity())) {
    747             ASSERT_NOT_REACHED();
     758        if (![self _waitForDrawToPDFCallback])
    748759            return nullptr;
    749         }
    750         ASSERT(!_isPrintingToPDF);
    751760    }
    752761
  • trunk/Tools/ChangeLog

    r249582 r249584  
     12019-09-06  Tim Horton  <timothy_horton@apple.com>
     2
     3        Marking up a note on iOS results in a PDF with no contents
     4        https://bugs.webkit.org/show_bug.cgi?id=201530
     5        <rdar://problem/53686019>
     6
     7        Reviewed by Andy Estes.
     8
     9        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     10        * TestWebKitAPI/Tests/WebKitCocoa/WKWebViewPrintFormatter.mm:
     11        Add some tests for WKWebViewPrintFormatter; specifically that it is
     12        possible to _recalcPageCount twice in quick succession, and that
     13        we don't hang if we start painting the printed content immediately
     14        after a Web Content process crash.
     15
    1162019-09-06  Matt Lewis  <jlewis3@apple.com>
    217
  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r249535 r249584  
    122122                297234B7173AFAC700983601 /* CustomProtocolsInvalidScheme_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 297234B5173AFAC700983601 /* CustomProtocolsInvalidScheme_Bundle.cpp */; };
    123123                2D00065F1C1F589A0088E6A7 /* WKPDFViewResizeCrash.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2D00065D1C1F58940088E6A7 /* WKPDFViewResizeCrash.mm */; };
     124                2D01D06E23218FEE0039AA3A /* WKWebViewPrintFormatter.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2D01D06D23218FEE0039AA3A /* WKWebViewPrintFormatter.mm */; };
    124125                2D08E9372267D0F4002518DA /* ReparentWebViewTimeout.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2D08E9362267D0F3002518DA /* ReparentWebViewTimeout.mm */; };
    125126                2D1646E21D1862CD00015A1A /* DeferredViewInWindowStateChange.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2D1646E11D1862CD00015A1A /* DeferredViewInWindowStateChange.mm */; };
     
    15661567                29AB8AA3164C7A9300D49BEC /* TestBrowsingContextLoadDelegate.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = TestBrowsingContextLoadDelegate.h; sourceTree = "<group>"; };
    15671568                2D00065D1C1F58940088E6A7 /* WKPDFViewResizeCrash.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WKPDFViewResizeCrash.mm; sourceTree = "<group>"; };
     1569                2D01D06D23218FEE0039AA3A /* WKWebViewPrintFormatter.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WKWebViewPrintFormatter.mm; sourceTree = "<group>"; };
    15681570                2D08E9362267D0F3002518DA /* ReparentWebViewTimeout.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ReparentWebViewTimeout.mm; sourceTree = "<group>"; };
    15691571                2D1646E11D1862CD00015A1A /* DeferredViewInWindowStateChange.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = DeferredViewInWindowStateChange.mm; path = WebKit/DeferredViewInWindowStateChange.mm; sourceTree = "<group>"; };
     
    29272929                                F4106C6821ACBF84004B89A1 /* WKWebViewFirstResponderTests.mm */,
    29282930                                D3BE5E341E4CE85E00FD563A /* WKWebViewGetContents.mm */,
     2931                                2D01D06D23218FEE0039AA3A /* WKWebViewPrintFormatter.mm */,
    29292932                                37A9DBE7213B4C9300D261A2 /* WKWebViewServerTrustKVC.mm */,
    29302933                                93F56DA81E5F9181003EDE84 /* WKWebViewSnapshot.mm */,
     
    48394842                                F4FA91811E61849B007B8C1D /* WKWebViewMacEditingTests.mm in Sources */,
    48404843                                1CACADA1230620AE0007D54C /* WKWebViewOpaque.mm in Sources */,
     4844                                2D01D06E23218FEE0039AA3A /* WKWebViewPrintFormatter.mm in Sources */,
    48414845                                37A9DBE9213B4C9300D261A2 /* WKWebViewServerTrustKVC.mm in Sources */,
    48424846                                93F56DA91E5F919D003EDE84 /* WKWebViewSnapshot.mm in Sources */,
Note: See TracChangeset for help on using the changeset viewer.