Bug 119836 - [GTK] ChromeClient::paint is susceptible to system time changes
Summary: [GTK] ChromeClient::paint is susceptible to system time changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Critical
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-15 05:59 PDT by snyh
Modified: 2013-08-21 00:02 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.19 KB, patch)
2013-08-20 13:40 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description snyh 2013-08-15 05:59:45 PDT
In Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:ChromeClient::paint.

There has check timeUntilNextDisplay > 0  to reduce the no necessary painting, but you forget to check timeSinceLastDisplay's value can be an minus!!!

please add 
this code before " if (timeUntilNextDisplay > 0) { "


-------------------------------------------------------------
if (timeSinceLastDisplay < 0) { // We can go back to the past in computer World:)
    timeUntilNextDisplay = 0;
}
------------------------------------------------------------------
Comment 1 Zan Dobersek 2013-08-17 01:27:27 PDT
Do you have a reproducible test case that shows the problem?

Technically, timeSinceLastDisplay can end up being negative, but it would in the worst case simply cause skipping one paint (dropping a frame).
This can be avoided by using WTF::monotonicallyIncreasingTime() instead of WTF::currentTime().
Comment 2 snyh 2013-08-17 07:35:42 PDT
reproducible test case is very easy

set the system time go back to past
Comment 3 snyh 2013-08-19 18:25:35 PDT
It's not only dropping one frame, it freeze the whole GUI rendering!!!(only webkitgtk, qtwebkit hasn't this problem)

You can run any GUI program based on webkitgtk and then adjust system time go back one hour(or more) then any GUI is freeze. 




(In reply to comment #1)
> Do you have a reproducible test case that shows the problem?
> 
> Technically, timeSinceLastDisplay can end up being negative, but it would in the worst case simply cause skipping one paint (dropping a frame).
> This can be avoided by using WTF::monotonicallyIncreasingTime() instead of WTF::currentTime().
Comment 4 snyh 2013-08-19 18:34:17 PDT
m_lastDisplayTime = currentTime() is update too later! (so used m_lastDisplayTime and update it immediately is a good idea) 

So "double timeSinceLastDisplay = currentTime() - m_lastDisplayTime"
timeSinceLastDisplay is always an very big negative number ( minus 3000+ is we go to past one hour) until one hour past!


(In reply to comment #1)
> Do you have a reproducible test case that shows the problem?
> 
> Technically, timeSinceLastDisplay can end up being negative, but it would in the worst case simply cause skipping one paint (dropping a frame).
> This can be avoided by using WTF::monotonicallyIncreasingTime() instead of WTF::currentTime().
Comment 5 Zan Dobersek 2013-08-20 13:37:17 PDT
(In reply to comment #3)
> It's not only dropping one frame, it freeze the whole GUI rendering!!!(only webkitgtk, qtwebkit hasn't this problem)

Yeah, I was wrong - didn't thought of the possibility of changing the system date to irrational values.

(In reply to comment #4)
> m_lastDisplayTime = currentTime() is update too later! (so used m_lastDisplayTime and update it immediately is a good idea) 

That part is just fine - m_lastDisplayTime is assigned the new value when the last display is actually complete.
Comment 6 Zan Dobersek 2013-08-20 13:40:55 PDT
Created attachment 209224 [details]
Patch
Comment 7 Zan Dobersek 2013-08-21 00:02:14 PDT
Comment on attachment 209224 [details]
Patch

Clearing flags on attachment: 209224

Committed r154380: <http://trac.webkit.org/changeset/154380>
Comment 8 Zan Dobersek 2013-08-21 00:02:21 PDT
All reviewed patches have been landed.  Closing bug.