Bug 157924 – REGRESSION (r188642): All pages are blank when printing a webpage in iOS Safari

• Component: Printing
• Status: RESOLVED FIXED

[Andy Estes]

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.

[Andy Estes]

rdar://problem/22524550

[Filip Pizlo]

Ouch!

[Geoffrey Garen]

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();

[Filip Pizlo]

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.

[Andy Estes]

(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.

[Filip Pizlo]

(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.

[Andy Estes]

(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

[Andy Estes]

(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.

[Andy Estes]

Created attachment 279475 [details]
Patch

[Brent Fulgham]

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?

[WebKit Commit Bot]

Comment on attachment 279475 [details]
Patch

Clearing flags on attachment: 279475

Committed r201246: <http://trac.webkit.org/changeset/201246>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

(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.

[Brent Fulgham]

(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!