Fix race condition in heap profiler that could affect tracing config.

There was a race condition that could cause the vm region map in a memlog trace
to contain full file paths. This happened if the CoordinatorImpl callback occurred before the
TracingController was able to set the TracingConfig on TraceLog.

Change-Id: If3b424ed13b91a0bd644316fe1a7b30a6254b191
Reviewed-on: https://chromium-review.googlesource.com/1072312Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562551}

android_webview/browser/tracing/aw_trace_event_args_whitelist.cc

@@ -18,12 +18,17 @@ struct WhitelistEntry {
   const char* const* arg_name_filter;
 };
 
+const char* const kMemoryDumpAllowedArgs[] = {"dumps", nullptr};
+
 const WhitelistEntry kEventArgsWhitelist[] = {
     {"__metadata", "thread_name", nullptr},
     {"__metadata", "process_name", nullptr},
     {"__metadata", "process_uptime_seconds", nullptr},
     {"__metadata", "stackFrames", nullptr},
     {"__metadata", "typeNames", nullptr},
+    // Redefined the string since MemoryDumpManager::kTraceCategory causes
+    // static initialization of this struct.
+    {TRACE_DISABLED_BY_DEFAULT("memory-infra"), "*", kMemoryDumpAllowedArgs},
     {nullptr, nullptr, nullptr}};
 
 }  // namespace

components/heap_profiling/supervisor.cc

@@ -151,16 +151,25 @@ void Supervisor::RequestTraceWithHeapDump(TraceFinishedCallback callback,
       },
       std::move(callback));
 
-  memory_instrumentation::MemoryInstrumentation::GetInstance()
-      ->RequestGlobalDumpAndAppendToTrace(
-          base::trace_event::MemoryDumpType::EXPLICITLY_TRIGGERED,
-          base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND,
-          base::AdaptCallbackForRepeating(std::move(finished_dump_callback)));
+  auto trigger_memory_dump_callback = base::BindOnce(
+      [](base::OnceCallback<void(bool success, uint64_t dump_guid)>
+             finished_dump_callback) {
+        memory_instrumentation::MemoryInstrumentation::GetInstance()
+            ->RequestGlobalDumpAndAppendToTrace(
+                base::trace_event::MemoryDumpType::EXPLICITLY_TRIGGERED,
+                base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND,
+                base::AdaptCallbackForRepeating(
+                    std::move(finished_dump_callback)));
+      },
+      std::move(finished_dump_callback));
 
   // The only reason this should return false is if tracing is already enabled,
   // which we've already checked.
+  // Use AdaptCallbackForRepeating since the argument passed to StartTracing()
+  // is intended to be a OnceCallback, but the code has not yet been migrated.
   bool result = content::TracingController::GetInstance()->StartTracing(
-      GetBackgroundTracingConfig(anonymize), base::Closure());
+      GetBackgroundTracingConfig(anonymize),
+      base::AdaptCallbackForRepeating(std::move(trigger_memory_dump_callback)));
   DCHECK(result);
 }
 

components/heap_profiling/test_driver.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/files/file_path.h"
 #include "base/json/json_reader.h"
 #include "base/process/process_handle.h"
 #include "base/run_loop.h"
@@ -518,17 +519,34 @@ bool ValidateProcessMmaps(base::Value* process_mmaps,
                           bool should_have_contents) {
   base::Value* vm_regions = process_mmaps->FindKey("vm_regions");
   size_t count = vm_regions->GetList().size();
-  if (should_have_contents) {
-    if (count == 0) {
-      LOG(ERROR) << "vm_regions should have contents, but doesn't";
-      return false;
-    }
-  } else {
+  if (!should_have_contents) {
     if (count != 0) {
       LOG(ERROR) << "vm_regions should be empty, but has contents";
       return false;
     }
+    return true;
+  }
+
+  if (count == 0) {
+    LOG(ERROR) << "vm_regions should have contents, but doesn't";
+    return false;
+  }
+
+  // File paths may contain PII. Make sure that "mf" entries only contain the
+  // basename, rather than a full path.
+  for (const base::Value& vm_region : vm_regions->GetList()) {
+    const base::Value* file_path_value = vm_region.FindKey("mf");
+    if (file_path_value) {
+      std::string file_path = file_path_value->GetString();
+
+      base::FilePath::StringType path(file_path.begin(), file_path.end());
+      if (base::FilePath(path).BaseName().AsUTF8Unsafe() != file_path) {
+        LOG(ERROR) << "vm_region should not contain file path: " << file_path;
+        return false;
+      }
+    }
   }
+
   return true;
 }
 
@@ -830,7 +848,7 @@ void TestDriver::CollectResults(bool synchronous) {
   Supervisor::GetInstance()->RequestTraceWithHeapDump(
       base::Bind(&TestDriver::TraceFinished, base::Unretained(this),
                  std::move(finish_tracing_closure)),
-      false /* strip_path_from_mapped_files */);
+      /* anonymize= */ true);
 
   if (synchronous)
     run_loop->Run();
@@ -1029,6 +1047,11 @@ void TestDriver::WaitForProfilingToStartForAllRenderersUIThreadCallback(
     wait_for_ui_thread_.Signal();
     return;
   }
+
+  // Brief sleep to prevent spamming the task queue, since this code is called
+  // in a tight loop.
+  base::PlatformThread::Sleep(base::TimeDelta::FromMicroseconds(100));
+
   WaitForProfilingToStartForAllRenderersUIThreadAndSignal();
 }