base/threading: fix a potential race issue on start_event_

base/threading: fix a potential race issue on start_event_

WaitableEvent is thread-safe, but scoped_ptr<WaitableEvent> is not.
It should need a mutex lock to access the internal pointer if the
pointer may be set or reset after the thread creation that will share
the scoped_ptr.
Avoid the issue, use WaitableEvent instead of scoped_ptr<WaitableEvent>.

Committed: https://crrev.com/53818d55bbb632e703158aa831a03794bffd23f2
Cr-Commit-Position: refs/heads/master@{#342082}

[Takashi Toyoshima, 2015-07-31 09:25:55]

toyoshim@chromium.org changed reviewers:
+ thestig@chromium.org

[Takashi Toyoshima, 2015-07-31 09:25:56]

Lei:
I achieve the initial goal to remove a jank of thread creation on pthreads. But
I still have two more cleanups. One is this potential race issue that I noticed
while working around the Thread class. And the other is to remove the
ScopedTrakcer from GetThreadId() once I get enough data.
Can you take a look this former one?

https://codereview.chromium.org/1256903005/diff/40001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1256903005/diff/40001/base/threading/thread.c...
base/threading/thread.cc:148: if
(thread_.is_equal(base::PlatformThreadHandle()))
This change is needed to ignore the second call from the dtor.

[Lei Zhang, 2015-07-31 20:52:50]

Do you think it's possible to write a test for the existing code that causes a
race with |start_event_| so that it shows up with TSAN v2? Then apply this CL
and see the race is gone.

https://codereview.chromium.org/1256903005/diff/40001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1256903005/diff/40001/base/threading/thread.c...
base/threading/thread.cc:148: if
(thread_.is_equal(base::PlatformThreadHandle()))
Just check thread_.is_null() ?

[Takashi Toyoshima, 2015-08-05 04:48:42]

Hum... this is just a potential issue, and current code could not hit the case
if we call each method within the expected usage.
So this patch is just a kind of clean up to protect from future code changes
that will cause a race unconsciously.
I'm fine to abandon this change for now since this is not a real problem.

[Lei Zhang, 2015-08-05 20:06:10]

On 2015/08/05 04:48:42, Takashi Toyoshima (chromium) wrote:
> Hum... this is just a potential issue, and current code could not hit the case
> if we call each method within the expected usage.
> So this patch is just a kind of clean up to protect from future code changes
> that will cause a race unconsciously.
> I'm fine to abandon this change for now since this is not a real problem.

Since you wrote this CL already, let's go ahead and land it. I have a comment in
thread.cc for patch set 3.

[Takashi Toyoshima, 2015-08-06 05:20:52]

Thanks. I uploaded new patch set 4 that addressed the point, and patch set 5 is
just rebased.

https://codereview.chromium.org/1256903005/diff/40001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1256903005/diff/40001/base/threading/thread.c...
base/threading/thread.cc:148: if
(thread_.is_equal(base::PlatformThreadHandle()))
On 2015/07/31 20:52:50, Lei Zhang wrote:
> Just check thread_.is_null() ?

Done.

[Lei Zhang, 2015-08-06 06:10:42]

lgtm

[Takashi Toyoshima, 2015-08-06 06:18:37]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-08-06 06:18:43]

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

[commit-bot: I haz the power, 2015-08-06 07:30:23]

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

[commit-bot: I haz the power, 2015-08-06 07:30:24]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Takashi Toyoshima, 2015-08-06 08:03:09]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-08-06 08:03:30]

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

[commit-bot: I haz the power, 2015-08-06 09:03:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-06 09:03:52]

Patchset 5 (id:??) landed as
https://crrev.com/53818d55bbb632e703158aa831a03794bffd23f2
Cr-Commit-Position: refs/heads/master@{#342082}