[tracing] Auto disable failing memory dumpers (sandboxing workaround)

This CL introduces a workaround to deal with limitations that memory
dumpers face in child processes when sandboxed.
In the long term, this will be solved by doing the proper /proc/PID/*
fd propagation via IPC.
In the meanwhile this CL prevents invoking a dumper over and over
once it fails for the duration of the trace.

BUG=460884,461788

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

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

7 files changed, 90 insertions, 20 deletions

diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index c8be8f8..3430169 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -92,12 +92,28 @@ void MemoryDumpManager::BroadcastDumpRequest() {
 // Creates a dump point for the current process and appends it to the trace.
 void MemoryDumpManager::CreateLocalDumpPoint() {
   AutoLock lock(lock_);
+  bool did_any_provider_dump = false;
   scoped_ptr<ProcessMemoryDump> pmd(new ProcessMemoryDump());
 
-  for (MemoryDumpProvider* dump_provider : dump_providers_enabled_) {
-    dump_provider->DumpInto(pmd.get());
+  for (auto it = dump_providers_enabled_.begin();
+       it != dump_providers_enabled_.end();) {
+    MemoryDumpProvider* dump_provider = *it;
+    if (!dump_provider->DumpInto(pmd.get())) {
+      LOG(ERROR) << "The memory dumper " << dump_provider->GetFriendlyName()
+                 << " failed, possibly due to sandboxing (crbug.com/461788), "
+                    "disabling it for current process. Try restarting chrome "
+                    "with the --no-sandbox switch.";
+      it = dump_providers_enabled_.erase(it);
+    } else {
+      did_any_provider_dump = true;
+      ++it;
+    }
   }
 
+  // Don't create a dump point if all the dumpers failed.
+  if (!did_any_provider_dump)
+    return;
+
   scoped_refptr<TracedValue> value(new TracedValue());
   pmd->AsValueInto(value.get());
   // TODO(primiano): add the dump point to the trace at this point.
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 1ba73e6..78be377 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -10,6 +10,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::Return;
 
 namespace base {
 namespace trace_event {
@@ -48,7 +49,9 @@ class MemoryDumpManagerTest : public testing::Test {
 
 class MockDumpProvider : public MemoryDumpProvider {
  public:
-  MOCK_METHOD1(DumpInto, void(ProcessMemoryDump* pmd));
+  MOCK_METHOD1(DumpInto, bool(ProcessMemoryDump* pmd));
+
+  const char* GetFriendlyName() const override { return "MockDumpProvider"; }
 };
 
 TEST_F(MemoryDumpManagerTest, SingleDumper) {
@@ -64,7 +67,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   // Now repeat enabling the memory category and check that the dumper is
   // invoked this time.
   EnableTracing(kTraceCategory);
-  EXPECT_CALL(mdp, DumpInto(_)).Times(3);
+  EXPECT_CALL(mdp, DumpInto(_)).Times(3).WillRepeatedly(Return(true));
   for (int i = 0; i < 3; ++i)
     mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
   DisableTracing();
@@ -83,7 +86,7 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileTracing) {
   mdm_->RegisterDumpProvider(&mdp);
 
   EnableTracing(kTraceCategory);
-  EXPECT_CALL(mdp, DumpInto(_)).Times(1);
+  EXPECT_CALL(mdp, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
 
   mdm_->UnregisterDumpProvider(&mdp);
@@ -100,7 +103,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   // Enable only mdp1.
   mdm_->RegisterDumpProvider(&mdp1);
   EnableTracing(kTraceCategory);
-  EXPECT_CALL(mdp1, DumpInto(_)).Times(1);
+  EXPECT_CALL(mdp1, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, DumpInto(_)).Times(0);
   mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
   DisableTracing();
@@ -110,16 +113,38 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   mdm_->RegisterDumpProvider(&mdp2);
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp1, DumpInto(_)).Times(0);
-  EXPECT_CALL(mdp2, DumpInto(_)).Times(1);
+  EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
   DisableTracing();
 
   // Enable both mdp1 and mdp2.
   mdm_->RegisterDumpProvider(&mdp1);
   EnableTracing(kTraceCategory);
-  EXPECT_CALL(mdp1, DumpInto(_)).Times(1);
-  EXPECT_CALL(mdp2, DumpInto(_)).Times(1);
+  EXPECT_CALL(mdp1, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
+  EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
+  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  DisableTracing();
+}
+
+// Enable both dump providers, make mdp1 fail and assert that only mdp2 is
+// invoked the 2nd time.
+// FIXME(primiano): remove once crbug.com/461788 gets fixed.
+TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
+  MockDumpProvider mdp1;
+  MockDumpProvider mdp2;
+
+  mdm_->RegisterDumpProvider(&mdp1);
+  mdm_->RegisterDumpProvider(&mdp2);
+  EnableTracing(kTraceCategory);
+
+  EXPECT_CALL(mdp1, DumpInto(_)).Times(1).WillRepeatedly(Return(false));
+  EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+
+  EXPECT_CALL(mdp1, DumpInto(_)).Times(0);
+  EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(false));
+  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+
   DisableTracing();
 }
 
diff --git a/base/trace_event/memory_dump_provider.h b/base/trace_event/memory_dump_provider.h
index 18363c5..1c5bbb1 100644
--- a/base/trace_event/memory_dump_provider.h
+++ b/base/trace_event/memory_dump_provider.h
@@ -17,7 +17,10 @@ class ProcessMemoryDump;
 class BASE_EXPORT MemoryDumpProvider {
  public:
   // Called by the MemoryDumpManager when generating dump points.
-  virtual void DumpInto(ProcessMemoryDump* pmd) = 0;
+  // Returns: true if the |pmd| was successfully populated, false otherwise.
+  virtual bool DumpInto(ProcessMemoryDump* pmd) = 0;
+
+  virtual const char* GetFriendlyName() const = 0;
 
  protected:
   MemoryDumpProvider() {}
diff --git a/base/trace_event/process_memory_maps_dump_provider.cc b/base/trace_event/process_memory_maps_dump_provider.cc
index e1cefc3..93feded 100644
--- a/base/trace_event/process_memory_maps_dump_provider.cc
+++ b/base/trace_event/process_memory_maps_dump_provider.cc
@@ -15,6 +15,10 @@
 namespace base {
 namespace trace_event {
 
+namespace {
+const char kDumperFriendlyName[] = "ProcessMemoryMaps";
+}
+
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 // static
 std::istream* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
@@ -104,10 +108,9 @@ uint32 ParseSmapsCounter(std::istream* smaps,
 }
 
 uint32 ReadLinuxProcSmapsFile(std::istream* smaps, ProcessMemoryMaps* pmm) {
-  if (!smaps->good()) {
-    LOG(ERROR) << "Could not read smaps file.";
+  if (!smaps->good())
     return 0;
-  }
+
   const uint32 kNumExpectedCountersPerRegion = 2;
   uint32 counters_parsed_for_current_region = 0;
   uint32 num_valid_regions = 0;
@@ -154,7 +157,7 @@ ProcessMemoryMapsDumpProvider::~ProcessMemoryMapsDumpProvider() {
 
 // Called at trace dump point time. Creates a snapshot the memory maps for the
 // current process.
-void ProcessMemoryMapsDumpProvider::DumpInto(ProcessMemoryDump* pmd) {
+bool ProcessMemoryMapsDumpProvider::DumpInto(ProcessMemoryDump* pmd) {
   uint32 res = 0;
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
@@ -168,8 +171,16 @@ void ProcessMemoryMapsDumpProvider::DumpInto(ProcessMemoryDump* pmd) {
   LOG(ERROR) << "ProcessMemoryMaps dump provider is supported only on Linux";
 #endif
 
-  if (res > 0)
+  if (res > 0) {
     pmd->set_has_process_mmaps();
+    return true;
+  }
+
+  return false;
+}
+
+const char* ProcessMemoryMapsDumpProvider::GetFriendlyName() const {
+  return kDumperFriendlyName;
 }
 
 }  // namespace trace_event
diff --git a/base/trace_event/process_memory_maps_dump_provider.h b/base/trace_event/process_memory_maps_dump_provider.h
index 543f7fd..0d30db2 100644
--- a/base/trace_event/process_memory_maps_dump_provider.h
+++ b/base/trace_event/process_memory_maps_dump_provider.h
@@ -20,7 +20,8 @@ class BASE_EXPORT ProcessMemoryMapsDumpProvider : public MemoryDumpProvider {
   static ProcessMemoryMapsDumpProvider* GetInstance();
 
   // MemoryDumpProvider implementation.
-  void DumpInto(ProcessMemoryDump* pmd) override;
+  bool DumpInto(ProcessMemoryDump* pmd) override;
+  const char* GetFriendlyName() const override;
 
  private:
   friend struct DefaultSingletonTraits<ProcessMemoryMapsDumpProvider>;
diff --git a/base/trace_event/process_memory_totals_dump_provider.cc b/base/trace_event/process_memory_totals_dump_provider.cc
index cda0ff1..125be38 100644
--- a/base/trace_event/process_memory_totals_dump_provider.cc
+++ b/base/trace_event/process_memory_totals_dump_provider.cc
@@ -15,6 +15,9 @@ namespace trace_event {
 uint64 ProcessMemoryTotalsDumpProvider::rss_bytes_for_testing = 0;
 
 namespace {
+
+const char kDumperFriendlyName[] = "ProcessMemoryTotals";
+
 ProcessMetrics* CreateProcessMetricsForCurrentProcess() {
 #if !defined(OS_MACOSX) || defined(OS_IOS)
   return ProcessMetrics::CreateProcessMetrics(GetCurrentProcessHandle());
@@ -41,12 +44,22 @@ ProcessMemoryTotalsDumpProvider::~ProcessMemoryTotalsDumpProvider() {
 
 // Called at trace dump point time. Creates a snapshot the memory counters for
 // the current process.
-void ProcessMemoryTotalsDumpProvider::DumpInto(ProcessMemoryDump* pmd) {
+bool ProcessMemoryTotalsDumpProvider::DumpInto(ProcessMemoryDump* pmd) {
   const uint64 rss_bytes = rss_bytes_for_testing
                                ? rss_bytes_for_testing
                                : process_metrics_->GetWorkingSetSize();
-  pmd->process_totals()->set_resident_set_bytes(rss_bytes);
-  pmd->set_has_process_totals();
+
+  if (rss_bytes > 0) {
+    pmd->process_totals()->set_resident_set_bytes(rss_bytes);
+    pmd->set_has_process_totals();
+    return true;
+  }
+
+  return false;
+}
+
+const char* ProcessMemoryTotalsDumpProvider::GetFriendlyName() const {
+  return kDumperFriendlyName;
 }
 
 }  // namespace trace_event
diff --git a/base/trace_event/process_memory_totals_dump_provider.h b/base/trace_event/process_memory_totals_dump_provider.h
index 45917a8..8dae966 100644
--- a/base/trace_event/process_memory_totals_dump_provider.h
+++ b/base/trace_event/process_memory_totals_dump_provider.h
@@ -22,7 +22,8 @@ class BASE_EXPORT ProcessMemoryTotalsDumpProvider : public MemoryDumpProvider {
   static ProcessMemoryTotalsDumpProvider* GetInstance();
 
   // MemoryDumpProvider implementation.
-  void DumpInto(ProcessMemoryDump* pmd) override;
+  bool DumpInto(ProcessMemoryDump* pmd) override;
+  const char* GetFriendlyName() const override;
 
  private:
   friend struct DefaultSingletonTraits<ProcessMemoryTotalsDumpProvider>;