Run SR tests both with and without persistent allocator.

Run SR tests both with and without persistent allocator.

Fix bug with iterator returning a persistent histogram
when it should not have.

Test for re-entry of lock when importing persistent
histograms (https://codereview.chromium.org/1779503002/).

BUG=546019
Committed: https://crrev.com/99731cf14e3c3da57c3322c4e9eb0d527369da4e
Cr-Commit-Position: refs/heads/master@{#387002}

[bcwhite, 2016-04-11 12:43:02]

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

[Alexei Svitkine (slow), 2016-04-11 14:51:34]

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:42: if (!include_persistent_ && iter_ !=
histograms_->end() &&
Shouldn't this be a loop? What if the second entry is also persistent? (If so,
also add a test.)

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder_unittest.cc (right):

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder_unittest.cc:36: if
(use_persistent_histogram_allocator_)
Nit: {}

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder_unittest.cc:72: int
CountIterableHistograms(StatisticsRecorder::HistogramIterator& iter) {
Nit: Non-const ref as function params are discouraged.

You can just pass by ptr and deref in the body.

[bcwhite, 2016-04-12 00:32:03]

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:42: if (!include_persistent_ && iter_ !=
histograms_->end() &&
On 2016/04/11 14:51:34, Alexei Svitkine wrote:
> Shouldn't this be a loop? What if the second entry is also persistent? (If so,
> also add a test.)

operator++ already includes a loop that skips as appropriate.

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder_unittest.cc (right):

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder_unittest.cc:36: if
(use_persistent_histogram_allocator_)
On 2016/04/11 14:51:34, Alexei Svitkine wrote:
> Nit: {}

Done.

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder_unittest.cc:72: int
CountIterableHistograms(StatisticsRecorder::HistogramIterator& iter) {
On 2016/04/11 14:51:34, Alexei Svitkine wrote:
> Nit: Non-const ref as function params are discouraged.
> 
> You can just pass by ptr and deref in the body.

Done.

[Alexei Svitkine (slow), 2016-04-12 21:02:08]

lgtm

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:42: if (!include_persistent_ && iter_ !=
histograms_->end() &&
On 2016/04/12 00:32:02, bcwhite wrote:
> On 2016/04/11 14:51:34, Alexei Svitkine wrote:
> > Shouldn't this be a loop? What if the second entry is also persistent? (If
so,
> > also add a test.)
> 
> operator++ already includes a loop that skips as appropriate.

Makes sense. Maybe add a comment so it's clear when reading this code.

[bcwhite, 2016-04-13 12:59:45]

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/1876053002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:42: if (!include_persistent_ && iter_ !=
histograms_->end() &&
On 2016/04/12 21:02:08, Alexei Svitkine wrote:
> On 2016/04/12 00:32:02, bcwhite wrote:
> > On 2016/04/11 14:51:34, Alexei Svitkine wrote:
> > > Shouldn't this be a loop? What if the second entry is also persistent? (If
> so,
> > > also add a test.)
> > 
> > operator++ already includes a loop that skips as appropriate.
> 
> Makes sense. Maybe add a comment so it's clear when reading this code.

Done.

[bcwhite, 2016-04-13 12:59:53]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-04-13 12:59:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1876053002/#ps60001
(title: "added comment about operation of operator++")

[commit-bot: I haz the power, 2016-04-13 13:00:09]

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

[commit-bot: I haz the power, 2016-04-13 13:57:52]

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

[commit-bot: I haz the power, 2016-04-13 13:57:53]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2016-04-13 16:04:20]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-04-13 16:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1876053002/#ps80001
(title: "fix use-after-free in test")

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

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

[commit-bot: I haz the power, 2016-04-13 16:10:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-13 16:11:34]

Description was changed from

==========
Run SR tests both with and without persistent allocator.

Fix bug with iterator returning a persistent histogram
when it should not have.

Test for re-entry of lock when importing persistent
histograms (https://codereview.chromium.org/1779503002/).

BUG=546019
==========

to

==========
Run SR tests both with and without persistent allocator.

Fix bug with iterator returning a persistent histogram
when it should not have.

Test for re-entry of lock when importing persistent
histograms (https://codereview.chromium.org/1779503002/).

BUG=546019

Committed: https://crrev.com/99731cf14e3c3da57c3322c4e9eb0d527369da4e
Cr-Commit-Position: refs/heads/master@{#387002}
==========

[commit-bot: I haz the power, 2016-04-13 16:11:34]

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