Remove debug instrumentation for crbug.com/359406.
There are no more signs of the crash. This CL reverts the code
to its original state to verify that the crash doesn't re-appear.
If it doesn't re-appear, we can conclude that the issue is fixed,
although it still bothers me that we don't know why.
BUG=359406
Review-Url: https://codereview.chromium.org/2024683002
Cr-Commit-Position: refs/heads/master@{#396753}
CrOS-Libchrome-Original-Commit: a6f3f2dca40126f21bd937e3106eef97aba10fde
diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index 3b398cd..430cae0 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -7,7 +7,6 @@
#include <algorithm>
#include "base/build_time.h"
-#include "base/lazy_instance.h"
#include "base/logging.h"
#include "base/rand_util.h"
#include "base/strings/string_number_conversions.h"
@@ -107,38 +106,6 @@
return true;
}
-void CheckTrialGroup(const std::string& trial_name,
- const std::string& trial_group,
- std::map<std::string, std::string>* seen_states) {
- if (ContainsKey(*seen_states, trial_name)) {
- CHECK_EQ((*seen_states)[trial_name], trial_group) << trial_name;
- } else {
- (*seen_states)[trial_name] = trial_group;
- }
-}
-
-// A second copy of FieldTrialList::seen_states_ that is meant to outlive the
-// FieldTrialList object to determine if the inconsistency happens because there
-// might be multiple FieldTrialList objects.
-// TODO(asvitkine): Remove when crbug.com/359406 is resolved.
-base::LazyInstance<std::map<std::string, std::string>>::Leaky g_seen_states =
- LAZY_INSTANCE_INITIALIZER;
-
-// A debug token generated during FieldTrialList construction. Used to diagnose
-// crbug.com/359406.
-// TODO(asvitkine): Remove when crbug.com/359406 is resolved.
-int32_t g_debug_token = -1;
-
-// Whether to append the debug token to the child process --force-fieldtrials
-// command line. Used to diagnose crbug.com/359406.
-// TODO(asvitkine): Remove when crbug.com/359406 is resolved.
-bool g_append_debug_token_to_trial_string = false;
-
-// Tracks whether |g_seen_states| is used. Defaults to false, because unit tests
-// will create multiple FieldTrialList instances. Also controls whether
-// |g_debug_token| is included in the field trial state string.
-bool g_use_global_check_states = false;
-
} // namespace
// statics
@@ -242,9 +209,7 @@
// static
void FieldTrial::EnableBenchmarking() {
- // TODO(asvitkine): Change this back to 0u after the trial in FieldTrialList
- // constructor is removed.
- DCHECK_EQ(1u, FieldTrialList::GetFieldTrialCount());
+ DCHECK_EQ(0u, FieldTrialList::GetFieldTrialCount());
enable_benchmarking_ = true;
}
@@ -276,9 +241,6 @@
DCHECK_GT(total_probability, 0);
DCHECK(!trial_name_.empty());
DCHECK(!default_group_name_.empty());
-
- if (g_debug_token == -1)
- g_debug_token = RandInt(1, INT32_MAX);
}
FieldTrial::~FieldTrial() {}
@@ -344,8 +306,7 @@
: entropy_provider_(entropy_provider),
observer_list_(new ObserverListThreadSafe<FieldTrialList::Observer>(
ObserverListBase<FieldTrialList::Observer>::NOTIFY_EXISTING_ONLY)) {
- // TODO(asvitkine): Turn into a DCHECK after http://crbug.com/359406 is fixed.
- CHECK(!global_);
+ DCHECK(!global_);
DCHECK(!used_without_global_);
global_ = this;
@@ -353,30 +314,6 @@
Time::Exploded exploded;
two_years_from_build_time.LocalExplode(&exploded);
kNoExpirationYear = exploded.year;
-
- // Run a 50/50 experiment that enables |g_use_global_check_states| only for
- // half the users, to investigate if this instrumentation is causing the
- // crashes to disappear for http://crbug.com/359406. Done here instead of a
- // server-side trial because this needs to be done early during FieldTrialList
- // initialization.
- //
- // Note: |g_use_global_check_states| is set via EnableGlobalStateChecks()
- // prior to the FieldTrialList being created. We only want to do the trial
- // check once the first time FieldTrialList is created, so use a static
- // |first_time| variable to track this.
- //
- // TODO(asvitkine): Remove after http://crbug.com/359406 is fixed.
- static bool first_time = true;
- if (first_time && g_use_global_check_states) {
- first_time = false;
- base::FieldTrial* trial =
- FactoryGetFieldTrial("UMA_CheckStates", 100, "NoChecks",
- kNoExpirationYear, 1, 1,
- FieldTrial::SESSION_RANDOMIZED, nullptr);
- trial->AppendGroup("Checks", 50);
- if (trial->group_name() == "NoChecks")
- g_use_global_check_states = false;
- }
}
FieldTrialList::~FieldTrialList() {
@@ -391,18 +328,6 @@
}
// static
-void FieldTrialList::EnableGlobalStateChecks() {
- CHECK(!g_use_global_check_states);
- g_use_global_check_states = true;
- g_append_debug_token_to_trial_string = true;
-}
-
-// static
-int32_t FieldTrialList::GetDebugToken() {
- return g_debug_token;
-}
-
-// static
FieldTrial* FieldTrialList::FactoryGetFieldTrial(
const std::string& trial_name,
FieldTrial::Probability total_probability,
@@ -534,12 +459,6 @@
output->append(it->group_name);
output->append(1, kPersistentStringSeparator);
}
- if (g_append_debug_token_to_trial_string) {
- output->append("DebugToken");
- output->append(1, kPersistentStringSeparator);
- output->append(IntToString(g_debug_token));
- output->append(1, kPersistentStringSeparator);
- }
}
// static
@@ -562,14 +481,6 @@
output->append(1, kPersistentStringSeparator);
trial.group_name.AppendToString(output);
output->append(1, kPersistentStringSeparator);
-
- // TODO(asvitkine): Remove these when http://crbug.com/359406 is fixed.
- CheckTrialGroup(trial.trial_name.as_string(), trial.group_name.as_string(),
- &global_->seen_states_);
- if (g_use_global_check_states) {
- CheckTrialGroup(trial.trial_name.as_string(),
- trial.group_name.as_string(), &g_seen_states.Get());
- }
}
}
@@ -694,16 +605,6 @@
if (!field_trial->enable_field_trial_)
return;
- // TODO(asvitkine): Remove this block when http://crbug.com/359406 is fixed.
- {
- AutoLock auto_lock(global_->lock_);
- CheckTrialGroup(field_trial->trial_name(),
- field_trial->group_name_internal(), &global_->seen_states_);
- if (g_use_global_check_states) {
- CheckTrialGroup(field_trial->trial_name(),
- field_trial->group_name_internal(), &g_seen_states.Get());
- }
- }
global_->observer_list_->Notify(
FROM_HERE, &FieldTrialList::Observer::OnFieldTrialGroupFinalized,
field_trial->trial_name(), field_trial->group_name_internal());
diff --git a/base/metrics/field_trial.h b/base/metrics/field_trial.h
index fc6237a..28a4606 100644
--- a/base/metrics/field_trial.h
+++ b/base/metrics/field_trial.h
@@ -347,20 +347,6 @@
// Destructor Release()'s references to all registered FieldTrial instances.
~FieldTrialList();
- // TODO(asvitkine): Temporary function to diagnose http://crbug.com/359406.
- // Remove when that bug is fixed. This enables using a global map that checks
- // the state of field trials between possible FieldTrialList instances. If
- // enabled, a CHECK will be hit if it's seen that a field trial is given a
- // different state then what was specified to a renderer process launch
- // command line.
- static void EnableGlobalStateChecks();
-
- // TODO(asvitkine): Temporary function to diagnose http://crbug.com/359406.
- // Remove when that bug is fixed. This returns a unique token generated during
- // FieldTrialList construction. This is used to verify that this value stays
- // consistent between renderer process invocations.
- static int32_t GetDebugToken();
-
// Get a FieldTrial instance from the factory.
//
// |name| is used to register the instance with the FieldTrialList class,