Hooked the trace event argument whitelist up to the background_trace_manager

content/browser/tracing/background_tracing_manager_browsertest.cc

Index: content/browser/tracing/background_tracing_manager_browsertest.cc
diff --git a/content/browser/tracing/background_tracing_manager_browsertest.cc b/content/browser/tracing/background_tracing_manager_browsertest.cc
index f353264f911aa59fb7b36d62422afce812058982..bc3bfb56ff136efa1d3a6bb54b1270c2281c4dea 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,43 @@ 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 output_buffer_length = 10 * 1024 * 1024;
+    std::vector<char> output_str(output_buffer_length);
+
+    z_stream stream = {0};
+    stream.total_in = stream.avail_in = compressed_length;
+    stream.total_out = stream.avail_out = output_buffer_length;

[davidben, 2015/06/02 22:52:02]

Are you supposed to be setting total_in and total_out? This says:

The fields total_in and total_out can be used for statistics or progress
reports. After compression, total_in holds the total size of the uncompressed
data and may be saved for use in the decompressor (particularly if the
decompressor wants to decompress everything in a single step).

net/filter/gzip_filter.cc doesn't set it. And poking around codesearch, I think
zlib sets it for you:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/zlib/i...

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

Done.

+    stream.next_in = (Bytef*)&file_contents->data()[0];
+    stream.next_out = (Bytef*)&output_str[0];

[davidben, 2015/06/02 22:52:02]

You can use vector_as_array from base/stl_util.h

+
+    stream.zalloc = Z_NULL;
+    stream.zfree = Z_NULL;
+    stream.opaque = Z_NULL;

[davidben, 2015/06/02 22:52:01]

You're already initializing it with zeros, right?

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

Removed.

+
+    int result = inflateInit2(&stream, 16 + MAX_WBITS);

[davidben, 2015/06/02 22:52:02]

Where does 16 + MAX_WBITS come from?

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

zlib.h:771: (inflateInit2 docs)

"The windowBits parameter is the base two logarithm of the maximum window size
(the size of the history buffer)."

(...)

"windowBits can also be greater than 15 for optional gzip decoding.  Add
   32 to windowBits to enable zlib and gzip decoding with automatic header
   detection, or add 16 to decode only the gzip format (the zlib format will
   return a Z_DATA_ERROR).  If a gzip stream is being decoded, strm->adler is a
   crc32 instead of an adler32."

Added a comment to explain this.

+    EXPECT_EQ(Z_OK, result);
+    result = inflate(&stream, Z_FINISH);
+    bool success = (result != Z_STREAM_ERROR);

[davidben, 2015/06/02 22:52:01]

result == Z_STREAM_END? It seems there are other possible error codes like
Z_BUF_ERROR. Or better:
EXPECT_EQ(Z_STREAM_END, result);

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

Done.

+    inflateEnd(&stream);
+    EXPECT_TRUE(success);
+
+    last_file_contents_ = &output_str[0];

[davidben, 2015/06/02 22:52:02]

This is rather confusing since you're writing a pointer into a std::string.
Switch it to last_file_contents_.assign?

It's also assuming that inflate writes a NUL-terminated string, and I would
think it doesn't. (I think you're actually getting the NUL terminator from
initializing the buffer to zeros.) I think you actually want:

  last_file_contents_.assign(vector_as_array(&output_str), bytes_written);

where bytes_written is the number of bytes that inflate actually wrote. I'm
having a hard time understanding inflate's API, but going by gzip_filter.cc, it
seems you're supposed to do this:

https://code.google.com/p/chromium/codesearch#chromium/src/net/filter/gzip_fi...

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

Ah good catch; yeah you're right, it was indeed relying on the vector
initialization. Done!

     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 +79,7 @@ class BackgroundTracingManagerUploadConfigWrapper {
   BackgroundTracingManager::ReceiveCallback receive_callback_;
   base::Closure callback_;
   int receive_count_;
+  std::string last_file_contents_;
 };
 
 void StartedFinalizingCallback(base::Closure callback,
@@ -81,7 +111,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.
@@ -101,7 +132,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
         BackgroundTracingManager::GetInstance()->RegisterTriggerType("test");
 
     BackgroundTracingManager::GetInstance()->SetActiveScenario(
-        config.Pass(), upload_config_wrapper.get_receive_callback(), true);

[davidben, 2015/06/02 22:52:02]

Why does both true and false (here and DisableScenarioWhenIdle) convert to
NO_DATA_FILTERING?

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

The parameter didn't actually do anything before this CL, and this test in
particular shouldn't test any filtering.

+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
 
     BackgroundTracingManager::GetInstance()->WhenIdle(
         base::Bind(&DisableScenarioWhenIdle));
@@ -133,7 +165,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
             "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));
@@ -149,6 +182,113 @@ 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) {
+  {

[davidben, 2015/06/02 22:52:02]

Why an extra level of scope here?

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

Whoops, copy and paste artifact from the existing tests :/. Removed, sorry.

+    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(
+            "test");
+
+    base::RunLoop wait_for_activated;
+    BackgroundTracingManager::GetInstance()
+        ->SetTracingEnabledCallbackForTesting(wait_for_activated.QuitClosure());
+    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) {
+  {

[davidben, 2015/06/02 22:52:01]

Ditto.

[oystein (OOO til 10th of July), 2015/06/03 00:14:43]

Done.

+    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(
+            "test");
+
+    BackgroundTracingManager::GetInstance()->SetActiveScenario(
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::ANONYMIZE_DATA);
+
+    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) {
@@ -177,7 +317,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));
@@ -239,7 +380,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));
@@ -275,7 +417,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));
@@ -310,7 +453,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);
   }
@@ -329,7 +473,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
         new BackgroundTracingReactiveConfig());
 
     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);
   }