From a0a12d5a031878cee3e8532bc5817ad84a55cd87 Mon Sep 17 00:00:00 2001 From: georgesak Date: Wed, 14 Jan 2015 14:32:40 -0800 Subject: [PATCH] Send all field trials from the browser. - Switching behavior of command line to send all field trials from the browser to the renderers. - Added notification from renderer to browser whenever a trial is activated. BUG=430924 Review URL: https://codereview.chromium.org/700953002 Cr-Commit-Position: refs/heads/master@{#311554} --- base/metrics/field_trial.cc | 37 ++++++++ base/metrics/field_trial.h | 27 ++++++ base/metrics/field_trial_unittest.cc | 105 +++++++++++++++++++++ .../renderer_host/chrome_render_message_filter.cc | 11 +++ .../renderer_host/chrome_render_message_filter.h | 4 + chrome/common/render_messages.h | 4 + chrome/renderer/chrome_render_process_observer.cc | 13 ++- chrome/renderer/chrome_render_process_observer.h | 8 +- .../renderer_host/render_process_host_impl.cc | 2 +- content/renderer/renderer_main.cc | 4 +- 10 files changed, 208 insertions(+), 7 deletions(-) diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc index 7efca7a3ef53..e03c94c59699 100644 --- a/base/metrics/field_trial.cc +++ b/base/metrics/field_trial.cc @@ -223,6 +223,20 @@ bool FieldTrial::GetActiveGroup(ActiveGroup* active_group) const { return true; } +bool FieldTrial::GetState(FieldTrialState* field_trial_state) const { + if (!enable_field_trial_) + return false; + field_trial_state->trial_name = trial_name_; + // If the group name is empty (hasn't been finalized yet), use the default + // group name instead. + if (!group_name_.empty()) + field_trial_state->group_name = group_name_; + else + field_trial_state->group_name = default_group_name_; + field_trial_state->activated = group_reported_; + return true; +} + //------------------------------------------------------------------------------ // FieldTrialList methods and members. @@ -387,6 +401,29 @@ void FieldTrialList::StatesToString(std::string* output) { } // static +void FieldTrialList::AllStatesToString(std::string* output) { + if (!global_) + return; + AutoLock auto_lock(global_->lock_); + + for (const auto& registered : global_->registered_) { + FieldTrial::FieldTrialState trial; + if (!registered.second->GetState(&trial)) + continue; + DCHECK_EQ(std::string::npos, + trial.trial_name.find(kPersistentStringSeparator)); + DCHECK_EQ(std::string::npos, + trial.group_name.find(kPersistentStringSeparator)); + if (trial.activated) + output->append(1, kActivationMarker); + output->append(trial.trial_name); + output->append(1, kPersistentStringSeparator); + output->append(trial.group_name); + output->append(1, kPersistentStringSeparator); + } +} + +// static void FieldTrialList::GetActiveFieldTrialGroups( FieldTrial::ActiveGroups* active_groups) { DCHECK(active_groups->empty()); diff --git a/base/metrics/field_trial.h b/base/metrics/field_trial.h index e2e543947a10..26257ab4a890 100644 --- a/base/metrics/field_trial.h +++ b/base/metrics/field_trial.h @@ -106,6 +106,14 @@ class BASE_EXPORT FieldTrial : public RefCounted { std::string group_name; }; + // A triplet representing a FieldTrial, its selected group and whether it's + // active. + struct FieldTrialState { + std::string trial_name; + std::string group_name; + bool activated; + }; + typedef std::vector ActiveGroups; // A return value to indicate that a given instance has not yet had a group @@ -180,8 +188,10 @@ class BASE_EXPORT FieldTrial : public RefCounted { FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, OneWinner); FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, DisableProbability); FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, ActiveGroups); + FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, AllGroups); FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, ActiveGroupsNotFinalized); FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, Save); + FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SaveAll); FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, DuplicateRestore); FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedTurnFeatureOff); FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedTurnFeatureOn); @@ -230,6 +240,13 @@ class BASE_EXPORT FieldTrial : public RefCounted { // untouched. bool GetActiveGroup(ActiveGroup* active_group) const; + // Returns the trial name and selected group name for this field trial via + // the output parameter |field_trial_state|, but only if the trial has not + // been disabled. In that case, true is returned and |field_trial_state| is + // filled in; otherwise, the result is false and |field_trial_state| is left + // untouched. + bool GetState(FieldTrialState* field_trial_state) const; + // Returns the group_name. A winner need not have been chosen. std::string group_name_internal() const { return group_name_; } @@ -404,6 +421,16 @@ class BASE_EXPORT FieldTrialList { // by |CreateTrialsFromString()|. static void StatesToString(std::string* output); + // Creates a persistent representation of all FieldTrial instances for + // resurrection in another process. This allows randomization to be done in + // one process, and secondary processes can be synchronized on the result. + // The resulting string contains the name and group name pairs of all + // registered FieldTrials which have not been disabled, with "/" used + // to separate all names and to terminate the string. All activated trials + // have their name prefixed with "*". This string is parsed by + // |CreateTrialsFromString()|. + static void AllStatesToString(std::string* output); + // Fills in the supplied vector |active_groups| (which must be empty when // called) with a snapshot of all registered FieldTrials for which the group // has been chosen and externally observed (via |group()|) and which have diff --git a/base/metrics/field_trial_unittest.cc b/base/metrics/field_trial_unittest.cc index ce95c2ae15b1..f1a104258525 100644 --- a/base/metrics/field_trial_unittest.cc +++ b/base/metrics/field_trial_unittest.cc @@ -311,6 +311,36 @@ TEST_F(FieldTrialTest, ActiveGroups) { } } +TEST_F(FieldTrialTest, AllGroups) { + FieldTrial::FieldTrialState field_trial_state; + std::string one_winner("One Winner"); + scoped_refptr trial = + CreateFieldTrial(one_winner, 10, "Default", NULL); + std::string winner("Winner"); + trial->AppendGroup(winner, 10); + EXPECT_TRUE(trial->GetState(&field_trial_state)); + EXPECT_EQ(one_winner, field_trial_state.trial_name); + EXPECT_EQ(winner, field_trial_state.group_name); + trial->group(); + EXPECT_TRUE(trial->GetState(&field_trial_state)); + EXPECT_EQ(one_winner, field_trial_state.trial_name); + EXPECT_EQ(winner, field_trial_state.group_name); + + std::string multi_group("MultiGroup"); + scoped_refptr multi_group_trial = + CreateFieldTrial(multi_group, 9, "Default", NULL); + + multi_group_trial->AppendGroup("Me", 3); + multi_group_trial->AppendGroup("You", 3); + multi_group_trial->AppendGroup("Them", 3); + EXPECT_TRUE(multi_group_trial->GetState(&field_trial_state)); + // Finalize the group selection by accessing the selected group. + multi_group_trial->group(); + EXPECT_TRUE(multi_group_trial->GetState(&field_trial_state)); + EXPECT_EQ(multi_group, field_trial_state.trial_name); + EXPECT_EQ(multi_group_trial->group_name(), field_trial_state.group_name); +} + TEST_F(FieldTrialTest, ActiveGroupsNotFinalized) { const char kTrialName[] = "TestTrial"; const char kSecondaryGroupName[] = "SecondaryGroup"; @@ -388,6 +418,44 @@ TEST_F(FieldTrialTest, Save) { EXPECT_EQ("Some name/Winner/xxx/yyyy/zzz/default/", save_string); } +TEST_F(FieldTrialTest, SaveAll) { + std::string save_string; + + scoped_refptr trial = + CreateFieldTrial("Some name", 10, "Default some name", NULL); + EXPECT_EQ("", trial->group_name_internal()); + FieldTrialList::AllStatesToString(&save_string); + EXPECT_EQ("Some name/Default some name/", save_string); + save_string.clear(); + + // Create a winning group. + trial->AppendGroup("Winner", 10); + // Finalize the group selection by accessing the selected group. + trial->group(); + FieldTrialList::AllStatesToString(&save_string); + EXPECT_EQ("*Some name/Winner/", save_string); + save_string.clear(); + + // Create a second trial and winning group. + scoped_refptr trial2 = + CreateFieldTrial("xxx", 10, "Default xxx", NULL); + trial2->AppendGroup("yyyy", 10); + // Finalize the group selection by accessing the selected group. + trial2->group(); + + FieldTrialList::AllStatesToString(&save_string); + // We assume names are alphabetized... though this is not critical. + EXPECT_EQ("*Some name/Winner/*xxx/yyyy/", save_string); + save_string.clear(); + + // Create a third trial with only the default group. + scoped_refptr trial3 = + CreateFieldTrial("zzz", 10, "default", NULL); + + FieldTrialList::AllStatesToString(&save_string); + EXPECT_EQ("*Some name/Winner/*xxx/yyyy/zzz/default/", save_string); +} + TEST_F(FieldTrialTest, Restore) { ASSERT_FALSE(FieldTrialList::TrialExists("Some_name")); ASSERT_FALSE(FieldTrialList::TrialExists("xxx")); @@ -1014,6 +1082,43 @@ TEST_F(FieldTrialTest, CreateSimulatedFieldTrial) { } } +TEST(FieldTrialTestWithoutList, StatesStringFormat) { + std::string save_string; + + // Scoping the first FieldTrialList, as we need another one to test the + // importing function. + { + FieldTrialList field_trial_list(NULL); + scoped_refptr trial = + CreateFieldTrial("Abc", 10, "Default some name", NULL); + trial->AppendGroup("cba", 10); + trial->group(); + scoped_refptr trial2 = + CreateFieldTrial("Xyz", 10, "Default xxx", NULL); + trial2->AppendGroup("zyx", 10); + trial2->group(); + scoped_refptr trial3 = + CreateFieldTrial("zzz", 10, "default", NULL); + + FieldTrialList::AllStatesToString(&save_string); + } + + // Starting with a new blank FieldTrialList. + FieldTrialList field_trial_list(NULL); + ASSERT_TRUE(field_trial_list.CreateTrialsFromString( + save_string, FieldTrialList::DONT_ACTIVATE_TRIALS, + std::set())); + + FieldTrial::ActiveGroups active_groups; + field_trial_list.GetActiveFieldTrialGroups(&active_groups); + ASSERT_EQ(2U, active_groups.size()); + EXPECT_EQ("Abc", active_groups[0].trial_name); + EXPECT_EQ("cba", active_groups[0].group_name); + EXPECT_EQ("Xyz", active_groups[1].trial_name); + EXPECT_EQ("zyx", active_groups[1].group_name); + EXPECT_TRUE(field_trial_list.TrialExists("zzz")); +} + #if GTEST_HAS_DEATH_TEST TEST(FieldTrialDeathTest, OneTimeRandomizedTrialWithoutFieldTrialList) { // Trying to instantiate a one-time randomized field trial before the diff --git a/chrome/browser/renderer_host/chrome_render_message_filter.cc b/chrome/browser/renderer_host/chrome_render_message_filter.cc index 2ffe311d8a31..78860b50ed1c 100644 --- a/chrome/browser/renderer_host/chrome_render_message_filter.cc +++ b/chrome/browser/renderer_host/chrome_render_message_filter.cc @@ -8,6 +8,7 @@ #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/metrics/field_trial.h" #include "base/metrics/histogram.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/content_settings/cookie_settings.h" @@ -84,6 +85,8 @@ bool ChromeRenderMessageFilter::OnMessageReceived(const IPC::Message& message) { IPC_MESSAGE_HANDLER(ChromeViewHostMsg_IsCrashReportingEnabled, OnIsCrashReportingEnabled) #endif + IPC_MESSAGE_HANDLER(ChromeViewHostMsg_FieldTrialActivated, + OnFieldTrialActivated) IPC_MESSAGE_UNHANDLED(handled = false) IPC_END_MESSAGE_MAP() @@ -363,3 +366,11 @@ void ChromeRenderMessageFilter::OnIsCrashReportingEnabled(bool* enabled) { *enabled = ChromeMetricsServiceAccessor::IsCrashReportingEnabled(); } #endif + +void ChromeRenderMessageFilter::OnFieldTrialActivated( + const std::string& trial_name) { + // Activate the trial in the browser process to match its state in the + // renderer. This is done by calling FindFullName which finalizes the group + // and activates the trial. + base::FieldTrialList::FindFullName(trial_name); +} diff --git a/chrome/browser/renderer_host/chrome_render_message_filter.h b/chrome/browser/renderer_host/chrome_render_message_filter.h index 4ece8753305f..f7e9bc245793 100644 --- a/chrome/browser/renderer_host/chrome_render_message_filter.h +++ b/chrome/browser/renderer_host/chrome_render_message_filter.h @@ -122,6 +122,10 @@ class ChromeRenderMessageFilter : public content::BrowserMessageFilter { void OnIsCrashReportingEnabled(bool* enabled); #endif + // Called when a message is received from a renderer that a trial has been + // activated (ChromeViewHostMsg_FieldTrialActivated). + void OnFieldTrialActivated(const std::string& trial_name); + const int render_process_id_; // The Profile associated with our renderer process. This should only be diff --git a/chrome/common/render_messages.h b/chrome/common/render_messages.h index 409c67410af3..b1a508a0e1cd 100644 --- a/chrome/common/render_messages.h +++ b/chrome/common/render_messages.h @@ -673,3 +673,7 @@ IPC_MESSAGE_CONTROL2(ChromeViewMsg_SetSearchURLs, IPC_SYNC_MESSAGE_CONTROL0_1(ChromeViewHostMsg_IsCrashReportingEnabled, bool /* enabled */) #endif + +// Sent by the renderer to indicate that a fields trial has been activated. +IPC_MESSAGE_CONTROL1(ChromeViewHostMsg_FieldTrialActivated, + std::string /* name */) diff --git a/chrome/renderer/chrome_render_process_observer.cc b/chrome/renderer/chrome_render_process_observer.cc index 2e8d4ee94322..860dd326207e 100644 --- a/chrome/renderer/chrome_render_process_observer.cc +++ b/chrome/renderer/chrome_render_process_observer.cc @@ -305,6 +305,8 @@ ChromeRenderProcessObserver::ChromeRenderProcessObserver( #endif // Setup initial set of crash dump data for Field Trials in this renderer. chrome_variations::SetChildProcessLoggingVariationList(); + // Listen for field trial activations to report them to the browser. + base::FieldTrialList::AddObserver(this); } ChromeRenderProcessObserver::~ChromeRenderProcessObserver() { @@ -371,8 +373,8 @@ void ChromeRenderProcessObserver::OnSetFieldTrialGroup( base::FieldTrialList::CreateFieldTrial(field_trial_name, group_name); // TODO(mef): Remove this check after the investigation of 359406 is complete. CHECK(trial) << field_trial_name << ":" << group_name; - // Ensure the trial is marked as "used" by calling group() on it. This is - // needed to ensure the trial is properly reported in renderer crash reports. + // Ensure the trial is marked as "used" by calling group() on it if it is + // marked as activated. trial->group(); chrome_variations::SetChildProcessLoggingVariationList(); } @@ -385,3 +387,10 @@ const RendererContentSettingRules* ChromeRenderProcessObserver::content_setting_rules() const { return &content_setting_rules_; } + +void ChromeRenderProcessObserver::OnFieldTrialGroupFinalized( + const std::string& trial_name, + const std::string& group_name) { + content::RenderThread::Get()->Send( + new ChromeViewHostMsg_FieldTrialActivated(trial_name)); +} diff --git a/chrome/renderer/chrome_render_process_observer.h b/chrome/renderer/chrome_render_process_observer.h index 75d8415e3997..7bee7941d591 100644 --- a/chrome/renderer/chrome_render_process_observer.h +++ b/chrome/renderer/chrome_render_process_observer.h @@ -10,6 +10,7 @@ #include "base/compiler_specific.h" #include "base/files/file_path.h" #include "base/memory/scoped_ptr.h" +#include "base/metrics/field_trial.h" #include "components/content_settings/core/common/content_settings.h" #include "content/public/renderer/render_process_observer.h" @@ -25,7 +26,8 @@ class ResourceDispatcherDelegate; // a RenderView) for Chrome specific messages that the content layer doesn't // happen. If a few messages are related, they should probably have their own // observer. -class ChromeRenderProcessObserver : public content::RenderProcessObserver { +class ChromeRenderProcessObserver : public content::RenderProcessObserver, + public base::FieldTrialList::Observer { public: explicit ChromeRenderProcessObserver( ChromeContentRendererClient* client); @@ -43,6 +45,10 @@ class ChromeRenderProcessObserver : public content::RenderProcessObserver { void WebKitInitialized() override; void OnRenderProcessShutdown() override; + // Observer implementation. + void OnFieldTrialGroupFinalized(const std::string& trial_name, + const std::string& group_name) override; + void OnSetIsIncognitoProcess(bool is_incognito_process); void OnSetContentSettingsForCurrentURL( const GURL& url, const ContentSettings& content_settings); diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc index 37e8b336ffba..fafd773952f5 100644 --- a/content/browser/renderer_host/render_process_host_impl.cc +++ b/content/browser/renderer_host/render_process_host_impl.cc @@ -1144,7 +1144,7 @@ void RenderProcessHostImpl::AppendRendererCommandLine( // renderer so that it can act in accordance with each state, or record // histograms relating to the base::FieldTrial states. std::string field_trial_states; - base::FieldTrialList::StatesToString(&field_trial_states); + base::FieldTrialList::AllStatesToString(&field_trial_states); if (!field_trial_states.empty()) { command_line->AppendSwitchASCII(switches::kForceFieldTrials, field_trial_states); diff --git a/content/renderer/renderer_main.cc b/content/renderer/renderer_main.cc index 762dd76be57f..32ec616b6ffc 100644 --- a/content/renderer/renderer_main.cc +++ b/content/renderer/renderer_main.cc @@ -169,11 +169,9 @@ int RendererMain(const MainFunctionParams& parameters) { base::FieldTrialList field_trial_list(NULL); // Ensure any field trials in browser are reflected into renderer. if (parsed_command_line.HasSwitch(switches::kForceFieldTrials)) { - // Field trials are created in an "activated" state to ensure they get - // reported in crash reports. bool result = base::FieldTrialList::CreateTrialsFromString( parsed_command_line.GetSwitchValueASCII(switches::kForceFieldTrials), - base::FieldTrialList::ACTIVATE_TRIALS, + base::FieldTrialList::DONT_ACTIVATE_TRIALS, std::set()); DCHECK(result); } -- 2.11.4.GIT