Changeset 201246 in webkit

Timestamp:

May 21, 2016 4:52:37 PM (8 years ago)

Author:

aestes@apple.com

Message:

REGRESSION (r188642): All pages are blank when printing a webpage in iOS Safari
​https://bugs.webkit.org/show_bug.cgi?id=157924
rdar://problem/22524550

Reviewed by Sam Weinig.

When UIPrintInteractionController asks WKWebView to print a webpage, it does so in several phases. First we're
asked to compute the page count, followed later by a series of messages asking us to draw each page into a
provided CGContext.

When asked for the page count, we send a message to the Web process instructing it to compute and
return the page count synchronously and then immediately start drawing the page for printing. If the drawing has
finished by the time we're asked to print the first page, then we can do so without waiting. But if it hasn't
then we block by calling Connection::waitForMessage(), passing std::chromo::milliseconds::max() as the relative
timeout.

Prior to r188642, Connection::waitForMessage() called std::condition_variable::wait_for(), which takes a
relative timeout value. r188642 replaced this with WTF::Condition::waitUntil(), which takes an absolute timeout
instead. To convert from relative to absolute, this line was added to Connection::waitForMessage():

Condition::Clock::time_point absoluteTimeout = Condition::Clock::now() + timeout;

std::chrono will convert both operands to a common duration type before performing the addition. When timeout
equals something very large, like milliseconds::max(), this conversion results in signed integer overflow,
giving absoluteTimeout a value less than Clock::now() and making waitForMessage time out immediately.

To fix this, compute how many milliseconds remain on our clock, and add the smaller of that and the timeout
value to Clock::now() to arrive at an absolute timeout.

• Platform/IPC/Connection.cpp:

(IPC::Connection::waitForMessage):

• UIProcess/API/Cocoa/WKWebView.mm:

(-[WKWebView _printedDocument]): Removed an unnecessary nanoseconds-to-milliseconds conversion.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• Platform/IPC/Connection.cpp (modified) (1 diff)
• UIProcess/API/Cocoa/WKWebView.mm (modified) (2 diffs)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2016-05-21  Andy Estes  <aestes@apple.com>
+
+        REGRESSION (r188642): All pages are blank when printing a webpage in iOS Safari
+        https://bugs.webkit.org/show_bug.cgi?id=157924
+        rdar://problem/22524550
+
+        Reviewed by Sam Weinig.
+
+        When UIPrintInteractionController asks WKWebView to print a webpage, it does so in several phases. First we're
+        asked to compute the page count, followed later by a series of messages asking us to draw each page into a
+        provided CGContext.
+
+        When asked for the page count, we send a message to the Web process instructing it to compute and
+        return the page count synchronously and then immediately start drawing the page for printing. If the drawing has
+        finished by the time we're asked to print the first page, then we can do so without waiting. But if it hasn't
+        then we block by calling Connection::waitForMessage(), passing std::chromo::milliseconds::max() as the relative
+        timeout.
+        
+        Prior to r188642, Connection::waitForMessage() called std::condition_variable::wait_for(), which takes a
+        relative timeout value. r188642 replaced this with WTF::Condition::waitUntil(), which takes an absolute timeout
+        instead. To convert from relative to absolute, this line was added to Connection::waitForMessage():
+
+            Condition::Clock::time_point absoluteTimeout = Condition::Clock::now() + timeout;
+
+        std::chrono will convert both operands to a common duration type before performing the addition. When timeout
+        equals something very large, like milliseconds::max(), this conversion results in signed integer overflow,
+        giving absoluteTimeout a value less than Clock::now() and making waitForMessage time out immediately.
+        
+        To fix this, compute how many milliseconds remain on our clock, and add the smaller of that and the timeout
+        value to Clock::now() to arrive at an absolute timeout.
+
+        * Platform/IPC/Connection.cpp:
+        (IPC::Connection::waitForMessage):
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _printedDocument]): Removed an unnecessary nanoseconds-to-milliseconds conversion.
+
 2016-05-20  Enrica Casucci  <enrica@apple.com>
 

trunk/Source/WebKit2/Platform/IPC/Connection.cpp

@@ -433 +433 @@
     }
 
+    // Clamp the timeout to however much time is remaining on our clock.
+    auto now = Condition::Clock::now();
+    auto remainingClockTime = std::chrono::duration_cast<std::chrono::milliseconds>(Condition::Clock::time_point::max() - now);
+    auto absoluteTimeout = now + std::min(remainingClockTime, timeout);
+
     // Now wait for it to be set.
-    Condition::Clock::time_point absoluteTimeout = Condition::Clock::now() + timeout;
     while (true) {
         std::unique_lock<Lock> lock(m_waitForMessageMutex);

trunk/Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm

@@ -4551 +4551 @@
 }
 
-// FIXME: milliseconds::max() overflows when converted to nanoseconds, causing condition_variable::wait_for() to believe
-// a timeout occurred on any spurious wakeup. Use nanoseconds::max() (converted to ms) to avoid this. We should perhaps
-// change waitForAndDispatchImmediately() to take nanoseconds to avoid this issue.
-static constexpr std::chrono::milliseconds didFinishLoadingTimeout = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::nanoseconds::max());
-
 - (CGPDFDocumentRef)_printedDocument
 {
@@ -4564 +4559 @@
 
     if (_pageIsPrintingToPDF) {
-        if (!_page->process().connection()->waitForAndDispatchImmediately<Messages::WebPageProxy::DidFinishDrawingPagesToPDF>(_page->pageID(), didFinishLoadingTimeout)) {
+        if (!_page->process().connection()->waitForAndDispatchImmediately<Messages::WebPageProxy::DidFinishDrawingPagesToPDF>(_page->pageID(), std::chrono::milliseconds::max())) {
             ASSERT_NOT_REACHED();
             return nullptr;