Hooked the trace event argument whitelist up to the BackgroundTracingManager

R=dsinclair,nduca,davidben,shatch
BUG=466769

Review URL: https://codereview.chromium.org/1148633007

Cr-Commit-Position: refs/heads/master@{#333214}

6 files changed, 186 insertions, 24 deletions

diff --git a/base/trace_event/trace_config.h b/base/trace_event/trace_config.h
index fe23c17..a9f8306 100644
--- a/base/trace_event/trace_config.h
+++ b/base/trace_event/trace_config.h
@@ -122,6 +122,7 @@ class BASE_EXPORT TraceConfig {
   void SetTraceRecordMode(TraceRecordMode mode) { record_mode_ = mode; }
   void EnableSampling() { enable_sampling_ = true; }
   void EnableSystrace() { enable_systrace_ = true; }
+  void EnableArgumentFilter() { enable_argument_filter_ = true; }
 
   // Writes the string representation of the TraceConfig. The string is JSON
   // formatted.
diff --git a/chrome/browser/tracing/background_tracing_field_trial.cc b/chrome/browser/tracing/background_tracing_field_trial.cc
index c509d68..8d459ba 100644
--- a/chrome/browser/tracing/background_tracing_field_trial.cc
+++ b/chrome/browser/tracing/background_tracing_field_trial.cc
@@ -34,7 +34,7 @@ void OnUploadComplete(TraceCrashServiceUploader* uploader,
   done_callback.Run();
 }
 
-void UploadCallback(const base::RefCountedString* file_contents,
+void UploadCallback(const scoped_refptr<base::RefCountedString>& file_contents,
                     base::Closure callback) {
   TraceCrashServiceUploader* uploader = new TraceCrashServiceUploader(
       g_browser_process->system_request_context());
@@ -66,7 +66,8 @@ void SetupBackgroundTracingFieldTrial() {
     return;
 
   content::BackgroundTracingManager::GetInstance()->SetActiveScenario(
-      config.Pass(), base::Bind(&UploadCallback), true);
+      config.Pass(), base::Bind(&UploadCallback),
+      content::BackgroundTracingManager::ANONYMIZE_DATA);
 }
 
 }  // namespace tracing
diff --git a/content/browser/tracing/background_tracing_manager_browsertest.cc b/content/browser/tracing/background_tracing_manager_browsertest.cc
index 54a1127..13a309e 100644
--- a/content/browser/tracing/background_tracing_manager_browsertest.cc
+++ b/content/browser/tracing/background_tracing_manager_browsertest.cc
@@ -3,12 +3,14 @@
 // found in the LICENSE file.
 
 #include "base/bind.h"
+#include "base/trace_event/trace_event.h"
 #include "content/public/browser/background_tracing_manager.h"
 #include "content/public/browser/background_tracing_preemptive_config.h"
 #include "content/public/browser/background_tracing_reactive_config.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "third_party/zlib/zlib.h"
 
 namespace content {
 
@@ -29,16 +31,42 @@ class BackgroundTracingManagerUploadConfigWrapper {
                    base::Unretained(this));
   }
 
-  void Upload(const base::RefCountedString* file_contents,
+  void Upload(const scoped_refptr<base::RefCountedString>& file_contents,
               base::Callback<void()> done_callback) {
     receive_count_ += 1;
+    EXPECT_TRUE(file_contents);
 
+    size_t compressed_length = file_contents->data().length();
+    const size_t kOutputBufferLength = 10 * 1024 * 1024;
+    std::vector<char> output_str(kOutputBufferLength);
+
+    z_stream stream = {0};
+    stream.avail_in = compressed_length;
+    stream.avail_out = kOutputBufferLength;
+    stream.next_in = (Bytef*)&file_contents->data()[0];
+    stream.next_out = (Bytef*)vector_as_array(&output_str);
+
+    // 16 + MAX_WBITS means only decoding gzip encoded streams, and using
+    // the biggest window size, according to zlib.h
+    int result = inflateInit2(&stream, 16 + MAX_WBITS);
+    EXPECT_EQ(Z_OK, result);
+    result = inflate(&stream, Z_FINISH);
+    int bytes_written = kOutputBufferLength - stream.avail_out;
+
+    inflateEnd(&stream);
+    EXPECT_EQ(Z_STREAM_END, result);
+
+    last_file_contents_.assign(vector_as_array(&output_str), bytes_written);
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             base::Bind(done_callback));
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             base::Bind(callback_));
   }
 
+  bool TraceHasMatchingString(const char* str) {
+    return last_file_contents_.find(str) != std::string::npos;
+  }
+
   int get_receive_count() const { return receive_count_; }
 
   const BackgroundTracingManager::ReceiveCallback& get_receive_callback()
@@ -50,6 +78,7 @@ class BackgroundTracingManagerUploadConfigWrapper {
   BackgroundTracingManager::ReceiveCallback receive_callback_;
   base::Closure callback_;
   int receive_count_;
+  std::string last_file_contents_;
 };
 
 void StartedFinalizingCallback(base::Closure callback,
@@ -97,7 +126,8 @@ void SetupBackgroundTracingManager() {
 
 void DisableScenarioWhenIdle() {
   BackgroundTracingManager::GetInstance()->SetActiveScenario(
-      NULL, BackgroundTracingManager::ReceiveCallback(), false);
+      NULL, BackgroundTracingManager::ReceiveCallback(),
+      BackgroundTracingManager::NO_DATA_FILTERING);
 }
 
 // This tests that the endpoint receives the final trace data.
@@ -118,7 +148,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
             GetInstance()->RegisterTriggerType("preemptive_test");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -150,7 +181,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
             "preemptive_test");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -166,6 +198,112 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
   }
 }
 
+namespace {
+
+bool IsTraceEventArgsWhitelisted(const char* category_group_name,
+                                 const char* event_name) {
+  if (MatchPattern(category_group_name, "benchmark") &&
+      MatchPattern(event_name, "whitelisted")) {
+    return true;
+  }
+
+  return false;
+}
+
+}  // namespace
+
+// This tests that non-whitelisted args get stripped if required.
+IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
+                       NoWhitelistedArgsStripped) {
+  SetupBackgroundTracingManager();
+
+  base::trace_event::TraceLog::GetInstance()->SetArgumentFilterPredicate(
+      base::Bind(&IsTraceEventArgsWhitelisted));
+
+  base::RunLoop wait_for_upload;
+  BackgroundTracingManagerUploadConfigWrapper upload_config_wrapper(
+      wait_for_upload.QuitClosure());
+
+  scoped_ptr<BackgroundTracingPreemptiveConfig> config =
+      CreatePreemptiveConfig();
+
+  content::BackgroundTracingManager::TriggerHandle handle =
+      content::BackgroundTracingManager::GetInstance()->RegisterTriggerType(
+          "preemptive_test");
+
+  base::RunLoop wait_for_activated;
+  BackgroundTracingManager::GetInstance()->SetTracingEnabledCallbackForTesting(
+      wait_for_activated.QuitClosure());
+  EXPECT_TRUE(BackgroundTracingManager::GetInstance()->SetActiveScenario(
+      config.Pass(), upload_config_wrapper.get_receive_callback(),
+      BackgroundTracingManager::ANONYMIZE_DATA));
+
+  wait_for_activated.Run();
+
+  TRACE_EVENT1("benchmark", "whitelisted", "find_this", 1);
+  TRACE_EVENT1("benchmark", "not_whitelisted", "this_not_found", 1);
+
+  BackgroundTracingManager::GetInstance()->WhenIdle(
+      base::Bind(&DisableScenarioWhenIdle));
+
+  BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
+      handle, base::Bind(&StartedFinalizingCallback, base::Closure(), true));
+
+  wait_for_upload.Run();
+
+  EXPECT_TRUE(upload_config_wrapper.get_receive_count() == 1);
+  EXPECT_TRUE(upload_config_wrapper.TraceHasMatchingString("{"));
+  EXPECT_TRUE(upload_config_wrapper.TraceHasMatchingString("find_this"));
+  EXPECT_TRUE(!upload_config_wrapper.TraceHasMatchingString("this_not_found"));
+}
+
+// This tests subprocesses (like a navigating renderer) which gets told to
+// provide a argument-filtered trace and has no predicate in place to do the
+// filtering (in this case, only the browser process gets it set), will crash
+// rather than return potential PII.
+IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
+                       CrashWhenSubprocessWithoutArgumentFilter) {
+  SetupBackgroundTracingManager();
+
+  base::trace_event::TraceLog::GetInstance()->SetArgumentFilterPredicate(
+      base::Bind(&IsTraceEventArgsWhitelisted));
+
+  base::RunLoop wait_for_upload;
+  BackgroundTracingManagerUploadConfigWrapper upload_config_wrapper(
+      wait_for_upload.QuitClosure());
+
+  scoped_ptr<BackgroundTracingPreemptiveConfig> config =
+      CreatePreemptiveConfig();
+
+  content::BackgroundTracingManager::TriggerHandle handle =
+      content::BackgroundTracingManager::GetInstance()->RegisterTriggerType(
+          "preemptive_test");
+
+  base::RunLoop wait_for_activated;
+  BackgroundTracingManager::GetInstance()->SetTracingEnabledCallbackForTesting(
+      wait_for_activated.QuitClosure());
+  EXPECT_TRUE(BackgroundTracingManager::GetInstance()->SetActiveScenario(
+      config.Pass(), upload_config_wrapper.get_receive_callback(),
+      BackgroundTracingManager::ANONYMIZE_DATA));
+
+  wait_for_activated.Run();
+
+  NavigateToURL(shell(), GetTestUrl("", "about:blank"));
+
+  BackgroundTracingManager::GetInstance()->WhenIdle(
+      base::Bind(&DisableScenarioWhenIdle));
+
+  BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
+      handle, base::Bind(&StartedFinalizingCallback, base::Closure(), true));
+
+  wait_for_upload.Run();
+
+  EXPECT_TRUE(upload_config_wrapper.get_receive_count() == 1);
+  // We should *not* receive anything at all from the renderer,
+  // the process should've crashed rather than letting that happen.
+  EXPECT_TRUE(!upload_config_wrapper.TraceHasMatchingString("CrRendererMain"));
+}
+
 // This tests multiple triggers still only gathers once.
 IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
                        CallMultipleTriggersOnlyGatherOnce) {
@@ -194,7 +332,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
         BackgroundTracingManager::GetInstance()->RegisterTriggerType("test2");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -256,7 +395,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
             "does_not_exist");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -292,7 +432,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
         ->InvalidateTriggerHandlesForTesting();
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -327,7 +468,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
     config->configs.push_back(rule);
 
     bool result = BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     EXPECT_FALSE(result);
   }
@@ -351,7 +493,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
             GetInstance()->RegisterTriggerType("reactive_test");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -385,7 +528,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
             GetInstance()->RegisterTriggerType("reactive_test");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -420,7 +564,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
             GetInstance()->RegisterTriggerType("reactive_test");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
diff --git a/content/browser/tracing/background_tracing_manager_impl.cc b/content/browser/tracing/background_tracing_manager_impl.cc
index b636cc8..14f07cf 100644
--- a/content/browser/tracing/background_tracing_manager_impl.cc
+++ b/content/browser/tracing/background_tracing_manager_impl.cc
@@ -130,7 +130,7 @@ bool BackgroundTracingManagerImpl::IsSupportedConfig(
 bool BackgroundTracingManagerImpl::SetActiveScenario(
     scoped_ptr<BackgroundTracingConfig> config,
     const BackgroundTracingManager::ReceiveCallback& receive_callback,
-    bool requires_anonymized_data) {
+    DataFiltering data_filtering) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (is_tracing_)
     return false;
@@ -144,7 +144,7 @@ bool BackgroundTracingManagerImpl::SetActiveScenario(
 
   config_ = config.Pass();
   receive_callback_ = receive_callback;
-  requires_anonymized_data_ = requires_anonymized_data;
+  requires_anonymized_data_ = (data_filtering == ANONYMIZE_DATA);
 
   EnableRecordingIfConfigNeedsIt();
 
@@ -296,6 +296,11 @@ void BackgroundTracingManagerImpl::InvalidateTriggerHandlesForTesting() {
   trigger_handles_.clear();
 }
 
+void BackgroundTracingManagerImpl::SetTracingEnabledCallbackForTesting(
+    const base::Closure& callback) {
+  tracing_enabled_callback_for_testing_ = callback;
+};
+
 void BackgroundTracingManagerImpl::FireTimerForTesting() {
   tracing_timer_->FireTimerForTesting();
 }
@@ -303,9 +308,12 @@ void BackgroundTracingManagerImpl::FireTimerForTesting() {
 void BackgroundTracingManagerImpl::EnableRecording(
     std::string category_filter_str,
     base::trace_event::TraceRecordMode record_mode) {
+  base::trace_event::TraceConfig trace_config(category_filter_str, record_mode);
+  if (requires_anonymized_data_)
+    trace_config.EnableArgumentFilter();
+
   is_tracing_ = TracingController::GetInstance()->EnableRecording(
-      base::trace_event::TraceConfig(category_filter_str, record_mode),
-      TracingController::EnableRecordingDoneCallback());
+      trace_config, tracing_enabled_callback_for_testing_);
 }
 
 void BackgroundTracingManagerImpl::OnFinalizeStarted(
@@ -314,7 +322,7 @@ void BackgroundTracingManagerImpl::OnFinalizeStarted(
 
   if (!receive_callback_.is_null())
     receive_callback_.Run(
-        file_contents.get(),
+        file_contents,
         base::Bind(&BackgroundTracingManagerImpl::OnFinalizeComplete,
                    base::Unretained(this)));
 }
diff --git a/content/browser/tracing/background_tracing_manager_impl.h b/content/browser/tracing/background_tracing_manager_impl.h
index 9795629..0ce73f9 100644
--- a/content/browser/tracing/background_tracing_manager_impl.h
+++ b/content/browser/tracing/background_tracing_manager_impl.h
@@ -21,7 +21,7 @@ class BackgroundTracingManagerImpl : public content::BackgroundTracingManager {
 
   bool SetActiveScenario(scoped_ptr<BackgroundTracingConfig>,
                          const ReceiveCallback&,
-                         bool) override;
+                         DataFiltering data_filtering) override;
   void WhenIdle(IdleCallback idle_callback) override;
 
   void TriggerNamedEvent(TriggerHandle, StartedFinalizingCallback) override;
@@ -29,7 +29,8 @@ class BackgroundTracingManagerImpl : public content::BackgroundTracingManager {
   void GetTriggerNameList(std::vector<std::string>* trigger_names) override;
 
   void InvalidateTriggerHandlesForTesting() override;
-
+  void SetTracingEnabledCallbackForTesting(
+      const base::Closure& callback) override;
   void FireTimerForTesting() override;
 
  private:
@@ -94,6 +95,7 @@ class BackgroundTracingManagerImpl : public content::BackgroundTracingManager {
   int trigger_handle_ids_;
 
   IdleCallback idle_callback_;
+  base::Closure tracing_enabled_callback_for_testing_;
 
   friend struct base::DefaultLazyInstanceTraits<BackgroundTracingManagerImpl>;
 
diff --git a/content/public/browser/background_tracing_manager.h b/content/public/browser/background_tracing_manager.h
index 87f3279..75e2d34 100644
--- a/content/public/browser/background_tracing_manager.h
+++ b/content/public/browser/background_tracing_manager.h
@@ -39,8 +39,8 @@ class BackgroundTracingManager {
   //     );
   // }
   //
-  typedef base::Callback<void(const base::RefCountedString*, base::Closure)>
-      ReceiveCallback;
+  typedef base::Callback<void(const scoped_refptr<base::RefCountedString>&,
+                              base::Closure)> ReceiveCallback;
 
   // Set the triggering rules for when to start recording.
   //
@@ -57,9 +57,13 @@ class BackgroundTracingManager {
   // Calls to SetActiveScenario() with a config will fail if tracing is
   // currently on. Use WhenIdle to register a callback to get notified when
   // the manager is idle and a config can be set again.
+  enum DataFiltering {
+    NO_DATA_FILTERING,
+    ANONYMIZE_DATA,
+  };
   virtual bool SetActiveScenario(scoped_ptr<BackgroundTracingConfig> config,
                                  const ReceiveCallback& receive_callback,
-                                 bool requires_anonymized_data) = 0;
+                                 DataFiltering data_filtering) = 0;
 
   // Notifies the caller when the manager is idle (not recording or uploading),
   // so that a call to SetActiveScenario() is likely to succeed.
@@ -84,7 +88,8 @@ class BackgroundTracingManager {
   virtual void GetTriggerNameList(std::vector<std::string>* trigger_names) = 0;
 
   virtual void InvalidateTriggerHandlesForTesting() = 0;
-
+  virtual void SetTracingEnabledCallbackForTesting(
+      const base::Closure& callback) = 0;
   virtual void FireTimerForTesting() = 0;
 
  protected: