Better handling of invalid files in metrics directory.

Previously, when managing a directory of files, only one file was ever
processed per call.  This wasn't a problem because after each call,
another would be scheduled.  However, the "independent" files are
processed by an outside caller and so if the file ready for it turned
out to be invalid, it would see "nothing to be done" and wait an
extended period of time to try again.

The new version does the validity test during the initial check and
immediately advances to the next file if it detects a problem.  This
means there is always a file to process and never a significant delay
before trying the next one.

Bug: 760317, 780036
Change-Id: I3fece9e41af3b8d29d0fa80b25141c00f9ae9db2
Reviewed-on: https://chromium-review.googlesource.com/804063Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521770}

components/metrics/file_metrics_provider.cc

@@ -367,6 +367,7 @@ void FileMetricsProvider::FinishedWithSource(SourceInfo* source,
       // the browser.
       if (result == ACCESS_RESULT_SUCCESS ||
           result == ACCESS_RESULT_NOT_MODIFIED ||
+          result == ACCESS_RESULT_MEMORY_DELETED ||
           result == ACCESS_RESULT_TOO_OLD) {
         DeleteFileWhenPossible(source->path);
       }
@@ -384,28 +385,39 @@ void FileMetricsProvider::CheckAndMergeMetricSourcesOnTaskRunner(
   // This method has all state information passed in |sources| and is intended
   // to run on a worker thread rather than the UI thread.
   for (std::unique_ptr<SourceInfo>& source : *sources) {
-    AccessResult result = CheckAndMapMetricSource(source.get());
+    AccessResult result;
+    do {
+      result = CheckAndMapMetricSource(source.get());
+
+      // Some results are not reported in order to keep the dashboard clean.
+      if (result != ACCESS_RESULT_DOESNT_EXIST &&
+          result != ACCESS_RESULT_NOT_MODIFIED &&
+          result != ACCESS_RESULT_THIS_PID) {
+        RecordAccessResult(result);
+      }
 
-    // Some results are not reported in order to keep the dashboard clean.
-    if (result != ACCESS_RESULT_DOESNT_EXIST &&
-        result != ACCESS_RESULT_NOT_MODIFIED &&
-        result != ACCESS_RESULT_THIS_PID) {
-      RecordAccessResult(result);
-    }
+      // If there are no files (or no more files) in this source, stop now.
+      if (result == ACCESS_RESULT_DOESNT_EXIST)
+        break;
 
-    // Metrics associated with internal profiles have to be fetched directly
-    // so just keep the mapping for use by the main thread.
-    if (source->association == ASSOCIATE_INTERNAL_PROFILE)
-      continue;
+      // Mapping was successful. Merge it.
+      if (result == ACCESS_RESULT_SUCCESS) {
+        // Metrics associated with internal profiles have to be fetched directly
+        // so just keep the mapping for use by the main thread.
+        if (source->association == ASSOCIATE_INTERNAL_PROFILE)
+          break;
 
-    // Mapping was successful. Merge it.
-    if (result == ACCESS_RESULT_SUCCESS) {
-      MergeHistogramDeltasFromSource(source.get());
-      DCHECK(source->read_complete);
-    }
+        MergeHistogramDeltasFromSource(source.get());
+        DCHECK(source->read_complete);
+      }
 
-    // All done with this source.
-    FinishedWithSource(source.get(), result);
+      // All done with this source.
+      FinishedWithSource(source.get(), result);
+
+      // If it's a directory, keep trying until a file is successfully opened.
+      // When there are no more files, ACCESS_RESULT_DOESNT_EXIST will be
+      // returned and the loop will exit above.
+    } while (result != ACCESS_RESULT_SUCCESS && !source->directory.empty());
   }
 }
 
@@ -480,6 +492,13 @@ FileMetricsProvider::AccessResult FileMetricsProvider::CheckAndMapMetricSource(
   source->allocator = base::MakeUnique<base::PersistentHistogramAllocator>(
       std::move(memory_allocator));
 
+  // Check that an "independent" file has the necessary information present.
+  if (source->association == ASSOCIATE_INTERNAL_PROFILE &&
+      !PersistentSystemProfile::GetSystemProfile(
+          *source->allocator->memory_allocator(), nullptr)) {
+    return ACCESS_RESULT_NO_PROFILE;
+  }
+
   return ACCESS_RESULT_SUCCESS;
 }
 

components/metrics/file_metrics_provider.h

@@ -222,6 +222,9 @@ class FileMetricsProvider : public MetricsProvider,
     // The file was skipped because it's being written by this process.
     ACCESS_RESULT_THIS_PID,
 
+    // The file had no embedded system profile.
+    ACCESS_RESULT_NO_PROFILE,
+
     ACCESS_RESULT_MAX
   };
 

components/metrics/file_metrics_provider_unittest.cc

@@ -137,6 +137,17 @@ class FileMetricsProviderTest : public testing::TestWithParam<bool> {
     return flattener.GetRecordedDeltaHistogramNames().size();
   }
 
+  size_t GetIndependentHistogramCount() {
+    HistogramFlattenerDeltaRecorder flattener;
+    base::HistogramSnapshotManager snapshot_manager(&flattener);
+    SystemProfileProto profile_proto;
+    if (!provider()->ProvideIndependentMetrics(&profile_proto,
+                                               &snapshot_manager)) {
+      return 0;
+    }
+    return flattener.GetRecordedDeltaHistogramNames().size();
+  }
+
   void CreateGlobalHistograms(int histogram_count) {
     DCHECK_GT(kMaxCreateHistograms, histogram_count);
 
@@ -177,6 +188,8 @@ class FileMetricsProviderTest : public testing::TestWithParam<bool> {
 
   std::unique_ptr<base::PersistentHistogramAllocator>
   CreateMetricsFileWithHistograms(
+      const base::FilePath& file_path,
+      base::Time write_time,
       int histogram_count,
       const std::function<void(base::PersistentHistogramAllocator*)>&
           callback) {
@@ -190,14 +203,15 @@ class FileMetricsProviderTest : public testing::TestWithParam<bool> {
         base::GlobalHistogramAllocator::ReleaseForTesting();
     callback(histogram_allocator.get());
 
-    WriteMetricsFile(metrics_file(), histogram_allocator.get());
+    WriteMetricsFileAtTime(file_path, histogram_allocator.get(), write_time);
     return histogram_allocator;
   }
 
   std::unique_ptr<base::PersistentHistogramAllocator>
   CreateMetricsFileWithHistograms(int histogram_count) {
     return CreateMetricsFileWithHistograms(
-        histogram_count, [](base::PersistentHistogramAllocator* allocator) {});
+        metrics_file(), base::Time::Now(), histogram_count,
+        [](base::PersistentHistogramAllocator* allocator) {});
   }
 
   base::HistogramBase* GetCreatedHistogram(int index) {
@@ -469,6 +483,74 @@ TEST_P(FileMetricsProviderTest, AccessDirectory) {
   EXPECT_TRUE(base::PathExists(metrics_files.GetPath().AppendASCII("baz")));
 }
 
+TEST_P(FileMetricsProviderTest, AccessDirectoryWithInvalidFiles) {
+  ASSERT_FALSE(PathExists(metrics_file()));
+
+  // Create files starting with a timestamp a few minutes back.
+  base::Time base_time = base::Time::Now() - base::TimeDelta::FromMinutes(10);
+
+  base::ScopedTempDir metrics_files;
+  EXPECT_TRUE(metrics_files.CreateUniqueTempDir());
+
+  CreateMetricsFileWithHistograms(
+      metrics_files.GetPath().AppendASCII("h1.pma"),
+      base_time + base::TimeDelta::FromMinutes(1), 1,
+      [](base::PersistentHistogramAllocator* allocator) {
+        allocator->memory_allocator()->SetMemoryState(
+            base::PersistentMemoryAllocator::MEMORY_DELETED);
+      });
+
+  CreateMetricsFileWithHistograms(
+      metrics_files.GetPath().AppendASCII("h2.pma"),
+      base_time + base::TimeDelta::FromMinutes(2), 2,
+      [](base::PersistentHistogramAllocator* allocator) {
+        SystemProfileProto profile_proto;
+        SystemProfileProto::FieldTrial* trial = profile_proto.add_field_trial();
+        trial->set_name_id(123);
+        trial->set_group_id(456);
+
+        PersistentSystemProfile persistent_profile;
+        persistent_profile.RegisterPersistentAllocator(
+            allocator->memory_allocator());
+        persistent_profile.SetSystemProfile(profile_proto, true);
+      });
+
+  CreateMetricsFileWithHistograms(
+      metrics_files.GetPath().AppendASCII("h3.pma"),
+      base_time + base::TimeDelta::FromMinutes(3), 3,
+      [](base::PersistentHistogramAllocator* allocator) {
+        allocator->memory_allocator()->SetMemoryState(
+            base::PersistentMemoryAllocator::MEMORY_DELETED);
+      });
+
+  // Register the file and allow the "checker" task to run.
+  provider()->RegisterSource(FileMetricsProvider::Params(
+      metrics_files.GetPath(),
+      FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_DIR,
+      FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE, kMetricsName));
+
+  // No files yet.
+  EXPECT_EQ(0U, GetIndependentHistogramCount());
+  EXPECT_TRUE(base::PathExists(metrics_files.GetPath().AppendASCII("h1.pma")));
+  EXPECT_TRUE(base::PathExists(metrics_files.GetPath().AppendASCII("h2.pma")));
+  EXPECT_TRUE(base::PathExists(metrics_files.GetPath().AppendASCII("h3.pma")));
+
+  // H1 should be skipped and H2 available.
+  OnDidCreateMetricsLog();
+  RunTasks();
+  EXPECT_EQ(2U, GetIndependentHistogramCount());
+  EXPECT_FALSE(base::PathExists(metrics_files.GetPath().AppendASCII("h1.pma")));
+  EXPECT_TRUE(base::PathExists(metrics_files.GetPath().AppendASCII("h2.pma")));
+  EXPECT_TRUE(base::PathExists(metrics_files.GetPath().AppendASCII("h3.pma")));
+
+  // Nothing else should be found.
+  OnDidCreateMetricsLog();
+  RunTasks();
+  EXPECT_EQ(0U, GetIndependentHistogramCount());
+  EXPECT_FALSE(base::PathExists(metrics_files.GetPath().AppendASCII("h2.pma")));
+  EXPECT_FALSE(base::PathExists(metrics_files.GetPath().AppendASCII("h3.pma")));
+}
+
 TEST_P(FileMetricsProviderTest, AccessTimeLimitedDirectory) {
   ASSERT_FALSE(PathExists(metrics_file()));
 
@@ -807,7 +889,8 @@ TEST_P(FileMetricsProviderTest, AccessEmbeddedProfileMetricsWithoutProfile) {
 TEST_P(FileMetricsProviderTest, AccessEmbeddedProfileMetricsWithProfile) {
   ASSERT_FALSE(PathExists(metrics_file()));
   CreateMetricsFileWithHistograms(
-      2, [](base::PersistentHistogramAllocator* allocator) {
+      metrics_file(), base::Time::Now(), 2,
+      [](base::PersistentHistogramAllocator* allocator) {
         SystemProfileProto profile_proto;
         SystemProfileProto::FieldTrial* trial = profile_proto.add_field_trial();
         trial->set_name_id(123);
@@ -878,7 +961,8 @@ TEST_P(FileMetricsProviderTest, AccessEmbeddedFallbackMetricsWithoutProfile) {
 TEST_P(FileMetricsProviderTest, AccessEmbeddedFallbackMetricsWithProfile) {
   ASSERT_FALSE(PathExists(metrics_file()));
   CreateMetricsFileWithHistograms(
-      2, [](base::PersistentHistogramAllocator* allocator) {
+      metrics_file(), base::Time::Now(), 2,
+      [](base::PersistentHistogramAllocator* allocator) {
         SystemProfileProto profile_proto;
         SystemProfileProto::FieldTrial* trial = profile_proto.add_field_trial();
         trial->set_name_id(123);
@@ -923,7 +1007,8 @@ TEST_P(FileMetricsProviderTest, AccessEmbeddedProfileMetricsFromDir) {
   std::vector<base::FilePath> file_names;
   for (int i = 0; i < file_count; ++i) {
     CreateMetricsFileWithHistograms(
-        2, [](base::PersistentHistogramAllocator* allocator) {
+        metrics_file(), base::Time::Now(), 2,
+        [](base::PersistentHistogramAllocator* allocator) {
           SystemProfileProto profile_proto;
           SystemProfileProto::FieldTrial* trial =
               profile_proto.add_field_trial();

components/metrics/persistent_system_profile.cc

@@ -361,6 +361,9 @@ bool PersistentSystemProfile::GetSystemProfile(
       return false;
   } while (type != kSystemProfileProto);
 
+  if (!system_profile)
+    return true;
+
   if (!system_profile->ParseFromString(record))
     return false;
 

components/metrics/persistent_system_profile.h

@@ -47,7 +47,9 @@ class PersistentSystemProfile {
       const base::PersistentMemoryAllocator& memory_allocator);
 
   // Retrieves the system profile from a persistent memory allocator. Returns
-  // true if a profile was successfully retrieved.
+  // true if a profile was successfully retrieved. If null is passed for the
+  // |system_profile|, only a basic check for the existance of one will be
+  // done.
   static bool GetSystemProfile(
       const base::PersistentMemoryAllocator& memory_allocator,
       SystemProfileProto* system_profile);

tools/metrics/histograms/enums.xml

@@ -18051,6 +18051,7 @@ Called by update_net_error_codes.py.-->
   <int value="11" label="File made for too many files in the directory."/>
   <int value="12" label="File made for too many bytes in the directory."/>
   <int value="13" label="File was skipped because it belongs to this process."/>
+  <int value="14" label="File did not have required embedded profile."/>
 </enum>
 
 <enum name="FileMetricsProviderEmbeddedProfileResult">