[Cronet] Make HistogramManager public APIs thread-safe.

[Cronet] Make HistogramManager public APIs thread-safe.

CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently. The current public APIs
are GetInstance() which simply accesses a thread-safe
LazyInstance, and GetDeltas() which doesn't look thread-safe
so I've added a Lock.

BUG=603028
Committed: https://crrev.com/487de2df8db4a96fd24e6998590ae715142447ec
Cr-Commit-Position: refs/heads/master@{#387009}

[pauljensen, 2016-04-13 13:42:21]

Description was changed from

==========
[Cronet] Make HistogramManager public APIs thread-safe.

CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently.

BUG=603028
==========

to

==========
[Cronet] Make HistogramManager public APIs thread-safe.

CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently. The current public APIs
are GetInstance() which simply accesses a thread-safe
LazyInstance, and GetDeltas() which doesn't look thread-safe
so I've added a Lock.

BUG=603028
==========

[pauljensen, 2016-04-13 13:42:21]

pauljensen@chromium.org changed reviewers:
+ rtenneti@chromium.org

[pauljensen, 2016-04-13 13:42:32]

Raman, PTAL.

[bcwhite, 2016-04-13 14:12:51]

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

[bcwhite, 2016-04-13 14:12:52]

https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
File components/cronet/histogram_manager.cc (right):

https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
components/cronet/histogram_manager.cc:56: base::AutoLock lock(lock_);
It doesn't make sense to make multiple, parallel calls to PrepareDeltas while
passing the begin/end iterators from the StatisticsRecorder.  The second thread
through the method will find an empty (or very small) delta.

I suggest explicit Try/Release of the lock and return immediately if the Try
fails.  And rename the lock to indicate that it applies only to the GetDeltas
method.

[pauljensen, 2016-04-13 14:29:45]

https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
File components/cronet/histogram_manager.cc (right):

https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
components/cronet/histogram_manager.cc:56: base::AutoLock lock(lock_);
On 2016/04/13 14:12:52, bcwhite wrote:
> It doesn't make sense to make multiple, parallel calls to PrepareDeltas while
> passing the begin/end iterators from the StatisticsRecorder.

It may not make sense to do this, but we shouldn't crash if Cronet users use
this API in a supported way.

> The second thread through the method will find an empty (or very small) delta.

We document this.  See the documentation:
 
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
Again, it may not make sense, but we shouldn't crash if Cronet users use this
API in a supported way.

> I suggest explicit Try/Release of the lock and return immediately if the Try
> fails.

Good idea, done.

> And rename the lock to indicate that it applies only to the GetDeltas
> method.

I'd rather we keep it with a broader usage connotation so that future changes
can use it...rather than fall into the prior inadvertently thread-unsafe
mechanisms.

[bcwhite, 2016-04-13 14:42:18]

https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
File components/cronet/histogram_manager.cc (right):

https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
components/cronet/histogram_manager.cc:56: base::AutoLock lock(lock_);
> > I suggest explicit Try/Release of the lock and return immediately if the Try
> > fails.
> 
> Good idea, done.
> 
> > And rename the lock to indicate that it applies only to the GetDeltas
> > method.
> 
> I'd rather we keep it with a broader usage connotation so that future changes
> can use it...rather than fall into the prior inadvertently thread-unsafe
> mechanisms.

If this method were to be called while some "future" method owned the lock, the
Try() would fail (and return) and filling of the protobuf might never happen.

[pauljensen, 2016-04-13 14:59:14]

On 2016/04/13 14:42:18, bcwhite wrote:
>
https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
> File components/cronet/histogram_manager.cc (right):
> 
>
https://codereview.chromium.org/1882563004/diff/1/components/cronet/histogram...
> components/cronet/histogram_manager.cc:56: base::AutoLock lock(lock_);
> > > I suggest explicit Try/Release of the lock and return immediately if the
Try
> > > fails.
> > 
> > Good idea, done.
> > 
> > > And rename the lock to indicate that it applies only to the GetDeltas
> > > method.
> > 
> > I'd rather we keep it with a broader usage connotation so that future
changes
> > can use it...rather than fall into the prior inadvertently thread-unsafe
> > mechanisms.
> 
> If this method were to be called while some "future" method owned the lock,
the
> Try() would fail (and return) and filling of the protobuf might never happen.

I'm kind of willing to accept inefficient behavior over possibly introducing
bugs.  But I don't feel strongly on the matter and I don't have a good future
vision of what APIs we'd add to this class, so I've made the lock specific to
GetDeltas(). Done.

[ramant (doing other things), 2016-04-13 16:19:20]

Thanks for doing this. LGTM.

[pauljensen, 2016-04-13 16:27:20]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2016-04-13 16:27:29]

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

[commit-bot: I haz the power, 2016-04-13 17:05:45]

Description was changed from

==========
[Cronet] Make HistogramManager public APIs thread-safe.

CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently. The current public APIs
are GetInstance() which simply accesses a thread-safe
LazyInstance, and GetDeltas() which doesn't look thread-safe
so I've added a Lock.

BUG=603028
==========

to

==========
[Cronet] Make HistogramManager public APIs thread-safe.

CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently. The current public APIs
are GetInstance() which simply accesses a thread-safe
LazyInstance, and GetDeltas() which doesn't look thread-safe
so I've added a Lock.

BUG=603028
==========

[commit-bot: I haz the power, 2016-04-13 17:05:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-13 17:08:06]

Description was changed from

==========
[Cronet] Make HistogramManager public APIs thread-safe.

CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently. The current public APIs
are GetInstance() which simply accesses a thread-safe
LazyInstance, and GetDeltas() which doesn't look thread-safe
so I've added a Lock.

BUG=603028
==========

to

==========
[Cronet] Make HistogramManager public APIs thread-safe.

CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently. The current public APIs
are GetInstance() which simply accesses a thread-safe
LazyInstance, and GetDeltas() which doesn't look thread-safe
so I've added a Lock.

BUG=603028

Committed: https://crrev.com/487de2df8db4a96fd24e6998590ae715142447ec
Cr-Commit-Position: refs/heads/master@{#387009}
==========

[commit-bot: I haz the power, 2016-04-13 17:08:07]

Patchset 3 (id:??) landed as
https://crrev.com/487de2df8db4a96fd24e6998590ae715142447ec
Cr-Commit-Position: refs/heads/master@{#387009}