Field trial synchronization to PPAPI process.

This enables the same field trial synchronization mechanism we use for renderers and GPU process for the PPAPI process. With this CL, crash reports from PPAPI process will include active field trials and field trials activated in the PPAPI process will be reported to UMA and crash. Moves child_process_field_trial_syncer.cc from chrome to components along the way. BUG=582602 TEST=In an official Chrome build with crash reporting enabled, verify you have Variations hashes listed in chrome://version (restart if you don't) and then go to http://www.adobe.com/software/flash/about/ and load some flash content before going to chrome://ppapiflashcrash to cause a PPAPI crash. Then, find the the crash entry that should now appear on chrome://crashes and view it on http://crash/ via the Server ID and check that the crash report has entries under Experiments (under Fields). Review-Url: https://codereview.chromium.org/2514593002 Cr-Commit-Position: refs/heads/master@{#434384}
marcosholgado · Nov 24, 2016 · 92c1a93 · 92c1a93
1 parent 4462ce6
commit 92c1a93
Show file tree

Hide file tree

Showing 17 changed files with 76 additions and 42 deletions.
diff --git a/chrome/common/BUILD.gn b/chrome/common/BUILD.gn
@@ -150,8 +150,6 @@ static_library("common") {
     "url_constants.h",
     "v8_breakpad_support_win.cc",
     "v8_breakpad_support_win.h",
-    "variations/child_process_field_trial_syncer.cc",
-    "variations/child_process_field_trial_syncer.h",
     "web_application_info.cc",
     "web_application_info.h",
     "widevine_cdm_constants.cc",

diff --git a/chrome/gpu/chrome_content_gpu_client.cc b/chrome/gpu/chrome_content_gpu_client.cc
@@ -71,8 +71,9 @@ void ChromeContentGpuClient::Initialize(
   // No need for field trial syncer if we're in the browser process.
   if (!command_line.HasSwitch(switches::kInProcessGPU)) {
     field_trial_syncer_.reset(
-        new chrome_variations::ChildProcessFieldTrialSyncer(observer));
-    field_trial_syncer_->InitFieldTrialObserving(command_line);
+        new variations::ChildProcessFieldTrialSyncer(observer));
+    field_trial_syncer_->InitFieldTrialObserving(command_line,
+                                                 switches::kSingleProcess);
   }
 }
 

diff --git a/chrome/gpu/chrome_content_gpu_client.h b/chrome/gpu/chrome_content_gpu_client.h
@@ -9,7 +9,7 @@
 
 #include "base/macros.h"
 #include "base/profiler/stack_sampling_profiler.h"
-#include "chrome/common/variations/child_process_field_trial_syncer.h"
+#include "components/variations/child_process_field_trial_syncer.h"
 #include "content/public/gpu/content_gpu_client.h"
 
 class ChromeContentGpuClient : public content::ContentGpuClient {
@@ -26,8 +26,7 @@ class ChromeContentGpuClient : public content::ContentGpuClient {
       service_manager::InterfaceProvider* provider) override;
 
  private:
-  std::unique_ptr<chrome_variations::ChildProcessFieldTrialSyncer>
-      field_trial_syncer_;
+  std::unique_ptr<variations::ChildProcessFieldTrialSyncer> field_trial_syncer_;
   // Used to profile process startup.
   base::StackSamplingProfiler stack_sampling_profiler_;
 

diff --git a/chrome/renderer/chrome_render_thread_observer.cc b/chrome/renderer/chrome_render_thread_observer.cc
@@ -39,6 +39,7 @@
 #include "chrome/renderer/security_filter_peer.h"
 #include "components/visitedlink/renderer/visitedlink_slave.h"
 #include "content/public/child/resource_dispatcher_delegate.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
 #include "content/public/renderer/render_view_visitor.h"
@@ -251,7 +252,8 @@ ChromeRenderThreadObserver::ChromeRenderThreadObserver()
   media::SetLocalizedStringProvider(
       chrome_common_media::LocalizedStringProvider);
 
-  field_trial_syncer_.InitFieldTrialObserving(command_line);
+  field_trial_syncer_.InitFieldTrialObserving(command_line,
+                                              switches::kSingleProcess);
 
   // chrome-native: is a scheme used for placeholder navigations that allow
   // UIs to be drawn with platform native widgets instead of HTML.  These pages

diff --git a/chrome/renderer/chrome_render_thread_observer.h b/chrome/renderer/chrome_render_thread_observer.h
@@ -12,17 +12,13 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/field_trial.h"
-#include "chrome/common/variations/child_process_field_trial_syncer.h"
 #include "components/content_settings/core/common/content_settings.h"
+#include "components/variations/child_process_field_trial_syncer.h"
 #include "content/public/renderer/render_thread_observer.h"
 
 class GURL;
 struct ContentSettings;
 
-namespace base {
-class CommandLine;
-}
-
 namespace content {
 class ResourceDispatcherDelegate;
 }
@@ -52,10 +48,6 @@ class ChromeRenderThreadObserver : public content::RenderThreadObserver,
   }
 
  private:
-  // Initializes field trial state change observation and notifies the browser
-  // of any field trials that might have already been activated.
-  void InitFieldTrialObserving(const base::CommandLine& command_line);
-
   // content::RenderThreadObserver:
   bool OnControlMessageReceived(const IPC::Message& message) override;
   void OnRenderProcessShutdown() override;
@@ -75,7 +67,7 @@ class ChromeRenderThreadObserver : public content::RenderThreadObserver,
   static bool is_incognito_process_;
   std::unique_ptr<content::ResourceDispatcherDelegate> resource_delegate_;
   RendererContentSettingRules content_setting_rules_;
-  chrome_variations::ChildProcessFieldTrialSyncer field_trial_syncer_;
+  variations::ChildProcessFieldTrialSyncer field_trial_syncer_;
 
   std::unique_ptr<visitedlink::VisitedLinkSlave> visited_link_slave_;
 

diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
@@ -3358,7 +3358,6 @@ test("unit_tests") {
     "../common/search/search_urls_unittest.cc",
     "../common/secure_origin_whitelist_unittest.cc",
     "../common/switch_utils_unittest.cc",
-    "../common/variations/child_process_field_trial_syncer_unittest.cc",
     "../renderer/app_categorizer_unittest.cc",
     "../renderer/chrome_content_renderer_client_unittest.cc",
     "../renderer/content_settings_observer_unittest.cc",

diff --git a/components/variations/BUILD.gn b/components/variations/BUILD.gn
@@ -18,6 +18,8 @@ static_library("variations") {
     "android/variations_seed_bridge.h",
     "caching_permuted_entropy_provider.cc",
     "caching_permuted_entropy_provider.h",
+    "child_process_field_trial_syncer.cc",
+    "child_process_field_trial_syncer.h",
     "entropy_provider.cc",
     "entropy_provider.h",
     "experiment_labels.cc",
@@ -96,6 +98,7 @@ source_set("unit_tests") {
   sources = [
     "active_field_trials_unittest.cc",
     "caching_permuted_entropy_provider_unittest.cc",
+    "child_process_field_trial_syncer_unittest.cc",
     "entropy_provider_unittest.cc",
     "experiment_labels_unittest.cc",
     "metrics_util_unittest.cc",

diff --git a/...tions/child_process_field_trial_syncer.cc → ...tions/child_process_field_trial_syncer.cc b/...tions/child_process_field_trial_syncer.cc → ...tions/child_process_field_trial_syncer.cc
@@ -2,17 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/common/variations/child_process_field_trial_syncer.h"
+#include "components/variations/child_process_field_trial_syncer.h"
 
 #include <set>
 #include <utility>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "components/variations/variations_util.h"
-#include "content/public/common/content_switches.h"
 
-namespace chrome_variations {
+namespace variations {
 
 ChildProcessFieldTrialSyncer::ChildProcessFieldTrialSyncer(
     base::FieldTrialList::Observer* observer)
@@ -21,14 +20,15 @@ ChildProcessFieldTrialSyncer::ChildProcessFieldTrialSyncer(
 ChildProcessFieldTrialSyncer::~ChildProcessFieldTrialSyncer() {}
 
 void ChildProcessFieldTrialSyncer::InitFieldTrialObserving(
-    const base::CommandLine& command_line) {
+    const base::CommandLine& command_line,
+    const char* single_process_switch_name) {
   // In single-process mode, there is no need to synchronize trials to the
   // browser process (because it's the same process), so this class is a no-op.
-  if (command_line.HasSwitch(switches::kSingleProcess))
+  if (command_line.HasSwitch(single_process_switch_name))
     return;
 
   // Set up initial set of crash dump data for field trials in this process.
-  variations::SetVariationListCrashKeys();
+  SetVariationListCrashKeys();
 
   // Listen for field trial activations to report them to the browser.
   base::FieldTrialList::AddObserver(observer_);
@@ -60,7 +60,7 @@ void ChildProcessFieldTrialSyncer::OnSetFieldTrialGroup(
   // Ensure the trial is marked as "used" by calling group() on it if it is
   // marked as activated.
   trial->group();
-  variations::SetVariationListCrashKeys();
+  SetVariationListCrashKeys();
 }
 
-}  // namespace chrome_variations
+}  // namespace variations
diff --git a/...ations/child_process_field_trial_syncer.h → ...ations/child_process_field_trial_syncer.h b/...ations/child_process_field_trial_syncer.h → ...ations/child_process_field_trial_syncer.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_COMMON_VARIATIONS_CHILD_PROCESS_FIELD_TRIAL_SYNCER_H_
-#define CHROME_COMMON_VARIATIONS_CHILD_PROCESS_FIELD_TRIAL_SYNCER_H_
+#ifndef COMPONENTS_VARIATIONS_FIELD_TRIAL_SYNCER_H_
+#define COMPONENTS_VARIATIONS_FIELD_TRIAL_SYNCER_H_
 
 #include <string>
 
@@ -14,7 +14,7 @@ namespace base {
 class CommandLine;
 }
 
-namespace chrome_variations {
+namespace variations {
 
 // ChildProcessFieldTrialSyncer is a utility class that's responsible for
 // syncing the "activated" state of field trials between browser and child
@@ -31,8 +31,11 @@ class ChildProcessFieldTrialSyncer {
   ~ChildProcessFieldTrialSyncer();
 
   // Initializes field trial state change observation and notifies the browser
-  // of any field trials that might have already been activated.
-  void InitFieldTrialObserving(const base::CommandLine& command_line);
+  // of any field trials that might have already been activated. Takes the
+  // switch name for single process mode as a parameter, since that's not
+  // visible to the component.
+  void InitFieldTrialObserving(const base::CommandLine& command_line,
+                               const char* single_process_switch_name);
 
   // Handler for messages from the browser process notifying the child process
   // that a field trial was activated.
@@ -45,6 +48,6 @@ class ChildProcessFieldTrialSyncer {
   DISALLOW_COPY_AND_ASSIGN(ChildProcessFieldTrialSyncer);
 };
 
-}  // namespace chrome_variations
+}  // namespace variations
 
-#endif  // CHROME_COMMON_VARIATIONS_CHILD_PROCESS_FIELD_TRIAL_SYNCER_H_
+#endif  // COMPONENTS_VARIATIONS_FIELD_TRIAL_SYNCER_H_
diff --git a/...ld_process_field_trial_syncer_unittest.cc → ...ld_process_field_trial_syncer_unittest.cc b/...ld_process_field_trial_syncer_unittest.cc → ...ld_process_field_trial_syncer_unittest.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/common/variations/child_process_field_trial_syncer.h"
+#include "components/variations/child_process_field_trial_syncer.h"
 
 #include <string>
 #include <utility>
@@ -15,7 +15,7 @@
 #include "base/run_loop.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace chrome_variations {
+namespace variations {
 
 namespace {
 
@@ -55,7 +55,7 @@ TEST(ChildProcessFieldTrialSyncerTest, FieldTrialState) {
   base::MessageLoop message_loop;
   base::FieldTrialList field_trial_list(nullptr);
   base::FieldTrialList::CreateTrialsFromCommandLine(
-     *base::CommandLine::ForCurrentProcess(), "field_trial_handle_switch");
+      *base::CommandLine::ForCurrentProcess(), "field_trial_handle_switch");
 
   base::FieldTrial* trial1 = base::FieldTrialList::CreateFieldTrial("A", "G1");
   base::FieldTrial* trial2 = base::FieldTrialList::CreateFieldTrial("B", "G2");
@@ -75,7 +75,8 @@ TEST(ChildProcessFieldTrialSyncerTest, FieldTrialState) {
 
   TestFieldTrialObserver observer;
   ChildProcessFieldTrialSyncer syncer(&observer);
-  syncer.InitFieldTrialObserving(*base::CommandLine::ForCurrentProcess());
+  syncer.InitFieldTrialObserving(*base::CommandLine::ForCurrentProcess(),
+                                 "single_process");
 
   // The observer should be notified of activated entries that were not activate
   // on the command line. In this case, trial 2. (Trial 1 was already active via
@@ -93,4 +94,4 @@ TEST(ChildProcessFieldTrialSyncerTest, FieldTrialState) {
   EXPECT_EQ(MakeStringPair("C", "G3"), observer.get_observed_entry(1));
 }
 
-}  // namespace chrome_variations
+}  // namespace variations
diff --git a/content/browser/ppapi_plugin_process_host.cc b/content/browser/ppapi_plugin_process_host.cc
@@ -377,6 +377,7 @@ bool PpapiPluginProcessHost::Init(const PepperPluginInfo& info) {
   cmd_line->AppendSwitchASCII(switches::kProcessType,
                               is_broker_ ? switches::kPpapiBrokerProcess
                                          : switches::kPpapiPluginProcess);
+  BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(cmd_line);
 
 #if defined(OS_WIN)
   cmd_line->AppendArg(is_broker_ ? switches::kPrefetchArgumentPpapiBroker
@@ -402,7 +403,7 @@ bool PpapiPluginProcessHost::Init(const PepperPluginInfo& info) {
     cmd_line->CopySwitchesFrom(browser_command_line, kPluginForwardSwitches,
                                arraysize(kPluginForwardSwitches));
 
-    // Copy any flash args over and introduce field trials if necessary.
+    // Copy any flash args over if necessary.
     // TODO(vtl): Stop passing flash args in the command line, or windows is
     // going to explode.
     std::string existing_args =
@@ -479,6 +480,8 @@ bool PpapiPluginProcessHost::OnMessageReceived(const IPC::Message& msg) {
   IPC_BEGIN_MESSAGE_MAP(PpapiPluginProcessHost, msg)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_ChannelCreated,
                         OnRendererPluginChannelCreated)
+    IPC_MESSAGE_HANDLER(PpapiHostMsg_FieldTrialActivated,
+                        OnFieldTrialActivated);
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   DCHECK(handled);
@@ -498,6 +501,14 @@ void PpapiPluginProcessHost::OnChannelConnected(int32_t peer_pid) {
   pending_requests_.clear();
 }
 
+void PpapiPluginProcessHost::OnFieldTrialActivated(
+    const std::string& trial_name) {
+  // Activate the trial in the browser process to match its state in the
+  // PPAPI process. This is done by calling FindFullName which finalizes the
+  // group and activates the trial.
+  base::FieldTrialList::FindFullName(trial_name);
+}
+
 // Called when the browser <--> plugin channel has an error. This normally
 // means the plugin has crashed.
 void PpapiPluginProcessHost::OnChannelError() {

diff --git a/content/browser/ppapi_plugin_process_host.h b/content/browser/ppapi_plugin_process_host.h
@@ -9,6 +9,7 @@
 
 #include <memory>
 #include <queue>
+#include <string>
 #include <vector>
 
 #include "base/files/file_path.h"
@@ -151,6 +152,7 @@ class PpapiPluginProcessHost : public BrowserChildProcessHostDelegate,
 
   // IPC message handlers.
   void OnRendererPluginChannelCreated(const IPC::ChannelHandle& handle);
+  void OnFieldTrialActivated(const std::string& trial_name);
 
   // Handles most requests from the plugin. May be NULL.
   scoped_refptr<PepperMessageFilter> filter_;

diff --git a/content/ppapi_plugin/BUILD.gn b/content/ppapi_plugin/BUILD.gn
@@ -46,6 +46,7 @@ source_set("ppapi_plugin_sources") {
   deps = [
     "//base",
     "//components/discardable_memory/client",
+    "//components/variations",
     "//content:export",
     "//content/child",
     "//content/public/child:child_sources",

diff --git a/content/ppapi_plugin/DEPS b/content/ppapi_plugin/DEPS
@@ -1,5 +1,6 @@
 include_rules = [
   "+components/discardable_memory/client",
+  "+components/variations",
   "+content/child",
   "+gin/public/isolate_holder.h",
   "+gin/v8_initializer.h",

diff --git a/content/ppapi_plugin/ppapi_thread.cc b/content/ppapi_plugin/ppapi_thread.cc
@@ -111,7 +111,8 @@ PpapiThread::PpapiThread(const base::CommandLine& command_line, bool is_broker)
       plugin_globals_(GetIOTaskRunner()),
       connect_instance_func_(NULL),
       local_pp_module_(base::RandInt(0, std::numeric_limits<PP_Module>::max())),
-      next_plugin_dispatcher_id_(1) {
+      next_plugin_dispatcher_id_(1),
+      field_trial_syncer_(this) {
   plugin_globals_.SetPluginProxyDelegate(this);
   plugin_globals_.set_command_line(
       command_line.GetSwitchValueASCII(switches::kPpapiFlashArgs));
@@ -133,6 +134,8 @@ PpapiThread::PpapiThread(const base::CommandLine& command_line, bool is_broker)
     base::DiscardableMemoryAllocator::SetInstance(
         ChildThreadImpl::discardable_shared_memory_manager());
   }
+  field_trial_syncer_.InitFieldTrialObserving(command_line,
+                                              switches::kSingleProcess);
 }
 
 PpapiThread::~PpapiThread() {
@@ -483,6 +486,12 @@ void PpapiThread::OnHang() {
     base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1));
 }
 
+void PpapiThread::OnFieldTrialGroupFinalized(const std::string& trial_name,
+                                             const std::string& group_name) {
+  // IPC to the browser process to tell it the specified trial was activated.
+  Send(new PpapiHostMsg_FieldTrialActivated(trial_name));
+}
+
 bool PpapiThread::SetupChannel(base::ProcessId renderer_pid,
                                int renderer_child_id,
                                bool incognito,