New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 293736 link

Starred by 1 user

Issue metadata

Status: Verified
Owner: ----
Closed: Oct 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue chrome-os-partner:22627



Sign in to add a comment

WaitableEvent::TimedWait uses system time on posix instead of monotonic time (Time instead of TimeTicks)

Project Member Reported by piman@chromium.org, Sep 17 2013

Issue description

This causes https://code.google.com/p/chrome-os-partner/issues/detail?id=22627

MessagePumpDefault uses WaitableEvent::TimedWait when waiting for a delayed task. On posix, this uses Time::Now which is system time (in the WaitableEvent logic, as well as the underlying ConditionVariable, which uses pthread_cond_timedwait at the end of the day, which takes an absolute system time deadline). If system time goes back (e.g. ntpdate/tlsdate), then WaitableEvent::TimedWait may wait for longer (possibly years longer) than the deadline.

Note that often, the MessagePumpDefault may wake up anyway if a non-delayed task is posted, at which point we'll check the TimeTicks to see if we need to run (belatedly) the delayed tasks.

r65322 changed the MessageLoops to use TimeTicks. I think we need to propagate that to WaitabeEvent, somehow.
 

Comment 1 by piman@chromium.org, Sep 17 2013

Blocking: chrome-os-partner:22627
Status: Available
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 19 2013

------------------------------------------------------------------------
r224074 | piman@chromium.org | 2013-09-19T07:18:08.839781Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_thread_impl.cc?r1=224074&r2=224073&pathrev=224074

Use IO loop for the compositor thread in the renderer on posix.

MessagePumpDefault breaks PostDelayedTask on posix because it uses system time.
MessagePumpIO uses monotonic time, so is immune to these problems.

(These threads because they schedule things with PostDelayedTask).

BUG= 293736 

Review URL: https://chromiumcodereview.appspot.com/24234002
------------------------------------------------------------------------
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 2 2013

------------------------------------------------------------------------
r226378 | piman@chromium.org | 2013-10-02T01:25:21.304463Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/synchronization/condition_variable_unittest.cc?r1=226378&r2=226377&pathrev=226378
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/synchronization/waitable_event_posix.cc?r1=226378&r2=226377&pathrev=226378
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/synchronization/condition_variable_posix.cc?r1=226378&r2=226377&pathrev=226378

Fix WaitableEvent and ConditionVariable::TimedWait to use monotonic time on non-Mac non-NaCl posix

Both use a relative time, and it's important that this relative time is consistent with the wall clock when the system time gets adjusted (e.g. NTP, tlsdate, etc.). The monotonic clock, when available, has that property. Unfortunately, by default, pthread_cond_timedwait takes the system time which doesn't have that property.

On Linux/Chrome OS, we use pthread_condattr_setclock which lets us use the monotonic clock. On Android, we use the non-portable pthread_cond_timedwait_monotonic_np which has the same effect.
Unfortunately, neither is supported by NaCl.
Mac can use the non-portable pthread_cond_timedwait_relative_np which uses a relative time.

BUG= 293736 
R=thakis@chromium.org

Review URL: https://codereview.chromium.org/24158005
------------------------------------------------------------------------

Comment 6 by piman@chromium.org, Oct 2 2013

Status: Fixed
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 2 2013

------------------------------------------------------------------------
r226425 | piman@chromium.org | 2013-10-02T07:52:33.031332Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_thread_impl.cc?r1=226425&r2=226424&pathrev=226425
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/compositor/compositor.cc?r1=226425&r2=226424&pathrev=226425

Revert r223961 and r224074 (using IO message loop instead of DEFAULT).

MessagePumpDefault was incorrectly using system time instead of monotonic time
for PostDelayedTask. This was fixed in r226378 so we don't need this workaround
any more.

BUG= 293736 

Review URL: https://codereview.chromium.org/25628004
------------------------------------------------------------------------

Comment 8 by krisr@chromium.org, Oct 17 2013

Status: Verified

Sign in to add a comment