Fix a race in ThreadLocalStorage::StaticSlot::initialized which triggers a TSAN error.

Fix a race in ThreadLocalStorage::StaticSlot::initialized which triggers a TSAN error.

Also fix a data race with ThreadData::status_. TSan doesn't see this race explicitly, but it contributes to a race with the use of StaticSlot in ThreadData::tls_index_.

BUG=268941
TESTED=Ran base_unittests.
TSAN errors before: lots, oodles, slathers, acres
After: Zip, zilch, nadda

Committed: https://crrev.com/42d16882080508876676ef164c1f7a863ca1fbf3
Cr-Commit-Position: refs/heads/master@{#339219}

[Alexander Potapenko, 2015-07-02 08:22:41]

glider@chromium.org changed reviewers:
+ glider@chromium.org

[Alexander Potapenko, 2015-07-02 08:22:41]

Thanks for working on this!

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc#new...
base/tracked_objects.cc:368: CHECK(tls_index_.initialized());
This one is indeed unnecessary and should not make TSan happy (at least TSan
should not complain if you remove it). Have you checked it does?

[Anand Mistry (off Chromium), 2015-07-02 08:49:11]

I think removing the suppression triggered a different (but maybe related) tsan
error. I'll take a look at that one tomorrow and fix it before I submit this.

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc#new...
base/tracked_objects.cc:368: CHECK(tls_index_.initialized());
On 2015/07/02 08:22:41, Alexander Potapenko wrote:
> This one is indeed unnecessary and should not make TSan happy (at least TSan
> should not complain if you remove it). Have you checked it does?

Yes. TSan was unhappy before I added it. I suspect it's because access to the
TLS slot was being gated by |status_| instead of initialized() which contains an
appropriate memory barrier. Given this should only run once per thread at the
start, there's no performance issue here.

[Alexander Potapenko, 2015-07-02 09:24:17]

Yeah, I've just checked that, the error is a different one.

[Anand Mistry (off Chromium), 2015-07-03 01:34:49]

amistry@chromium.org changed reviewers:
+ thakis@chromium.org

[Anand Mistry (off Chromium), 2015-07-04 13:09:55]

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc#new...
base/tracked_objects.cc:735: status_ = kInitialStartupState;
I've been thinking about this, and I believe there's a problem with this line.
Specifically, I think this store needs to be done with release semantics.
Without that, it's possible for the write to be re-ordered before the actual
slot allocation. On another thread, the condition (status >= DEACTIVATED) at the
top of this function would succeed even though the slot hasn't been allocated or
isn't visible. Note, that tls_index_.Initialize() in this change has a release
store at the end, but that won't prevent the re-order I'm describing. I think
this could be solved by placing a MemoryBarrier() before this assignment. That
would ensure the slot creation "happens before" this assignment.

[Anand Mistry (off Chromium), 2015-07-13 00:41:43]

Ping! Comments?

[Nico, 2015-07-13 02:36:52]

(the ping is for glider, right?)

[Anand Mistry (off Chromium), 2015-07-13 03:01:58]

On 2015/07/13 02:36:52, Nico wrote:
> (the ping is for glider, right?)

Yes, for now... but feel free to take a look. I probably should have waited
before adding you as a reviewer. Sorry.

[Alexander Potapenko, 2015-07-13 09:26:17]

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc#new...
base/tracked_objects.cc:735: status_ = kInitialStartupState;
On 2015/07/04 13:09:55, Anand Mistry wrote:
> I've been thinking about this, and I believe there's a problem with this line.
> Specifically, I think this store needs to be done with release semantics.
> Without that, it's possible for the write to be re-ordered before the actual
> slot allocation. 
This sounds correct to me.
> On another thread, the condition (status >= DEACTIVATED) at the
> top of this function would succeed even though the slot hasn't been allocated
or
> isn't visible. Note, that tls_index_.Initialize() in this change has a release
> store at the end, but that won't prevent the re-order I'm describing. I think
> this could be solved by placing a MemoryBarrier() before this assignment. That
> would ensure the slot creation "happens before" this assignment.

I do not remember exactly what's up with these status_ accesses.
This issue has been risen up twice (http://crbug.com/21259 and
http://crbug.com/108539) but hasn't been resolved.
Maybe this was the case where adding atomics had bad impact on the performance
in the past, but much has changed since then, so I think it's worth trying to
fix it and check the performance again.

Please avoid using MemoryBarrier() calls, as TSan can't comprehend them.
Use Release_Store() instead and make sure other accesses to status_ are also
done in an atomic fashion.

[Anand Mistry (off Chromium), 2015-07-15 08:00:01]

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1222123002/diff/1/base/tracked_objects.cc#new...
base/tracked_objects.cc:735: status_ = kInitialStartupState;
On 2015/07/13 09:26:17, Alexander Potapenko wrote:
> On 2015/07/04 13:09:55, Anand Mistry wrote:
> > I've been thinking about this, and I believe there's a problem with this
line.
> > Specifically, I think this store needs to be done with release semantics.
> > Without that, it's possible for the write to be re-ordered before the actual
> > slot allocation. 
> This sounds correct to me.
> > On another thread, the condition (status >= DEACTIVATED) at the
> > top of this function would succeed even though the slot hasn't been
allocated
> or
> > isn't visible. Note, that tls_index_.Initialize() in this change has a
release
> > store at the end, but that won't prevent the re-order I'm describing. I
think
> > this could be solved by placing a MemoryBarrier() before this assignment.
That
> > would ensure the slot creation "happens before" this assignment.
> 
> I do not remember exactly what's up with these status_ accesses.
> This issue has been risen up twice (http://crbug.com/21259 and
> http://crbug.com/108539) but hasn't been resolved.
> Maybe this was the case where adding atomics had bad impact on the performance
> in the past, but much has changed since then, so I think it's worth trying to
> fix it and check the performance again.
> 
> Please avoid using MemoryBarrier() calls, as TSan can't comprehend them.
> Use Release_Store() instead and make sure other accesses to status_ are also
> done in an atomic fashion.

I believe I've fixed the race with status_. However, I haven't had a chance to
benchmark the result. I'm not sure the performance different is going to be
significant in real-life since the places that use ThreadData (i.e.
TrackingInfo) are then used with locks which already have a hit from the CAS in
Lock::Acquire. That said, I'll see if there's an appropriate perftest and run
the before-after numbers. I just haven't had time yet and I'd rather send this
out now to get comments.

[Anand Mistry (off Chromium), 2015-07-16 01:50:01]

I ran base_perftests and it looks like the results are too noisy to say whether
or not this makes any difference. I don't think microbenchmarks on
tracked_objects::ThreadData makes any sense since it's only used in combination
with a PendingTask.

[Alexander Potapenko, 2015-07-16 08:19:16]

On 2015/07/16 01:50:01, Anand Mistry wrote:
> I ran base_perftests and it looks like the results are too noisy to say
whether
> or not this makes any difference. I don't think microbenchmarks on
> tracked_objects::ThreadData makes any sense since it's only used in
combination
> with a PendingTask.
We can land it then and check if there are regressions on the perf bots.

LGTM

[Nico, 2015-07-16 20:49:54]

can you update the cl description to also include the status_ change? with that,
lgtm

[Anand Mistry (off Chromium), 2015-07-17 01:23:55]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-07-17 01:23:56]

The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1222123002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-17 01:25:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1222123002/40001

[eugenis, 2015-07-17 01:58:01]

On 2015/07/17 01:25:23, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1222123002/40001

Could you check if this other suppression is still needed?
"race:tracked_objects::ThreadData::InitializeThreadContext\n"

[commit-bot: I haz the power, 2015-07-17 02:01:47]

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

[commit-bot: I haz the power, 2015-07-17 02:01:48]

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

(exceeded global retry quota)

[Anand Mistry (off Chromium), 2015-07-17 02:28:26]

On 2015/07/17 01:58:01, eugenis wrote:
> On 2015/07/17 01:25:23, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1222123002/40001
> 
> Could you check if this other suppression is still needed?
> "race:tracked_objects::ThreadData::InitializeThreadContext\n"

I'm doing that in a separate change
(https://codereview.chromium.org/1243533003/).

[Anand Mistry (off Chromium), 2015-07-17 02:31:01]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-07-17 02:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1222123002/#ps60001
(title: "Fix MSVC compile warning.")

[commit-bot: I haz the power, 2015-07-17 02:31:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1222123002/60001

[commit-bot: I haz the power, 2015-07-17 03:58:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-17 03:59:05]

Patchset 4 (id:??) landed as
https://crrev.com/42d16882080508876676ef164c1f7a863ca1fbf3
Cr-Commit-Position: refs/heads/master@{#339219}