Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

disk_cache::TraceObject has non-thread-safe ref count. Its AddRef() is
called on a background thread in disk_cache::BackendImpl::SyncInit(),
and Release() is called on the IO thread on disk_cache::Backend destruction.
So, if a Backend instance is initialized while another Backend instance is
being destroyed, these ref count manipulations cause data race.

BUG=688072
Review-Url: https://codereview.chromium.org/2672983004
Cr-Commit-Position: refs/heads/master@{#450917}
Committed: https://chromium.googlesource.com/chromium/src/+/704e5153f057ce179dc44ff6b4f6a0d565d886e4

[tzik, 2017-02-03 19:11:13]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 19:11:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-02-03 19:16:58]

Description was changed from

==========
Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

BUG=688072
==========

to

==========
Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

disk_cache::TraceObject does not live in a single thread in its nature
and its owners seem to hold the TraceObject instance on several different
threads. So, RefCountedThreadSafe is the best fit instead of RefCounted.

BUG=688072
==========

[tzik, 2017-02-03 19:18:37]

tzik@chromium.org changed reviewers:
+ gavinp@chromium.org

[tzik, 2017-02-03 19:18:47]

PTAL

[commit-bot: I haz the power, 2017-02-03 20:20:52]

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

[commit-bot: I haz the power, 2017-02-03 20:20:53]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-02-09 22:58:29]

tzik@chromium.org changed reviewers:
+ jkarlin@chromium.org

[tzik, 2017-02-09 23:00:05]

jkarlin: Could you take a look?

[jkarlin, 2017-02-13 17:33:55]

lgtm but you'll need a net/ owner (e.g., gavinp) to sign off as well.

[tzik, 2017-02-14 09:06:48]

On 2017/02/13 17:33:55, jkarlin wrote:
> lgtm but you'll need a net/ owner (e.g., gavinp) to sign off as well.

[tzik, 2017-02-14 09:13:47]

tzik@chromium.org changed reviewers:
+ cbentzel@chromium.org

[tzik, 2017-02-14 09:13:48]

On 2017/02/13 17:33:55, jkarlin wrote:
> lgtm but you'll need a net/ owner (e.g., gavinp) to sign off as well.

Sure. But Gavin seems not around here at the moment?

cbentzel: Could you take a look as a //net OWNER?

[cbentzel, 2017-02-14 18:30:32]

On 2017/02/14 09:13:48, tzik wrote:
> On 2017/02/13 17:33:55, jkarlin wrote:
> > lgtm but you'll need a net/ owner (e.g., gavinp) to sign off as well.
> 
> Sure. But Gavin seems not around here at the moment?
> 
> cbentzel: Could you take a look as a //net OWNER?

The CL description made it unclear to me if AddRef/Release are actually being
called on different threads and this is a known bug, or if this is simply a
defensive move. Can you clarify?

[tzik, 2017-02-15 06:09:41]

Description was changed from

==========
Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

disk_cache::TraceObject does not live in a single thread in its nature
and its owners seem to hold the TraceObject instance on several different
threads. So, RefCountedThreadSafe is the best fit instead of RefCounted.

BUG=688072
==========

to

==========
Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

disk_cache::TraceObject has non-thread-safe ref count. Its AddRef() is
called on a background thread in disk_cache::BackendImpl::SyncInit(),
and Release() is called on the IO thread on disk_cache::Backend destruction.
So, if a Backend instance is initialized while another Backend instance is
being destroyed, these ref count manipulations cause data race.

BUG=688072
==========

[tzik, 2017-02-15 06:13:39]

On 2017/02/14 18:30:32, cbentzel wrote:
> On 2017/02/14 09:13:48, tzik wrote:
> > On 2017/02/13 17:33:55, jkarlin wrote:
> > > lgtm but you'll need a net/ owner (e.g., gavinp) to sign off as well.
> > 
> > Sure. But Gavin seems not around here at the moment?
> > 
> > cbentzel: Could you take a look as a //net OWNER?
> 
> The CL description made it unclear to me if AddRef/Release are actually being
> called on different threads and this is a known bug, or if this is simply a
> defensive move. Can you clarify?

Updated the description. Yes, AddRef/Release are actually being called on
different threads.
I don't know there's a reported bug for this, but I saw several crash around
this:
http://crash/030a941580000000, http://crash/2b447ef900000000

[cbentzel, 2017-02-15 18:18:54]

LGTM

It is unclear to me if we actually need this class. It looks like this is only
used when STRESS_CACHE #define flags are set which is mainly for testing. I am
not sure if we are using that test infrastructure. But I will create a separate
bug for that.

Also one of the crashes points to the Shader Cache destructor and there are
other bugs with concerns about the lifetime issues there.

[tzik, 2017-02-16 00:46:09]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-02-16 00:47:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 02:49:33]

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

[commit-bot: I haz the power, 2017-02-16 02:49:34]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[tzik, 2017-02-16 04:00:24]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-02-16 04:01:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 04:39:30]

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

[commit-bot: I haz the power, 2017-02-16 04:39:31]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[tzik, 2017-02-16 07:08:31]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-02-16 07:09:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 09:42:14]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487228911455010, "parent_rev":
"3f9be266524354849e17f6a123ee20dd6e7d53e0", "commit_rev":
"704e5153f057ce179dc44ff6b4f6a0d565d886e4"}

[commit-bot: I haz the power, 2017-02-16 09:43:08]

Description was changed from

==========
Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

disk_cache::TraceObject has non-thread-safe ref count. Its AddRef() is
called on a background thread in disk_cache::BackendImpl::SyncInit(),
and Release() is called on the IO thread on disk_cache::Backend destruction.
So, if a Backend instance is initialized while another Backend instance is
being destroyed, these ref count manipulations cause data race.

BUG=688072
==========

to

==========
Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

disk_cache::TraceObject has non-thread-safe ref count. Its AddRef() is
called on a background thread in disk_cache::BackendImpl::SyncInit(),
and Release() is called on the IO thread on disk_cache::Backend destruction.
So, if a Backend instance is initialized while another Backend instance is
being destroyed, these ref count manipulations cause data race.

BUG=688072

Review-Url: https://codereview.chromium.org/2672983004
Cr-Commit-Position: refs/heads/master@{#450917}
Committed:
https://chromium.googlesource.com/chromium/src/+/704e5153f057ce179dc44ff6b4f6...
==========

[commit-bot: I haz the power, 2017-02-16 09:43:09]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/704e5153f057ce179dc44ff6b4f6...