Fix race condition in EventTimerPosix

Fix race condition in EventTimerPosix

The intended signalling from StartTimer() to Process() is that
created_at_.tv_sec is set to 0, and timer_event_->Set() is then called
in order to wake the process thread from timer_event_->Wait(). When this
happens the process thread will return early and the run Process()
again. This time it will pick up created_at_.tv_sec = 0 and run a new
Wait() call with the desired end time.

However if the process thread was NOT blocking on timer_event_->Wait()
when timer_event_->Set() was called from StartTimer() it will mean that
the first call to timer_event_->Wait() from Process(), AFTER the new
time has been configured (count_ = 1), will return early.

If the timer is not periodic it means that Set() will never be called,
and any calls will Wait() will block until the time out.

The solution is to always reset the event in timer_event_ on the first
call to timerEvent_->Wait(), after a timer has started.

Also some general cleanup.

BUG=

[sprang_webrtc, 2016-03-17 10:32:29]

sprang@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2016-03-17 10:43:46]

Functionality LG, but how can we test / make sure this works well?

[sprang_webrtc, 2016-03-17 16:13:13]

Added a few unit tests. The last one fails without the changes to
EventTimerPosix.

[sprang_webrtc, 2016-03-17 16:13:32]

Description was changed from

==========
Fix race condition in EventTimePosix

The intended signalling from StartTimer() to Process() is that
created_at_.tv_sec is set to 0, and timer_event_->Set() is then called
in order to wake the process thread from timer_event_->Wait(). When this
happens the process thread will return early and the run Process()
again. This time it will pick up created_at_.tv_sec = 0 and run a new
Wait() call with the desired end time.

However if the process thread was NOT blocking on timer_event_->Wait()
when timer_event_->Set() was called from StartTimer() it will mean that
the first call to timer_event_->Wait() from Process(), AFTER the new
time has been configured (count_ = 1), will return early.

If the timer is not periodic it means that Set() will never be called,
and any calls will Wait() will block until the time out.

The solution is to always reset the event in timer_event_ on the first
call to timerEvent_->Wait(), after a timer has started.

Also some general cleanup.

BUG=
==========

to

==========
Fix race condition in EventTimerPosix

The intended signalling from StartTimer() to Process() is that
created_at_.tv_sec is set to 0, and timer_event_->Set() is then called
in order to wake the process thread from timer_event_->Wait(). When this
happens the process thread will return early and the run Process()
again. This time it will pick up created_at_.tv_sec = 0 and run a new
Wait() call with the desired end time.

However if the process thread was NOT blocking on timer_event_->Wait()
when timer_event_->Set() was called from StartTimer() it will mean that
the first call to timer_event_->Wait() from Process(), AFTER the new
time has been configured (count_ = 1), will return early.

If the timer is not periodic it means that Set() will never be called,
and any calls will Wait() will block until the time out.

The solution is to always reset the event in timer_event_ on the first
call to timerEvent_->Wait(), after a timer has started.

Also some general cleanup.

BUG=
==========

[sprang_webrtc, 2016-03-17 20:07:27]

The CQ bit was checked by sprang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-17 20:07:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812533002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812533002/40001

[commit-bot: I haz the power, 2016-03-17 21:11:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-17 21:11:24]

Dry run: Try jobs failed on following builders:
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/13598)

[mflodman, 2016-03-18 09:19:24]

lgtm

https://codereview.webrtc.org/1812533002/diff/60001/webrtc/system_wrappers/so...
File webrtc/system_wrappers/source/event_timer_posix.h (right):

https://codereview.webrtc.org/1812533002/diff/60001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/event_timer_posix.h:60: friend class
EventTimerPosixTest;
This is normally put directly on the line after 'private:'.

[sprang_webrtc, 2016-03-18 09:24:28]

https://codereview.webrtc.org/1812533002/diff/60001/webrtc/system_wrappers/so...
File webrtc/system_wrappers/source/event_timer_posix.h (right):

https://codereview.webrtc.org/1812533002/diff/60001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/event_timer_posix.h:60: friend class
EventTimerPosixTest;
On 2016/03/18 09:19:23, mflodman wrote:
> This is normally put directly on the line after 'private:'.

Done.

[sprang_webrtc, 2016-03-18 09:24:33]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-03-18 09:24:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1812533002/#ps80001 (title:
"Formatting")

[commit-bot: I haz the power, 2016-03-18 09:24:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812533002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812533002/80001

[commit-bot: I haz the power, 2016-03-18 10:46:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-18 10:46:23]

Try jobs failed on following builders:
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/13330)

[sprang_webrtc, 2016-03-21 11:03:32]

Fixed a few issues, you may want to take another pass...

[mflodman, 2016-03-22 07:47:02]

LGTM

https://codereview.webrtc.org/1812533002/diff/140001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/event_timer_posix.cc (right):

https://codereview.webrtc.org/1812533002/diff/140001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/event_timer_posix.cc:192:
pthread_mutex_unlock(&mutex_);
Ouch, I should have spotted this too...

[sprang_webrtc, 2016-03-22 08:46:40]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2016-03-22 08:46:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812533002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812533002/140001

[commit-bot: I haz the power, 2016-03-22 08:51:51]

Description was changed from

==========
Fix race condition in EventTimerPosix

The intended signalling from StartTimer() to Process() is that
created_at_.tv_sec is set to 0, and timer_event_->Set() is then called
in order to wake the process thread from timer_event_->Wait(). When this
happens the process thread will return early and the run Process()
again. This time it will pick up created_at_.tv_sec = 0 and run a new
Wait() call with the desired end time.

However if the process thread was NOT blocking on timer_event_->Wait()
when timer_event_->Set() was called from StartTimer() it will mean that
the first call to timer_event_->Wait() from Process(), AFTER the new
time has been configured (count_ = 1), will return early.

If the timer is not periodic it means that Set() will never be called,
and any calls will Wait() will block until the time out.

The solution is to always reset the event in timer_event_ on the first
call to timerEvent_->Wait(), after a timer has started.

Also some general cleanup.

BUG=
==========

to

==========
Fix race condition in EventTimerPosix

The intended signalling from StartTimer() to Process() is that
created_at_.tv_sec is set to 0, and timer_event_->Set() is then called
in order to wake the process thread from timer_event_->Wait(). When this
happens the process thread will return early and the run Process()
again. This time it will pick up created_at_.tv_sec = 0 and run a new
Wait() call with the desired end time.

However if the process thread was NOT blocking on timer_event_->Wait()
when timer_event_->Set() was called from StartTimer() it will mean that
the first call to timer_event_->Wait() from Process(), AFTER the new
time has been configured (count_ = 1), will return early.

If the timer is not periodic it means that Set() will never be called,
and any calls will Wait() will block until the time out.

The solution is to always reset the event in timer_event_ on the first
call to timerEvent_->Wait(), after a timer has started.

Also some general cleanup.

BUG=

Committed: https://crrev.com/53cf3463c0c31039a750d49acf4755ed03043277
Cr-Commit-Position: refs/heads/master@{#12082}
==========

[commit-bot: I haz the power, 2016-03-22 08:51:52]

Patchset 8 (id:??) landed as
https://crrev.com/53cf3463c0c31039a750d49acf4755ed03043277
Cr-Commit-Position: refs/heads/master@{#12082}

[commit-bot: I haz the power, 2016-03-22 08:51:54]

Description was changed from

==========
Fix race condition in EventTimerPosix

The intended signalling from StartTimer() to Process() is that
created_at_.tv_sec is set to 0, and timer_event_->Set() is then called
in order to wake the process thread from timer_event_->Wait(). When this
happens the process thread will return early and the run Process()
again. This time it will pick up created_at_.tv_sec = 0 and run a new
Wait() call with the desired end time.

However if the process thread was NOT blocking on timer_event_->Wait()
when timer_event_->Set() was called from StartTimer() it will mean that
the first call to timer_event_->Wait() from Process(), AFTER the new
time has been configured (count_ = 1), will return early.

If the timer is not periodic it means that Set() will never be called,
and any calls will Wait() will block until the time out.

The solution is to always reset the event in timer_event_ on the first
call to timerEvent_->Wait(), after a timer has started.

Also some general cleanup.

BUG=

Committed: https://crrev.com/53cf3463c0c31039a750d49acf4755ed03043277
Cr-Commit-Position: refs/heads/master@{#12082}
==========

to

==========
Fix race condition in EventTimerPosix

The intended signalling from StartTimer() to Process() is that
created_at_.tv_sec is set to 0, and timer_event_->Set() is then called
in order to wake the process thread from timer_event_->Wait(). When this
happens the process thread will return early and the run Process()
again. This time it will pick up created_at_.tv_sec = 0 and run a new
Wait() call with the desired end time.

However if the process thread was NOT blocking on timer_event_->Wait()
when timer_event_->Set() was called from StartTimer() it will mean that
the first call to timer_event_->Wait() from Process(), AFTER the new
time has been configured (count_ = 1), will return early.

If the timer is not periodic it means that Set() will never be called,
and any calls will Wait() will block until the time out.

The solution is to always reset the event in timer_event_ on the first
call to timerEvent_->Wait(), after a timer has started.

Also some general cleanup.

BUG=
==========

[commit-bot: I haz the power, 2016-03-22 08:51:55]

Committed patchset #8 (id:140001)