Open persistent metrics files exclusively for write on Windows.

Also, return false if the file couldn't be opened and skip
creating a MemoryMappedFile.

We think that these files sometimes get opened by two Chrome
processes at the same time, and this is the cause of duplicated
samples for some startup metrics from the same report and
negative values for bucket counts.

We should be able to see if occurrences of the issue go away
after this change to verify if this was the cause.

Bug: 1176977
Change-Id: Ia85c81c7ac7e7c65fb877e729ef43acfa205a8d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3097769
Auto-Submit: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912981}

base/metrics/persistent_histogram_allocator.cc

@@ -680,14 +680,18 @@ void GlobalHistogramAllocator::CreateWithLocalMemory(
 
 #if !defined(OS_NACL)
 // static
-bool GlobalHistogramAllocator::CreateWithFile(
-    const FilePath& file_path,
-    size_t size,
-    uint64_t id,
-    StringPiece name) {
-  File file(
-      file_path, File::FLAG_OPEN_ALWAYS | File::FLAG_SHARE_DELETE |
-                 File::FLAG_READ | File::FLAG_WRITE);
+bool GlobalHistogramAllocator::CreateWithFile(const FilePath& file_path,
+                                              size_t size,
+                                              uint64_t id,
+                                              StringPiece name,
+                                              bool exclusive_write) {
+  uint32_t flags = File::FLAG_OPEN_ALWAYS | File::FLAG_SHARE_DELETE |
+                   File::FLAG_READ | File::FLAG_WRITE;
+  if (exclusive_write)
+    flags |= File::FLAG_EXCLUSIVE_WRITE;
+  File file(file_path, flags);
+  if (!file.IsValid())
+    return false;
 
   std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
   bool success = false;

base/metrics/persistent_histogram_allocator.h

@@ -356,11 +356,13 @@ class BASE_EXPORT GlobalHistogramAllocator
   // not exist, it will be created with the specified |size|. If the file does
   // exist, the allocator will use and add to its contents, ignoring the passed
   // size in favor of the existing size. Returns whether the global allocator
-  // was set.
+  // was set. If |exclusive_write| is true, the file will be opened in a mode
+  // that disallows multiple concurrent writers.
   static bool CreateWithFile(const FilePath& file_path,
                              size_t size,
                              uint64_t id,
-                             StringPiece name);
+                             StringPiece name,
+                             bool exclusive_write = false);
 
   // Creates a new file at |active_path|. If it already exists, it will first be
   // moved to |base_path|. In all cases, any old file at |base_path| will be

components/metrics/file_metrics_provider.cc

@@ -61,27 +61,29 @@ enum : int {
 };
 
 constexpr SourceOptions kSourceOptions[] = {
-  // SOURCE_HISTOGRAMS_ATOMIC_FILE
-  {
-    // Ensure that no other process reads this at the same time.
-    STD_OPEN | base::File::FLAG_EXCLUSIVE_READ,
-    base::MemoryMappedFile::READ_ONLY,
-    true
-  },
-  // SOURCE_HISTOGRAMS_ATOMIC_DIR
-  {
-    // Ensure that no other process reads this at the same time.
-    STD_OPEN | base::File::FLAG_EXCLUSIVE_READ,
-    base::MemoryMappedFile::READ_ONLY,
-    true
-  },
-  // SOURCE_HISTOGRAMS_ACTIVE_FILE
-  {
-    // Allow writing (updated "logged" values) to the file.
-    STD_OPEN | base::File::FLAG_WRITE,
-    base::MemoryMappedFile::READ_WRITE,
-    false
-  }
+    // SOURCE_HISTOGRAMS_ATOMIC_FILE
+    {
+        // Ensure that no other process reads this at the same time.
+        STD_OPEN | base::File::FLAG_EXCLUSIVE_READ,
+        base::MemoryMappedFile::READ_ONLY,
+        true,
+    },
+    // SOURCE_HISTOGRAMS_ATOMIC_DIR
+    {
+        // Ensure that no other process reads this at the same time.
+        STD_OPEN | base::File::FLAG_EXCLUSIVE_READ,
+        base::MemoryMappedFile::READ_ONLY,
+        true,
+    },
+    // SOURCE_HISTOGRAMS_ACTIVE_FILE
+    {
+        // Allow writing to the file. This is needed so we can keep track of
+        // deltas that have been uploaded (by modifying the file), while the
+        // file may still be open by an external process (e.g. Crashpad).
+        STD_OPEN | base::File::FLAG_WRITE,
+        base::MemoryMappedFile::READ_WRITE,
+        false,
+    },
 };
 
 void DeleteFileWhenPossible(const base::FilePath& path) {

components/metrics/persistent_histograms.cc

@@ -126,14 +126,26 @@ InitResult InitWithMappedFile(const base::FilePath& metrics_dir,
     // "active" filename is supposed to be unique so this shouldn't happen.
     result = kMappedFileExists;
   } else {
+#if defined(OS_WIN)
+    // Disallow multiple writers on Windows. Needed to ensure multiple instances
+    // of Chrome aren't writing to the same file, which could happen in some
+    // rare circumstances observed in the wild (e.g. on FAT FS where the file
+    // name ends up not being unique due to truncation and two processes racing
+    // on base::PathExists(active_file) above).
+    // TODO(crbug.com/1176977): If this resolves the bug, consider making
+    // consistent on all platforms.
+    bool exclusive_write = true;
+#else
+    bool exclusive_write = false;
+#endif
     // Move any spare file into the active position.
-    // TODO(crbug.com/1183166): Don't do this if |kSpareFileRequired| = false.
     base::ReplaceFile(spare_file, active_file, nullptr);
     // Create global allocator using the |active_file|.
     if (kSpareFileRequired && !base::PathExists(active_file)) {
       result = kNoSpareFile;
     } else if (base::GlobalHistogramAllocator::CreateWithFile(
-                   active_file, kAllocSize, kAllocId, kBrowserMetricsName)) {
+                   active_file, kAllocSize, kAllocId, kBrowserMetricsName,
+                   exclusive_write)) {
       // TODO(crbug.com/1176977): Remove this instrumentation when bug is fixed.
       // Log the drive type on Windows.
 #if defined(OS_WIN)