Bug 92009 - Don’t use currentTime() in places where it’s inappropriate
Summary: Don’t use currentTime() in places where it’s inappropriate
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-23 10:42 PDT by Yong Li
Modified: 2013-08-26 23:50 PDT (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2012-07-23 10:42:04 PDT
WebKit condition variable should use monotonic clock, and its consumers should use monotonic time as absolution time. Otherwise, if system date is changed back for a month, some threads could sleep too long...

It seems to me only WebKit is using condition variables only in 2 cases:

1. BlockFreeingThread
2. Timers in Web Worker threads.

The latter uses monotonic time as absolute wait time (see Timer.cpp), but the former uses currentTime.

The solution in my mind is:

1. Make condition variable use absolute time
2. Let BlockFreeingThread uses monotonicallyIncreasingTime()
Comment 1 Yong Li 2012-07-24 10:03:29 PDT
"Make condition variable use absolute time"

should be "Make condition variable use monotonic clock" with pthread_condattr_setclock
Comment 2 Yong Li 2012-07-27 14:21:22 PDT
Even though monotonicallyIncreasingTime() has been introduced for a while, WebKit is still using currentTime at so many places where monotonic time is expected.

It is too hard for one to fix all of these without breaking any port. For example, changing DOM event's timestamp to use monotonicallyIncreasingTime() could break plugins. Making ThreadCondition use monotonic clock will break WebKit2.

BTW, Web Worker's timer is using currentTime(), so it works well with ThreadCondition currently.

But the problem is still there: what if user adjusts system date to 1 month earlier?

Any idea how we can get this thing done smoothly?
Comment 3 Darin Adler 2012-07-27 14:37:31 PDT
(In reply to comment #2)
> But the problem is still there: what if user adjusts system date to 1 month earlier?

A good way to make further use of this insight is to do some testing. We can find out how real these problems are that could happen when the user changes the computer time.

We are experts so we can see the bugs that could exist, but testing makes them more concrete and helps motivate us better than just architectural insight.
Comment 4 Yong Li 2012-07-27 15:07:56 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > But the problem is still there: what if user adjusts system date to 1 month earlier?
> 
> A good way to make further use of this insight is to do some testing. We can find out how real these problems are that could happen when the user changes the computer time.
> 
> We are experts so we can see the bugs that could exist, but testing makes them more concrete and helps motivate us better than just architectural insight.

I've seen changing system date back when Safari win32 is loading a page (like cnn.com, bgr.com) caused the page frozen (cannot be scrolled) for a while (>20 seconds). Not sure if it is related to this issue. Such kind of issues usually take developers quite some time to finally realize it is timer that doesn't work.

One way to show real problems might be running a script that keeps changing system date on a testbot at same time when it is running layout tests. But bugs like BlockFreeingThread never wakes up or takes very long to wake up are very hard to be detected.
Comment 5 Antonio Gomes 2013-08-23 12:23:41 PDT
Adding arurajku@cisco.com so it can be duplicated against the proper bug.
Comment 6 Darin Adler 2013-08-23 13:11:25 PDT
We don’t plan to wipe out currentTime entirely, do we? Retitling assuming that is correct.
Comment 7 Yong Li 2013-08-23 13:29:09 PDT
(In reply to comment #6)
> We don’t plan to wipe out currentTime entirely, do we? Retitling assuming that is correct.

Initially, I thought the only need for currentTime was JS date object, and all other references should be changed to use monotonic time. This assumption was probably wrong. I like the new title.