Commit 105f942e authored by Alexei Svitkine's avatar Alexei Svitkine Committed by Commit Bot

Don't use CHECK() to quit browser on bad field trial command line.

CHECK() produces crashes and sometimes these are prevalent enough
for crash triagers to file and ping crbugs about, which just wastes
everyone's time. This changes the CHECKs to a printed message via
puts and an exit() call.

Differentiates between invalid variation ids coming from command
line vs. those from about:flags and uses a NOTREACHED in the latter
case.

Also, cleans up the code a bit by changing the APIs around passing
additional variation ids - making the param const and making a
helper function private. Also fixes some lint errors around includes
and header guards.

BUG=812164
TBR=sky@chromium.org,rohitrao@chromium.org

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Id765738791ada14540ff51fe5f6fc837dd42e7c7
Reviewed-on: https://chromium-review.googlesource.com/919194Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Reviewed-by: default avatarIlya Sherman <isherman@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537516}
parent 232d27cb
......@@ -106,7 +106,6 @@ void AwFieldTrialCreator::SetUpFieldTrials() {
// VariationsFieldTrialCreator::SetupFieldTrials().
// TODO(isherman): We might want a more genuine SafeSeedManager:
// https://crbug.com/801771
std::vector<std::string> variation_ids;
std::set<std::string> unforceable_field_trials;
variations::SafeSeedManager ignored_safe_seed_manager(true,
local_state.get());
......@@ -115,8 +114,9 @@ void AwFieldTrialCreator::SetUpFieldTrials() {
variations_field_trial_creator_->SetupFieldTrials(
cc::switches::kEnableGpuBenchmarking, switches::kEnableFeatures,
switches::kDisableFeatures, unforceable_field_trials,
CreateLowEntropyProvider(), std::make_unique<base::FeatureList>(),
&variation_ids, aw_field_trials_.get(), &ignored_safe_seed_manager);
std::vector<std::string>(), CreateLowEntropyProvider(),
std::make_unique<base::FeatureList>(), aw_field_trials_.get(),
&ignored_safe_seed_manager);
}
} // namespace android_webview
......@@ -934,8 +934,8 @@ void ChromeBrowserMainParts::SetupFieldTrials() {
browser_process_->variations_service();
variations_service->SetupFieldTrials(
cc::switches::kEnableGpuBenchmarking, switches::kEnableFeatures,
switches::kDisableFeatures, unforceable_field_trials,
std::move(feature_list), &variation_ids, &browser_field_trials_);
switches::kDisableFeatures, unforceable_field_trials, variation_ids,
std::move(feature_list), &browser_field_trials_);
// Initialize FieldTrialSynchronizer system. This is a singleton and is used
// for posting tasks via base::Bind. Its deleted when it goes out of scope.
......
......@@ -49,7 +49,7 @@ class VariationsHttpHeadersBrowserTest : public InProcessBrowserTest {
// Set up some fake variations.
auto* variations_provider =
variations::VariationsHttpHeaderProvider::GetInstance();
variations_provider->SetDefaultVariationIds({"12", "456", "t789"});
variations_provider->ForceVariationIds({"12", "456", "t789"}, "");
}
void SetUpCommandLine(base::CommandLine* command_line) override {
......
......@@ -116,8 +116,8 @@ struct FeatureEntry {
const FeatureParam* params;
int num_params;
// A variation id number in the format of
// VariationsHttpHeaderProvider::SetDefaultVariationIds or nullptr
// if you do not need to set any variation_id for this feature variation.
// VariationsHttpHeaderProvider::ForceVariationIds() or nullptr if you do
// not need to set any variation_id for this feature variation.
const char* variation_id;
};
......@@ -203,8 +203,8 @@ namespace testing {
// name-of-experiment + kMultiSeparator + selected_index.
extern const char kMultiSeparator[];
} // namespace
} // namespace testing
} // namespace flag_ui
} // namespace flags_ui
#endif // COMPONENTS_FLAGS_UI_FEATURE_ENTRY_H_
......@@ -6,8 +6,11 @@
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <memory>
#include <set>
#include <utility>
#include <vector>
......@@ -16,6 +19,7 @@
#include "base/command_line.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/sparse_histogram.h"
#include "base/strings/stringprintf.h"
#include "base/sys_info.h"
#include "base/trace_event/trace_event.h"
#include "base/version.h"
......@@ -139,6 +143,14 @@ void RecordSeedFreshness(base::TimeDelta seed_age) {
1, base::TimeDelta::FromDays(30).InMinutes(), 50);
}
// If an invalid command-line to force field trials was specified, exit the
// browser with a helpful error message, so that the user can correct their
// mistake.
void ExitWithMessage(const std::string& message) {
puts(message.c_str());
exit(1);
}
} // namespace
VariationsFieldTrialCreator::VariationsFieldTrialCreator(
......@@ -402,10 +414,10 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
const char* kEnableFeatures,
const char* kDisableFeatures,
const std::set<std::string>& unforceable_field_trials,
const std::vector<std::string>& variation_ids,
std::unique_ptr<const base::FieldTrial::EntropyProvider>
low_entropy_provider,
std::unique_ptr<base::FeatureList> feature_list,
std::vector<std::string>* variation_ids,
PlatformFieldTrials* platform_field_trials,
SafeSeedManager* safe_seed_manager) {
const base::CommandLine* command_line =
......@@ -418,8 +430,10 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
if (command_line->HasSwitch(switches::kForceFieldTrialParams)) {
bool result = AssociateParamsFromString(
command_line->GetSwitchValueASCII(switches::kForceFieldTrialParams));
CHECK(result) << "Invalid --" << switches::kForceFieldTrialParams
<< " list specified.";
if (!result) {
ExitWithMessage(base::StringPrintf("Invalid --%s list specified.",
switches::kForceFieldTrialParams));
}
}
// Ensure any field trials specified on the command line are initialized.
......@@ -429,19 +443,32 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
bool result = base::FieldTrialList::CreateTrialsFromString(
command_line->GetSwitchValueASCII(::switches::kForceFieldTrials),
unforceable_field_trials);
CHECK(result) << "Invalid --" << ::switches::kForceFieldTrials
<< " list specified.";
if (!result) {
ExitWithMessage(base::StringPrintf("Invalid --%s list specified.",
::switches::kForceFieldTrials));
}
}
VariationsHttpHeaderProvider* http_header_provider =
VariationsHttpHeaderProvider::GetInstance();
// Force the variation ids selected in chrome://flags and/or specified using
// the command-line flag.
bool result = http_header_provider->ForceVariationIds(
command_line->GetSwitchValueASCII(switches::kForceVariationIds),
variation_ids);
CHECK(result) << "Invalid list of variation ids specified (either in --"
<< switches::kForceVariationIds << " or in chrome://flags)";
auto result = http_header_provider->ForceVariationIds(
variation_ids,
command_line->GetSwitchValueASCII(switches::kForceVariationIds));
switch (result) {
case VariationsHttpHeaderProvider::ForceIdsResult::INVALID_SWITCH_ENTRY:
ExitWithMessage(base::StringPrintf("Invalid --%s list specified.",
switches::kForceVariationIds));
break;
case VariationsHttpHeaderProvider::ForceIdsResult::INVALID_VECTOR_ENTRY:
// It should not be possible to have invalid variation ids from the
// vector param (which corresponds to chrome://flags).
NOTREACHED();
break;
case VariationsHttpHeaderProvider::ForceIdsResult::SUCCESS:
break;
}
feature_list->InitializeFromCommandLine(
command_line->GetSwitchValueASCII(kEnableFeatures),
......@@ -471,6 +498,6 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
VariationsSeedStore* VariationsFieldTrialCreator::GetSeedStore() {
return &seed_store_;
};
}
} // namespace variations
......@@ -2,10 +2,13 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef COMPONENTS_VARIATIONS_FIELD_TRIAL_CREATOR_H_
#define COMPONENTS_VARIATIONS_FIELD_TRIAL_CREATOR_H_
#ifndef COMPONENTS_VARIATIONS_SERVICE_VARIATIONS_FIELD_TRIAL_CREATOR_H_
#define COMPONENTS_VARIATIONS_SERVICE_VARIATIONS_FIELD_TRIAL_CREATOR_H_
#include <memory>
#include <set>
#include <string>
#include <vector>
#include "base/compiler_specific.h"
#include "base/macros.h"
......@@ -42,10 +45,10 @@ class VariationsFieldTrialCreator {
// feature controlling flags not directly accesible from variations.
// |unforcable_field_trials| contains the list of trials that can not be
// overridden.
// |low_entropy_provider| allows for field trial randomization.
// |feature_list| contains the list of all active features for this client.
// |variation_ids| allows for forcing ids selected in chrome://flags and/or
// specified using the command-line flag.
// |low_entropy_provider| allows for field trial randomization.
// |feature_list| contains the list of all active features for this client.
// |platform_field_trials| provides the platform specific field trial set up
// for Chrome.
// |safe_seed_manager| should be notified of the combined server and client
......@@ -55,10 +58,10 @@ class VariationsFieldTrialCreator {
const char* kEnableFeatures,
const char* kDisableFeatures,
const std::set<std::string>& unforceable_field_trials,
const std::vector<std::string>& variation_ids,
std::unique_ptr<const base::FieldTrial::EntropyProvider>
low_entropy_provider,
std::unique_ptr<base::FeatureList> feature_list,
std::vector<std::string>* variation_ids,
PlatformFieldTrials* platform_field_trials,
SafeSeedManager* safe_seed_manager);
......@@ -151,4 +154,4 @@ class VariationsFieldTrialCreator {
} // namespace variations
#endif // COMPONENTS_VARIATIONS_FIELD_TRIAL_CREATOR_H_
#endif // COMPONENTS_VARIATIONS_SERVICE_VARIATIONS_FIELD_TRIAL_CREATOR_H_
......@@ -200,12 +200,11 @@ class TestVariationsFieldTrialCreator : public VariationsFieldTrialCreator {
// A convenience wrapper around SetupFieldTrials() which passes default values
// for uninteresting params.
bool SetupFieldTrials() {
std::vector<std::string> variation_ids;
TestPlatformFieldTrials platform_field_trials;
return VariationsFieldTrialCreator::SetupFieldTrials(
"", "", "", std::set<std::string>(), nullptr,
std::make_unique<base::FeatureList>(), &variation_ids,
&platform_field_trials, safe_seed_manager_);
"", "", "", std::set<std::string>(), std::vector<std::string>(),
nullptr, std::make_unique<base::FeatureList>(), &platform_field_trials,
safe_seed_manager_);
}
TestVariationsSeedStore* seed_store() { return &seed_store_; }
......
......@@ -758,14 +758,13 @@ bool VariationsService::SetupFieldTrials(
const char* kEnableFeatures,
const char* kDisableFeatures,
const std::set<std::string>& unforceable_field_trials,
const std::vector<std::string>& variation_ids,
std::unique_ptr<base::FeatureList> feature_list,
std::vector<std::string>* variation_ids,
variations::PlatformFieldTrials* platform_field_trials) {
return field_trial_creator_.SetupFieldTrials(
kEnableGpuBenchmarking, kEnableFeatures, kDisableFeatures,
unforceable_field_trials, CreateLowEntropyProvider(),
std::move(feature_list), variation_ids, platform_field_trials,
&safe_seed_manager_);
unforceable_field_trials, variation_ids, CreateLowEntropyProvider(),
std::move(feature_list), platform_field_trials, &safe_seed_manager_);
}
std::string VariationsService::GetStoredPermanentCountry() {
......
......@@ -6,7 +6,9 @@
#define COMPONENTS_VARIATIONS_SERVICE_VARIATIONS_SERVICE_H_
#include <memory>
#include <set>
#include <string>
#include <vector>
#include "base/compiler_specific.h"
#include "base/gtest_prod_util.h"
......@@ -163,8 +165,8 @@ class VariationsService
const char* kEnableFeatures,
const char* kDisableFeatures,
const std::set<std::string>& unforceable_field_trials,
const std::vector<std::string>& variation_ids,
std::unique_ptr<base::FeatureList> feature_list,
std::vector<std::string>* variation_ids,
variations::PlatformFieldTrials* platform_field_trials);
protected:
......
......@@ -61,49 +61,23 @@ std::string VariationsHttpHeaderProvider::GetVariationsString() {
return ids_string;
}
bool VariationsHttpHeaderProvider::ForceVariationIds(
const std::string& command_line_variation_ids,
std::vector<std::string>* variation_ids) {
VariationsHttpHeaderProvider::ForceIdsResult
VariationsHttpHeaderProvider::ForceVariationIds(
const std::vector<std::string>& variation_ids,
const std::string& command_line_variation_ids) {
default_variation_ids_set_.clear();
if (!AddDefaultVariationIds(variation_ids))
return ForceIdsResult::INVALID_VECTOR_ENTRY;
if (!command_line_variation_ids.empty()) {
// Combine |variation_ids| with |command_line_variation_ids|.
std::vector<std::string> variation_ids_flags =
std::vector<std::string> variation_ids_from_command_line =
base::SplitString(command_line_variation_ids, ",",
base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
variation_ids->insert(variation_ids->end(), variation_ids_flags.begin(),
variation_ids_flags.end());
if (!AddDefaultVariationIds(variation_ids_from_command_line))
return ForceIdsResult::INVALID_SWITCH_ENTRY;
}
if (!variation_ids->empty()) {
// Create default variation ids which will always be included in the
// X-Client-Data request header.
return SetDefaultVariationIds(*variation_ids);
}
return true;
}
bool VariationsHttpHeaderProvider::SetDefaultVariationIds(
const std::vector<std::string>& variation_ids) {
default_variation_ids_set_.clear();
for (const std::string& entry : variation_ids) {
if (entry.empty()) {
default_variation_ids_set_.clear();
return false;
}
bool trigger_id =
base::StartsWith(entry, "t", base::CompareCase::SENSITIVE);
// Remove the "t" prefix if it's there.
std::string trimmed_entry = trigger_id ? entry.substr(1) : entry;
int variation_id = 0;
if (!base::StringToInt(trimmed_entry, &variation_id)) {
default_variation_ids_set_.clear();
return false;
}
default_variation_ids_set_.insert(VariationIDEntry(
variation_id,
trigger_id ? GOOGLE_WEB_PROPERTIES_TRIGGER : GOOGLE_WEB_PROPERTIES));
}
return true;
return ForceIdsResult::SUCCESS;
}
void VariationsHttpHeaderProvider::ResetForTesting() {
......@@ -260,6 +234,30 @@ std::string VariationsHttpHeaderProvider::GenerateBase64EncodedProto(
return hashed;
}
bool VariationsHttpHeaderProvider::AddDefaultVariationIds(
const std::vector<std::string>& variation_ids) {
for (const std::string& entry : variation_ids) {
if (entry.empty()) {
default_variation_ids_set_.clear();
return false;
}
bool trigger_id =
base::StartsWith(entry, "t", base::CompareCase::SENSITIVE);
// Remove the "t" prefix if it's there.
std::string trimmed_entry = trigger_id ? entry.substr(1) : entry;
int variation_id = 0;
if (!base::StringToInt(trimmed_entry, &variation_id)) {
default_variation_ids_set_.clear();
return false;
}
default_variation_ids_set_.insert(VariationIDEntry(
variation_id,
trigger_id ? GOOGLE_WEB_PROPERTIES_TRIGGER : GOOGLE_WEB_PROPERTIES));
}
return true;
}
std::set<VariationsHttpHeaderProvider::VariationIDEntry>
VariationsHttpHeaderProvider::GetAllVariationIds() {
lock_.AssertAcquired();
......
......@@ -45,19 +45,21 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer,
// a leading and trailing space, e.g. " 123 234 345 ".
std::string GetVariationsString();
// Forces the list of |variation_ids| (which will be modified by adding the
// comma-separated |command_line_variation_ids|). This is a wrapper function
// around SetDefaultVariationIds.
bool ForceVariationIds(
const std::string& command_line_variation_ids,
std::vector<std::string>* variation_ids);
// Result of ForceVariationIds() call.
enum class ForceIdsResult {
SUCCESS,
INVALID_VECTOR_ENTRY, // Invalid entry in |variation_ids|.
INVALID_SWITCH_ENTRY, // Invalid entry in |command_line_variation_ids|.
};
// Sets *additional* variation ids and trigger variation ids to be encoded in
// the X-Client-Data request header. This is intended for development use to
// force a server side experiment id. |variation_ids| should be a list of
// strings of numeric experiment ids. If an id is prefixed with "t" it will
// be treated as a trigger experiment id.
bool SetDefaultVariationIds(const std::vector<std::string>& variation_ids);
// strings of numeric experiment ids. Ids explicitly passed in |variation_ids|
// and those in the comma-separated |command_line_variation_ids| are added.
ForceIdsResult ForceVariationIds(
const std::vector<std::string>& variation_ids,
const std::string& command_line_variation_ids);
// Resets any cached state for tests.
void ResetForTesting();
......@@ -68,9 +70,11 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer,
typedef std::pair<VariationID, IDCollectionKey> VariationIDEntry;
FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest,
SetDefaultVariationIds_Valid);
ForceVariationIds_Valid);
FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest,
SetDefaultVariationIds_Invalid);
ForceVariationIds_ValidCommandLine);
FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest,
ForceVariationIds_Invalid);
FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest,
OnFieldTrialGroupFinalized);
FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest,
......@@ -109,6 +113,10 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer,
// |is_signed_in| state.
std::string GenerateBase64EncodedProto(bool is_signed_in);
// Adds *additional* variation ids and trigger variation ids to be encoded in
// the X-Client-Data request header.
bool AddDefaultVariationIds(const std::vector<std::string>& variation_ids);
// Returns the currently active set of variation ids, which includes any
// default values, synthetic variations and actual field trial variations.
std::set<VariationIDEntry> GetAllVariationIds();
......
......@@ -60,12 +60,13 @@ class VariationsHttpHeaderProviderTest : public ::testing::Test {
void TearDown() override { testing::ClearAllVariationIDs(); }
};
TEST_F(VariationsHttpHeaderProviderTest, SetDefaultVariationIds_Valid) {
TEST_F(VariationsHttpHeaderProviderTest, ForceVariationIds_Valid) {
base::MessageLoop loop;
VariationsHttpHeaderProvider provider;
// Valid experiment ids.
EXPECT_TRUE(provider.SetDefaultVariationIds({"12", "456", "t789"}));
EXPECT_EQ(VariationsHttpHeaderProvider::ForceIdsResult::SUCCESS,
provider.ForceVariationIds({"12", "456", "t789"}, ""));
provider.InitVariationIDsCacheIfNeeded();
std::string variations = provider.GetClientDataHeader(false);
EXPECT_FALSE(variations.empty());
......@@ -78,19 +79,44 @@ TEST_F(VariationsHttpHeaderProviderTest, SetDefaultVariationIds_Valid) {
EXPECT_FALSE(variation_ids.find(789) != variation_ids.end());
}
TEST_F(VariationsHttpHeaderProviderTest, SetDefaultVariationIds_Invalid) {
TEST_F(VariationsHttpHeaderProviderTest, ForceVariationIds_ValidCommandLine) {
base::MessageLoop loop;
VariationsHttpHeaderProvider provider;
// Valid experiment ids.
EXPECT_EQ(VariationsHttpHeaderProvider::ForceIdsResult::SUCCESS,
provider.ForceVariationIds({"12"}, "456,t789"));
provider.InitVariationIDsCacheIfNeeded();
std::string variations = provider.GetClientDataHeader(false);
EXPECT_FALSE(variations.empty());
std::set<VariationID> variation_ids;
std::set<VariationID> trigger_ids;
ASSERT_TRUE(ExtractVariationIds(variations, &variation_ids, &trigger_ids));
EXPECT_TRUE(variation_ids.find(12) != variation_ids.end());
EXPECT_TRUE(variation_ids.find(456) != variation_ids.end());
EXPECT_TRUE(trigger_ids.find(789) != trigger_ids.end());
EXPECT_FALSE(variation_ids.find(789) != variation_ids.end());
}
TEST_F(VariationsHttpHeaderProviderTest, ForceVariationIds_Invalid) {
base::MessageLoop loop;
VariationsHttpHeaderProvider provider;
// Invalid experiment ids.
EXPECT_FALSE(provider.SetDefaultVariationIds(
std::vector<std::string>{"abcd12", "456"}));
EXPECT_EQ(VariationsHttpHeaderProvider::ForceIdsResult::INVALID_VECTOR_ENTRY,
provider.ForceVariationIds({"abcd12", "456"}, ""));
provider.InitVariationIDsCacheIfNeeded();
EXPECT_TRUE(provider.GetClientDataHeader(false).empty());
// Invalid trigger experiment id
EXPECT_FALSE(provider.SetDefaultVariationIds(
std::vector<std::string>{"12", "tabc456"}));
EXPECT_EQ(VariationsHttpHeaderProvider::ForceIdsResult::INVALID_VECTOR_ENTRY,
provider.ForceVariationIds({"12", "tabc456"}, ""));
provider.InitVariationIDsCacheIfNeeded();
EXPECT_TRUE(provider.GetClientDataHeader(false).empty());
// Invalid command-line ids.
EXPECT_EQ(VariationsHttpHeaderProvider::ForceIdsResult::INVALID_SWITCH_ENTRY,
provider.ForceVariationIds({"12", "50"}, "tabc456"));
provider.InitVariationIDsCacheIfNeeded();
EXPECT_TRUE(provider.GetClientDataHeader(false).empty());
}
......@@ -154,10 +180,7 @@ TEST_F(VariationsHttpHeaderProviderTest, GetVariationsString) {
CreateTrialAndAssociateId("t3", "g3", GOOGLE_WEB_PROPERTIES_SIGNED_IN, 125);
VariationsHttpHeaderProvider provider;
std::vector<std::string> ids;
ids.push_back("100");
ids.push_back("200");
provider.SetDefaultVariationIds(ids);
provider.ForceVariationIds({"100", "200"}, "");
EXPECT_EQ(" 100 123 124 200 ", provider.GetVariationsString());
}
......
......@@ -228,8 +228,8 @@ void IOSChromeMainParts::SetupFieldTrials() {
application_context_->GetVariationsService()->SetupFieldTrials(
"dummy-enable-gpu-benchmarking", switches::kEnableFeatures,
switches::kDisableFeatures,
/*unforceable_field_trials=*/std::set<std::string>(),
std::move(feature_list), &variation_ids, &ios_field_trials_);
/*unforceable_field_trials=*/std::set<std::string>(), variation_ids,
std::move(feature_list), &ios_field_trials_);
}
void IOSChromeMainParts::SetupMetrics() {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment