Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(568)

Issue 1872713002: Remove unnecessary lock that causes reentry. (Closed)

Created:
4 years, 8 months ago by bcwhite
Modified:
4 years, 8 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove unnecessary lock that causes reentry. The StatisticsRecorder is acquiring a lock before calling Import but the import calls back to the SR which again tries to acquire the lock... and crashes. The lock isn't necessary with the new persistent iterator (which is lock-free) so move the Import call outside of the locking. BUG=601457 Committed: https://crrev.com/2f153f83f1d6fcfb4b0b8dae5a6b12dbc6544d6b Cr-Commit-Position: refs/heads/master@{#386102}

Patch Set 1 #

Total comments: 2

Patch Set 2 : added comment as to why Import is done before lock #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -5 lines) Patch
M base/metrics/persistent_histogram_allocator.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M base/metrics/statistics_recorder.cc View 1 1 chunk +7 lines, -3 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 17 (8 generated)
bcwhite
4 years, 8 months ago (2016-04-08 06:14:34 UTC) #2
bcwhite
On 2016/04/08 06:14:34, bcwhite wrote: I tried to write a test for this but cannot ...
4 years, 8 months ago (2016-04-08 13:24:27 UTC) #3
Alexei Svitkine (slow)
lgtm I agree that this should have test, please work on that after landing this. ...
4 years, 8 months ago (2016-04-08 14:51:49 UTC) #5
bcwhite
Already started the test based on the order-fix CL. https://codereview.chromium.org/1872713002/diff/1/base/metrics/statistics_recorder.cc File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1872713002/diff/1/base/metrics/statistics_recorder.cc#newcode297 base/metrics/statistics_recorder.cc:297: ...
4 years, 8 months ago (2016-04-08 15:06:08 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1872713002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1872713002/20001
4 years, 8 months ago (2016-04-08 15:06:48 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/193823)
4 years, 8 months ago (2016-04-08 16:15:33 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1872713002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1872713002/20001
4 years, 8 months ago (2016-04-08 16:52:08 UTC) #13
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 8 months ago (2016-04-08 16:58:11 UTC) #15
commit-bot: I haz the power
4 years, 8 months ago (2016-04-08 16:59:43 UTC) #17
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/2f153f83f1d6fcfb4b0b8dae5a6b12dbc6544d6b
Cr-Commit-Position: refs/heads/master@{#386102}

Powered by Google App Engine
This is Rietveld 408576698