Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517193003:
Fix adding duplicate trials to allocator (finally) (Closed)
|
Created:
4 years, 1 month ago by lawrencewu Modified:
4 years ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix adding duplicate trials to allocator (finally)
It looks like the reason we were crashing from shared memory was because
simulated field trials were getting added to the allocator when they
shouldn't be. This CL now checks |trial_registered_| before adding the
the trial to the allocator. Also includes a unittest for this behavior.
BUG=666230
Committed: https://crrev.com/46df0efa72f7bb53ae173831d06bc9127ce34091
Cr-Commit-Position: refs/heads/master@{#434046}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address nit #
Total comments: 1
Patch Set 4 : fix unittest memory leak #Patch Set 5 : git rebase-update #Patch Set 6 : git rebase-update #Patch Set 7 : git rebase-update #
Messages
Total messages: 63 (34 generated)
Description was changed from ========== Fix adding duplicate trials to allocator (finally) BUG=666230 ========== to ========== Fix adding duplicate trials to allocator (finally) It looks like the reason we were crashing from shared memory was because simulated field trials were getting added to the allocator when they shouldn't be. This CL now checks |trial_registered_| before adding the the trial to the allocator. Also includes a unittest for this behavior. BUG=666230 ==========
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org
Description was changed from ========== Fix adding duplicate trials to allocator (finally) It looks like the reason we were crashing from shared memory was because simulated field trials were getting added to the allocator when they shouldn't be. This CL now checks |trial_registered_| before adding the the trial to the allocator. Also includes a unittest for this behavior. BUG=666230 ========== to ========== Fix adding duplicate trials to allocator (finally) It looks like the reason we were crashing from shared memory was because simulated field trials were getting added to the allocator when they shouldn't be. This CL now checks |trial_registered_| before adding the the trial to the allocator. Also includes a unittest for this behavior. BUG=666230 ==========
initial CL
lgtm % comments Assuming test crashes without the fix. https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... base/metrics/field_trial_unittest.cc:1190: shm.get()->Map(64 << 10); // Hardcoded, equal to kFieldTrialAllocationSize. This seems suboptimal. I think you shouldn't make them equal and just use some size that's big enough with a comment mentioning it should be big enough to hold the trials created in the test. This way, it doesn't have to be updated when the other constant changes. (The other option would be to actually use the same constant and expose it through an internal namespace - but that doesn't seem needed in this case.) https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... base/metrics/field_trial_unittest.cc:1227: ASSERT_TRUE(check_string.find("Simulated") == std::string::npos); EXPECT_NE?
"Assuming test crashes without the fix." Verified that it does crash without this fix. On 2016/11/21 18:28:45, Alexei Svitkine (slow) wrote: > lgtm % comments > > Assuming test crashes without the fix. > > https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... > File base/metrics/field_trial_unittest.cc (right): > > https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... > base/metrics/field_trial_unittest.cc:1190: shm.get()->Map(64 << 10); // > Hardcoded, equal to kFieldTrialAllocationSize. > This seems suboptimal. > > I think you shouldn't make them equal and just use some size that's big enough > with a comment mentioning it should be big enough to hold the trials created in > the test. This way, it doesn't have to be updated when the other constant > changes. > > (The other option would be to actually use the same constant and expose it > through an internal namespace - but that doesn't seem needed in this case.) > > https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... > base/metrics/field_trial_unittest.cc:1227: > ASSERT_TRUE(check_string.find("Simulated") == std::string::npos); > EXPECT_NE?
Address comments. https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... base/metrics/field_trial_unittest.cc:1190: shm.get()->Map(64 << 10); // Hardcoded, equal to kFieldTrialAllocationSize. On 2016/11/21 18:28:45, Alexei Svitkine (slow) wrote: > This seems suboptimal. > > I think you shouldn't make them equal and just use some size that's big enough > with a comment mentioning it should be big enough to hold the trials created in > the test. This way, it doesn't have to be updated when the other constant > changes. > > (The other option would be to actually use the same constant and expose it > through an internal namespace - but that doesn't seem needed in this case.) Done. https://codereview.chromium.org/2517193003/diff/1/base/metrics/field_trial_un... base/metrics/field_trial_unittest.cc:1227: ASSERT_TRUE(check_string.find("Simulated") == std::string::npos); On 2016/11/21 18:28:45, Alexei Svitkine (slow) wrote: > EXPECT_NE? Done.
The CQ bit was checked by lawrencewu@chromium.org
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/2517193003/#ps20001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2517193003/diff/20001/base/metrics/field_tria... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2517193003/diff/20001/base/metrics/field_tria... base/metrics/field_trial_unittest.cc:1191: 4 << 10); // This is enough to hold the trials created for this test. Nit: If you have to wrap because of end of line comment, instead put the comment above the line. Also, mention what 4 << 10 is in terms of size to make it clearer for readers. Same for the test below.
The CQ bit was unchecked by lawrencewu@chromium.org
Fix nit https://codereview.chromium.org/2517193003/diff/20001/base/metrics/field_tria... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2517193003/diff/20001/base/metrics/field_tria... base/metrics/field_trial_unittest.cc:1191: 4 << 10); // This is enough to hold the trials created for this test. On 2016/11/21 19:13:58, Alexei Svitkine (slow) wrote: > Nit: If you have to wrap because of end of line comment, instead put the comment > above the line. > > Also, mention what 4 << 10 is in terms of size to make it clearer for readers. > > Same for the test below. Done.
The CQ bit was checked by lawrencewu@chromium.org
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/2517193003/#ps40001 (title: "address nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:41: const bool kUseSharedMemoryForFieldTrials = false; This probably conflict with your other CL that flipped this?
I don't think so -- the other CL turned it off, and this keeps it off. On 2016/11/21 19:20:10, Alexei Svitkine (slow) wrote: > lgtm > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > File base/metrics/field_trial.cc (right): > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > base/metrics/field_trial.cc:41: const bool kUseSharedMemoryForFieldTrials = > false; > This probably conflict with your other CL that flipped this?
On 2016/11/21 19:31:19, lawrencewu wrote: > I don't think so -- the other CL turned it off, and this keeps it off. > > On 2016/11/21 19:20:10, Alexei Svitkine (slow) wrote: > > lgtm > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > File base/metrics/field_trial.cc (right): > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > base/metrics/field_trial.cc:41: const bool kUseSharedMemoryForFieldTrials = > > false; > > This probably conflict with your other CL that flipped this? My point is that it's still in the diff on this CL - so you'll get a patch error. I think you need to sync and rebase and then have a patchset that doesn't have it in the diff.
The CQ bit was unchecked by lawrencewu@chromium.org
On 2016/11/21 19:45:04, Alexei Svitkine (slow) wrote: > On 2016/11/21 19:31:19, lawrencewu wrote: > > I don't think so -- the other CL turned it off, and this keeps it off. > > > > On 2016/11/21 19:20:10, Alexei Svitkine (slow) wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > > File base/metrics/field_trial.cc (right): > > > > > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > > base/metrics/field_trial.cc:41: const bool kUseSharedMemoryForFieldTrials = > > > false; > > > This probably conflict with your other CL that flipped this? > > My point is that it's still in the diff on this CL - so you'll get a patch > error. I think you need to sync and rebase and then have a patchset that doesn't > have it in the diff. Actually, nm - I looked at the diff and it's not there - I think I what happened is I looked at the diff between two of your patches and you rebased in the meantime - so it was showing both what this CL was changing and what changed in the rebase.
Oh, I see what you mean now. Will fix. On 2016/11/21 19:45:04, Alexei Svitkine (slow) wrote: > On 2016/11/21 19:31:19, lawrencewu wrote: > > I don't think so -- the other CL turned it off, and this keeps it off. > > > > On 2016/11/21 19:20:10, Alexei Svitkine (slow) wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > > File base/metrics/field_trial.cc (right): > > > > > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > > base/metrics/field_trial.cc:41: const bool kUseSharedMemoryForFieldTrials = > > > false; > > > This probably conflict with your other CL that flipped this? > > My point is that it's still in the diff on this CL - so you'll get a patch > error. I think you need to sync and rebase and then have a patchset that doesn't > have it in the diff.
The CQ bit was checked by lawrencewu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Okay yeah, I thought I had rebased recently. Never mind then! On 2016/11/21 19:46:31, Alexei Svitkine (slow) wrote: > On 2016/11/21 19:45:04, Alexei Svitkine (slow) wrote: > > On 2016/11/21 19:31:19, lawrencewu wrote: > > > I don't think so -- the other CL turned it off, and this keeps it off. > > > > > > On 2016/11/21 19:20:10, Alexei Svitkine (slow) wrote: > > > > lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > > > File base/metrics/field_trial.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2517193003/diff/40001/base/metrics/field_tria... > > > > base/metrics/field_trial.cc:41: const bool kUseSharedMemoryForFieldTrials > = > > > > false; > > > > This probably conflict with your other CL that flipped this? > > > > My point is that it's still in the diff on this CL - so you'll get a patch > > error. I think you need to sync and rebase and then have a patchset that > doesn't > > have it in the diff. > > Actually, nm - I looked at the diff and it's not there - I think I what happened > is I looked at the diff between two of your patches and you rebased in the > meantime - so it was showing both what this CL was changing and what changed in > the rebase.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lawrencewu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lawrencewu@chromium.org
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/2517193003/#ps80001 (title: "git rebase-update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for base/metrics/field_trial.h:
While running git apply --index -p1;
error: patch failed: base/metrics/field_trial.h:218
error: base/metrics/field_trial.h: patch does not apply
Patch: base/metrics/field_trial.h
Index: base/metrics/field_trial.h
diff --git a/base/metrics/field_trial.h b/base/metrics/field_trial.h
index
306f0075c6657d04c6e23f95a41e8dce455e37a3..2e5ce5b72bbf15f0fccb3d47c9ee7e438e714161
100644
--- a/base/metrics/field_trial.h
+++ b/base/metrics/field_trial.h
@@ -218,6 +218,8 @@ class BASE_EXPORT FieldTrial : public RefCounted<FieldTrial>
{
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedChangeDefault_NonDefault);
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, FloatBoundariesGiveEqualGroupSizes);
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, DoesNotSurpassTotalProbability);
+ FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest,
+ DoNotAddSimulatedFieldTrialsToAllocator);
typedef SharedPersistentMemoryAllocator::Reference FieldTrialRef;
@@ -543,6 +545,8 @@ class BASE_EXPORT FieldTrialList {
// Allow tests to access our innards for testing purposes.
FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest, InstantiateAllocator);
FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest, AddTrialsToAllocator);
+ FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest,
+ DoNotAddSimulatedFieldTrialsToAllocator);
#if defined(OS_WIN)
// Takes in |handle| that should have been retrieved from the command line
and
The CQ bit was checked by lawrencewu@chromium.org
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/2517193003/#ps100001 (title: "git rebase-update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by lawrencewu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by lawrencewu@chromium.org
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/2517193003/#ps120001 (title: "git rebase-update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479852695091100,
"parent_rev": "ee24984070aad5e35680512f514a76e1616d08a0", "commit_rev":
"d02a345524675311c68066ff51776a16d8bc4640"}
Message was sent while issue was closed.
Description was changed from ========== Fix adding duplicate trials to allocator (finally) It looks like the reason we were crashing from shared memory was because simulated field trials were getting added to the allocator when they shouldn't be. This CL now checks |trial_registered_| before adding the the trial to the allocator. Also includes a unittest for this behavior. BUG=666230 ========== to ========== Fix adding duplicate trials to allocator (finally) It looks like the reason we were crashing from shared memory was because simulated field trials were getting added to the allocator when they shouldn't be. This CL now checks |trial_registered_| before adding the the trial to the allocator. Also includes a unittest for this behavior. BUG=666230 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix adding duplicate trials to allocator (finally) It looks like the reason we were crashing from shared memory was because simulated field trials were getting added to the allocator when they shouldn't be. This CL now checks |trial_registered_| before adding the the trial to the allocator. Also includes a unittest for this behavior. BUG=666230 ========== to ========== Fix adding duplicate trials to allocator (finally) It looks like the reason we were crashing from shared memory was because simulated field trials were getting added to the allocator when they shouldn't be. This CL now checks |trial_registered_| before adding the the trial to the allocator. Also includes a unittest for this behavior. BUG=666230 Committed: https://crrev.com/46df0efa72f7bb53ae173831d06bc9127ce34091 Cr-Commit-Position: refs/heads/master@{#434046} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/46df0efa72f7bb53ae173831d06bc9127ce34091 Cr-Commit-Position: refs/heads/master@{#434046}
Message was sent while issue was closed.
On 2016/11/23 00:58:50, commit-bot: I haz the power wrote: > Patchset 7 (id:??) landed as > https://crrev.com/46df0efa72f7bb53ae173831d06bc9127ce34091 > Cr-Commit-Position: refs/heads/master@{#434046} Well that took a really long time to land. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
