Bug 157924 - REGRESSION (r188642): All pages are blank when printing a webpage in iOS Safari
Summary: REGRESSION (r188642): All pages are blank when printing a webpage in iOS Safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: Other
Hardware: iPhone / iPad All
: P1 Blocker
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on: 148140
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-19 14:34 PDT by Andy Estes
Modified: 2016-05-21 18:12 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.01 KB, patch)
2016-05-20 03:57 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2016-05-19 14:34:32 PDT
When UIPrintInteractionController asks WKWebView to print a webpage, it does so in several phases. First we're asked to compute the page count, then later we're asked to draw each page into a supplied CGContext in a series of messages.

When WKWebView is asked for the page count, we send a message to the Web process asking 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;

Condition::Clock::now() has a duration in nanoseconds, which causes signed overflow when converted to milliseconds and added to milliseconds::max(). This makes absoluteTimeout end up being less than Condition::Clock::now(), and so instead of waiting indefinitely for the printed data, we timeout immediately and print nothing.
Comment 1 Andy Estes 2016-05-19 14:38:45 PDT
rdar://problem/22524550
Comment 2 Filip Pizlo 2016-05-19 14:39:09 PDT
Ouch!
Comment 3 Geoffrey Garen 2016-05-19 14:43:24 PDT
I think the right solution here is probably to change the "+" to an add with clamping to max on overflow.

One way to do that:

If (absoluteTimeout < timeout)
    absoluteTimeout = Condition::Clock::time_point::max();
Comment 4 Filip Pizlo 2016-05-19 14:52:09 PDT
There are two issues I think:

1) The functional style would have you let WTF::Condition do the time math for you.  Instead of having a wait loop, do:

m_condition.waitFor(m_lock, timeout, [&] () -> bool { loop body });

2) The style that I've been settling on is to just use doubles for time.  Maybe when I have time to mess around I'll propose that we do this.  I've encountered so many bugs due to std::chrono having overflows where our old double-based time code would have recovered like a champ.  In fact, one of those overflows was in GCC's version of libstdc++!  It would cause some uses of std::condition_variable to freak out on Linux but not anywhere else.

In this case, we could just go back to using a double timeout.  waitForSeconds(+Inf) should correctly cause our code to recognize that you want to timeout forever.

I'm also fine with Geoff's proposed solution.
Comment 5 Andy Estes 2016-05-19 16:25:51 PDT
(In reply to comment #4)
> There are two issues I think:
> 
> 1) The functional style would have you let WTF::Condition do the time math
> for you.  Instead of having a wait loop, do:
> 
> m_condition.waitFor(m_lock, timeout, [&] () -> bool { loop body });

Unfortunately this would just move the overflow into Condition::absoluteFromRelative(), where have these two lines:

    Clock::duration myRelativeTimeout =
        std::chrono::duration_cast<Clock::duration>(relativeTimeout);

    return Clock::now() + myRelativeTimeout;

libc++ represents both nanoseconds and milliseconds using the same type (long long), so the duration_cast will overflow trying to convert the largest long long into an even larger number of nanosecond ticks. Now we're right back where we started, subtracting from Clock::now() instead of adding.

> 
> 2) The style that I've been settling on is to just use doubles for time. 
> Maybe when I have time to mess around I'll propose that we do this.  I've
> encountered so many bugs due to std::chrono having overflows where our old
> double-based time code would have recovered like a champ.  In fact, one of
> those overflows was in GCC's version of libstdc++!  It would cause some uses
> of std::condition_variable to freak out on Linux but not anywhere else.
> 
> In this case, we could just go back to using a double timeout. 
> waitForSeconds(+Inf) should correctly cause our code to recognize that you
> want to timeout forever.
> 
> I'm also fine with Geoff's proposed solution.

Yeah, this is basically what I proposed doing in Radar, although now Geoff made me realize that my patch has a totally unnecessary subtraction in it!

Thanks for the feedback, Geoff and Phil.
Comment 6 Filip Pizlo 2016-05-19 16:26:52 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > There are two issues I think:
> > 
> > 1) The functional style would have you let WTF::Condition do the time math
> > for you.  Instead of having a wait loop, do:
> > 
> > m_condition.waitFor(m_lock, timeout, [&] () -> bool { loop body });
> 
> Unfortunately this would just move the overflow into
> Condition::absoluteFromRelative(), where have these two lines:
> 
>     Clock::duration myRelativeTimeout =
>         std::chrono::duration_cast<Clock::duration>(relativeTimeout);
> 
>     return Clock::now() + myRelativeTimeout;
> 
> libc++ represents both nanoseconds and milliseconds using the same type
> (long long), so the duration_cast will overflow trying to convert the
> largest long long into an even larger number of nanosecond ticks. Now we're
> right back where we started, subtracting from Clock::now() instead of adding.

Can you file a bug about that? :-)

> 
> > 
> > 2) The style that I've been settling on is to just use doubles for time. 
> > Maybe when I have time to mess around I'll propose that we do this.  I've
> > encountered so many bugs due to std::chrono having overflows where our old
> > double-based time code would have recovered like a champ.  In fact, one of
> > those overflows was in GCC's version of libstdc++!  It would cause some uses
> > of std::condition_variable to freak out on Linux but not anywhere else.
> > 
> > In this case, we could just go back to using a double timeout. 
> > waitForSeconds(+Inf) should correctly cause our code to recognize that you
> > want to timeout forever.
> > 
> > I'm also fine with Geoff's proposed solution.
> 
> Yeah, this is basically what I proposed doing in Radar, although now Geoff
> made me realize that my patch has a totally unnecessary subtraction in it!
> 
> Thanks for the feedback, Geoff and Phil.
Comment 7 Andy Estes 2016-05-19 17:56:03 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > There are two issues I think:
> > > 
> > > 1) The functional style would have you let WTF::Condition do the time math
> > > for you.  Instead of having a wait loop, do:
> > > 
> > > m_condition.waitFor(m_lock, timeout, [&] () -> bool { loop body });
> > 
> > Unfortunately this would just move the overflow into
> > Condition::absoluteFromRelative(), where have these two lines:
> > 
> >     Clock::duration myRelativeTimeout =
> >         std::chrono::duration_cast<Clock::duration>(relativeTimeout);
> > 
> >     return Clock::now() + myRelativeTimeout;
> > 
> > libc++ represents both nanoseconds and milliseconds using the same type
> > (long long), so the duration_cast will overflow trying to convert the
> > largest long long into an even larger number of nanosecond ticks. Now we're
> > right back where we started, subtracting from Clock::now() instead of adding.
> 
> Can you file a bug about that? :-)

Sure thing! https://bugs.webkit.org/show_bug.cgi?id=157937
Comment 8 Andy Estes 2016-05-20 03:24:49 PDT
(In reply to comment #3)
> I think the right solution here is probably to change the "+" to an add with
> clamping to max on overflow.
> 
> One way to do that:
> 
> If (absoluteTimeout < timeout)
>     absoluteTimeout = Condition::Clock::time_point::max();

Sorry, I spoke too soon. This ends up not working because the operator< for durations converts both operands into a common duration type before making the comparison. In this case the common duration is nanoseconds, which we already know milliseconds::max() will overflow on conversion.

What I ended up doing was to compute the amount of time remaining on the clock, convert that duration to milliseconds, then pick the smaller of that value and the timeout for computing absoluteTimeout. The conversion will reduce the maximum possible timeout by up to 1 millisecond, but that seems harmless.
Comment 9 Andy Estes 2016-05-20 03:57:17 PDT
Created attachment 279475 [details]
Patch
Comment 10 Brent Fulgham 2016-05-21 16:23:00 PDT
Comment on attachment 279475 [details]
Patch

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

I would approve this patch if I was a WK2 owner. r=me, though this carries no weight. :-)

> Source/WebKit2/ChangeLog:5
> +        rdar://problem/22524550

This should be <rdar://problem/22524550>

> Source/WebKit2/Platform/IPC/Connection.cpp:438
> +    auto absoluteTimeout = now + std::min(remainingClockTime, timeout);

Given how likely we are to make this mistake again, I wonder if we could create a Condition::Clock method, something like Condition::Clock::absoluteTimeout(timeout) or something, then police (in a separate patch) places where this type of calculation is needed.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:-4557
> -

It's funny that we had this calculation. Did std::chromo::milliseconds::max() not exist in earlier clang or something?
Comment 11 WebKit Commit Bot 2016-05-21 16:51:34 PDT
Comment on attachment 279475 [details]
Patch

Clearing flags on attachment: 279475

Committed r201246: <http://trac.webkit.org/changeset/201246>
Comment 12 WebKit Commit Bot 2016-05-21 16:51:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Andy Estes 2016-05-21 17:45:03 PDT
(In reply to comment #10)
> Comment on attachment 279475 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279475&action=review
> 
> I would approve this patch if I was a WK2 owner. r=me, though this carries
> no weight. :-)
> 
> > Source/WebKit2/ChangeLog:5
> > +        rdar://problem/22524550
> 
> This should be <rdar://problem/22524550>

I let the commit queue land my patch without changing this. Is there a reason we need to add the brackets? Do some of our tools expect them?

> 
> > Source/WebKit2/Platform/IPC/Connection.cpp:438
> > +    auto absoluteTimeout = now + std::min(remainingClockTime, timeout);
> 
> Given how likely we are to make this mistake again, I wonder if we could
> create a Condition::Clock method, something like
> Condition::Clock::absoluteTimeout(timeout) or something, then police (in a
> separate patch) places where this type of calculation is needed.

That would be a good idea, but I wanted to keep this patch constrained to the issue at hand. Condition::Clock already has a relativeToAbsolute() function, and https://bugs.webkit.org/show_bug.cgi?id=157937 is for fixing a similar bug in it. It sounds like Phil might want to make even more sweeping changes, like using doubles instead of std::chrono, so I'll leave the discussion of how to systematically prevent these issues occur there.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:-4557
> > -
> 
> It's funny that we had this calculation. Did
> std::chromo::milliseconds::max() not exist in earlier clang or something?

No, that was a previous fix to this issue back when we were using std::condition_variable. We encountered the same milliseconds->nanoseconds overflow bug back then, we just didn't have the overflow-on-addition issue since std::condition_variable took a relative timeout.
Comment 14 Brent Fulgham 2016-05-21 18:12:38 PDT
(In reply to comment #13)
> > > Source/WebKit2/ChangeLog:5
> > > +        rdar://problem/22524550
> > 
> > This should be <rdar://problem/22524550>
> 
> I let the commit queue land my patch without changing this. Is there a
> reason we need to add the brackets? Do some of our tools expect them?

I know that is the form our tools produce. I think this style was created because that was how copy/paste from Radar used to format it.

At any rate, no need to change this.

> function, and https://bugs.webkit.org/show_bug.cgi?id=157937 is for fixing a
> similar bug in it. It sounds like Phil might want to make even more sweeping
> changes, like using doubles instead of std::chrono, so I'll leave the
> discussion of how to systematically prevent these issues occur there.

Sounds good!