Revert of Add crash keys about field trial shared memory errors. (pat…

…chset chromium#3 id:140001 of https://codereview.chromium.org/2872503003/ ) Reason for revert: No crashes observed that this instrumentation was meant to help with. Reverting the change. Original issue's description: > Add crash keys about field trial shared memory errors. > > This is needed to figure out the cause of the failures in field > trial shared memory allocation & map operations. > > Adds crash keys for the error values when shared memory > APIs fail from field trial code, to diagnose the crash causes. > > The temporary logging CL adds coverage to Mac codepaths > specifically, since that is the issue being investigated. The > plan is to clean up the code once the crbug has been fixed. > > BUG=703649 > > Review-Url: https://codereview.chromium.org/2872503003 > Cr-Commit-Position: refs/heads/master@{#470802} > Committed: https://chromium.googlesource.com/chromium/src/+/0fe371d918bf4d467619c1cd2d9fd9f3ee117add TBR=erikchen@chromium.org,thakis@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=703649 Review-Url: https://codereview.chromium.org/2929013002 Cr-Commit-Position: refs/heads/master@{#478076}
Denger-Network · Jun 8, 2017 · c2f02c3 · c2f02c3
1 parent adff188
commit c2f02c3
Show file tree

Hide file tree

Showing 8 changed files with 26 additions and 103 deletions.
diff --git a/base/memory/shared_memory.h b/base/memory/shared_memory.h
@@ -64,22 +64,6 @@ struct BASE_EXPORT SharedMemoryCreateOptions {
   bool share_read_only = false;
 };
 
-// Enumeration of different shared memory error types. Note: Currently only
-// errors from Mac POSIX shared memory implementation are fully instrumented.
-// TODO(asvitkine): Evaluate whether we want to keep this ability after
-// crbug.com/703649 is fixed and expand to all platforms or remove.
-enum class SharedMemoryError {
-  NO_ERRORS,
-  NO_FILE,
-  BAD_PARAMS,
-  STAT_FAILED,
-  TRUNCATE_FAILED,
-  NO_TEMP_DIR,
-  MAKE_READONLY_FAILED,
-  INODE_MISMATCH,
-  MMAP_FAILED,
-};
-
 // Platform abstraction for shared memory.
 // SharedMemory consumes a SharedMemoryHandle [potentially one that it created]
 // to map a shared memory OS resource into the virtual address space of the
@@ -226,13 +210,6 @@ class BASE_EXPORT SharedMemory {
   // failure.
   SharedMemoryHandle GetReadOnlyHandle();
 
-  // Returns the last error encountered as a result of a call to Create() or
-  // Map(). Note: Currently only errors from Mac POSIX shared memory
-  // implementation are fully instrumented.
-  // TODO(asvitkine): Evaluate whether we want to keep this ability after
-  // crbug.com/703649 is fixed and expand to all platforms or remove.
-  SharedMemoryError get_last_error() const { return last_error_; }
-
  private:
 #if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_ANDROID) && \
     (!defined(OS_MACOSX) || defined(OS_IOS))
@@ -263,7 +240,6 @@ class BASE_EXPORT SharedMemory {
   void* memory_ = nullptr;
   bool read_only_ = false;
   size_t requested_size_ = 0;
-  SharedMemoryError last_error_ = SharedMemoryError::NO_ERRORS;
 
   DISALLOW_COPY_AND_ASSIGN(SharedMemory);
 };

diff --git a/base/memory/shared_memory_helper.cc b/base/memory/shared_memory_helper.cc
@@ -25,8 +25,7 @@ using ScopedPathUnlinker =
 bool CreateAnonymousSharedMemory(const SharedMemoryCreateOptions& options,
                                  ScopedFILE* fp,
                                  ScopedFD* readonly_fd,
-                                 FilePath* path,
-                                 SharedMemoryError* error) {
+                                 FilePath* path) {
 #if !(defined(OS_MACOSX) && !defined(OS_IOS))
   // It doesn't make sense to have a open-existing private piece of shmem
   DCHECK(!options.open_existing_deprecated);
@@ -35,16 +34,13 @@ bool CreateAnonymousSharedMemory(const SharedMemoryCreateOptions& options,
   // A: Because they're limited to 4mb on OS X.  FFFFFFFUUUUUUUUUUU
   FilePath directory;
   ScopedPathUnlinker path_unlinker;
-  if (!GetShmemTempDir(options.executable, &directory)) {
-    *error = SharedMemoryError::NO_TEMP_DIR;
+  if (!GetShmemTempDir(options.executable, &directory))
     return false;
-  }
 
   fp->reset(base::CreateAndOpenTemporaryFileInDir(directory, path));
-  if (!*fp) {
-    *error = SharedMemoryError::NO_FILE;
+
+  if (!*fp)
     return false;
-  }
 
   // Deleting the file prevents anyone else from mapping it in (making it
   // private), and prevents the need for cleanup (once the last fd is
@@ -57,7 +53,6 @@ bool CreateAnonymousSharedMemory(const SharedMemoryCreateOptions& options,
     if (!readonly_fd->is_valid()) {
       DPLOG(ERROR) << "open(\"" << path->value() << "\", O_RDONLY) failed";
       fp->reset();
-      *error = SharedMemoryError::MAKE_READONLY_FAILED;
       return false;
     }
   }
@@ -67,14 +62,11 @@ bool CreateAnonymousSharedMemory(const SharedMemoryCreateOptions& options,
 bool PrepareMapFile(ScopedFILE fp,
                     ScopedFD readonly_fd,
                     int* mapped_file,
-                    int* readonly_mapped_file,
-                    SharedMemoryError* error) {
+                    int* readonly_mapped_file) {
   DCHECK_EQ(-1, *mapped_file);
   DCHECK_EQ(-1, *readonly_mapped_file);
-  if (!fp) {
-    *error = SharedMemoryError::NO_FILE;
+  if (fp == NULL)
     return false;
-  }
 
   // This function theoretically can block on the disk, but realistically
   // the temporary files we create will just go into the buffer cache
@@ -91,7 +83,6 @@ bool PrepareMapFile(ScopedFILE fp,
       NOTREACHED();
     if (st.st_dev != readonly_st.st_dev || st.st_ino != readonly_st.st_ino) {
       LOG(ERROR) << "writable and read-only inodes don't match; bailing";
-      *error = SharedMemoryError::INODE_MISMATCH;
       return false;
     }
   }

diff --git a/base/memory/shared_memory_helper.h b/base/memory/shared_memory_helper.h
@@ -16,21 +16,18 @@ namespace base {
 // with the fdopened FILE. |readonly_fd| is populated with the opened fd if
 // options.share_read_only is true. |path| is populated with the location of
 // the file before it was unlinked.
-// Returns false if there's a failure and sets |error|.
+// Returns false if there's an unhandled failure.
 bool CreateAnonymousSharedMemory(const SharedMemoryCreateOptions& options,
                                  ScopedFILE* fp,
                                  ScopedFD* readonly_fd,
-                                 FilePath* path,
-                                 SharedMemoryError* error);
+                                 FilePath* path);
 
 // Takes the outputs of CreateAnonymousSharedMemory and maps them properly to
 // |mapped_file| or |readonly_mapped_file|, depending on which one is populated.
-// Returns false if there's a failure and sets |error|.
 bool PrepareMapFile(ScopedFILE fp,
                     ScopedFD readonly_fd,
                     int* mapped_file,
-                    int* readonly_mapped_file,
-                    SharedMemoryError* error);
+                    int* readonly_mapped_file);
 #endif
 
 }  // namespace base

diff --git a/base/memory/shared_memory_mac.cc b/base/memory/shared_memory_mac.cc
@@ -128,15 +128,11 @@ bool SharedMemory::CreateAndMapAnonymous(size_t size) {
 // "name == L"". The exception is in the StatsTable.
 bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
   DCHECK(!shm_.IsValid());
-  if (options.size == 0) {
-    last_error_ = SharedMemoryError::BAD_PARAMS;
+  if (options.size == 0)
     return false;
-  }
 
-  if (options.size > static_cast<size_t>(std::numeric_limits<int>::max())) {
-    last_error_ = SharedMemoryError::BAD_PARAMS;
+  if (options.size > static_cast<size_t>(std::numeric_limits<int>::max()))
     return false;
-  }
 
   if (options.type == SharedMemoryHandle::MACH) {
     shm_ = SharedMemoryHandle(options.size, UnguessableToken::Create());
@@ -153,31 +149,26 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
   ScopedFD readonly_fd;
 
   FilePath path;
-  bool result = CreateAnonymousSharedMemory(options, &fp, &readonly_fd, &path,
-                                            &last_error_);
+  bool result = CreateAnonymousSharedMemory(options, &fp, &readonly_fd, &path);
   if (!result)
     return false;
   DCHECK(fp);  // Should be guaranteed by CreateAnonymousSharedMemory().
 
   // Get current size.
   struct stat stat;
-  if (fstat(fileno(fp.get()), &stat) != 0) {
-    last_error_ = SharedMemoryError::STAT_FAILED;
+  if (fstat(fileno(fp.get()), &stat) != 0)
     return false;
-  }
   const size_t current_size = stat.st_size;
   if (current_size != options.size) {
-    if (HANDLE_EINTR(ftruncate(fileno(fp.get()), options.size)) != 0) {
-      last_error_ = SharedMemoryError::TRUNCATE_FAILED;
+    if (HANDLE_EINTR(ftruncate(fileno(fp.get()), options.size)) != 0)
       return false;
-    }
   }
   requested_size_ = options.size;
 
   int mapped_file = -1;
   int readonly_mapped_file = -1;
   result = PrepareMapFile(std::move(fp), std::move(readonly_fd), &mapped_file,
-                          &readonly_mapped_file, &last_error_);
+                          &readonly_mapped_file);
   shm_ = SharedMemoryHandle(FileDescriptor(mapped_file, false), options.size,
                             UnguessableToken::Create());
   readonly_shm_ =
@@ -187,18 +178,12 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
 }
 
 bool SharedMemory::MapAt(off_t offset, size_t bytes) {
-  if (!shm_.IsValid()) {
-    last_error_ = SharedMemoryError::BAD_PARAMS;
+  if (!shm_.IsValid())
     return false;
-  }
-  if (bytes > static_cast<size_t>(std::numeric_limits<int>::max())) {
-    last_error_ = SharedMemoryError::BAD_PARAMS;
+  if (bytes > static_cast<size_t>(std::numeric_limits<int>::max()))
     return false;
-  }
-  if (memory_) {
-    last_error_ = SharedMemoryError::BAD_PARAMS;
+  if (memory_)
     return false;
-  }
 
   bool success = shm_.MapAt(offset, bytes, &memory_, read_only_);
   if (success) {
@@ -208,7 +193,6 @@ bool SharedMemory::MapAt(off_t offset, size_t bytes) {
     mapped_memory_mechanism_ = shm_.type_;
     SharedMemoryTracker::GetInstance()->IncrementMemoryUsage(*this);
   } else {
-    last_error_ = SharedMemoryError::MMAP_FAILED;
     memory_ = NULL;
   }
 

diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc
@@ -100,8 +100,8 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
 
   FilePath path;
   if (options.name_deprecated == NULL || options.name_deprecated->empty()) {
-    bool result = CreateAnonymousSharedMemory(options, &fp, &readonly_fd, &path,
-                                              &last_error_);
+    bool result =
+        CreateAnonymousSharedMemory(options, &fp, &readonly_fd, &path);
     if (!result)
       return false;
   } else {
@@ -192,9 +192,8 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
 
   int mapped_file = -1;
   int readonly_mapped_file = -1;
-  bool result =
-      PrepareMapFile(std::move(fp), std::move(readonly_fd), &mapped_file,
-                     &readonly_mapped_file, &last_error_);
+  bool result = PrepareMapFile(std::move(fp), std::move(readonly_fd),
+                               &mapped_file, &readonly_mapped_file);
   shm_ = SharedMemoryHandle(base::FileDescriptor(mapped_file, false),
                             options.size, UnguessableToken::Create());
   readonly_shm_ =
@@ -234,9 +233,8 @@ bool SharedMemory::Open(const std::string& name, bool read_only) {
   }
   int mapped_file = -1;
   int readonly_mapped_file = -1;
-  bool result =
-      PrepareMapFile(std::move(fp), std::move(readonly_fd), &mapped_file,
-                     &readonly_mapped_file, &last_error_);
+  bool result = PrepareMapFile(std::move(fp), std::move(readonly_fd),
+                               &mapped_file, &readonly_mapped_file);
   // This form of sharing shared memory is deprecated. https://crbug.com/345734.
   // However, we can't get rid of it without a significant refactor because its
   // used to communicate between two versions of the same service process, very

diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
@@ -11,7 +11,6 @@
 #include "base/build_time.h"
 #include "base/command_line.h"
 #include "base/debug/activity_tracker.h"
-#include "base/debug/crash_logging.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial_param_associator.h"
 #include "base/process/memory.h"
@@ -1280,25 +1279,11 @@ void FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded() {
 #endif
 
   std::unique_ptr<SharedMemory> shm(new SharedMemory());
-  if (!shm->Create(options)) {
-#if !defined(OS_NACL)
-    // Temporary for http://crbug.com/703649.
-    base::debug::ScopedCrashKey crash_key(
-        "field_trial_shmem_create_error",
-        base::IntToString(static_cast<int>(shm->get_last_error())));
-#endif
+  if (!shm->Create(options))
     OnOutOfMemory(kFieldTrialAllocationSize);
-  }
 
-  if (!shm->Map(kFieldTrialAllocationSize)) {
-#if !defined(OS_NACL)
-    // Temporary for http://crbug.com/703649.
-    base::debug::ScopedCrashKey crash_key(
-        "field_trial_shmem_map_error",
-        base::IntToString(static_cast<int>(shm->get_last_error())));
-#endif
+  if (!shm->Map(kFieldTrialAllocationSize))
     OnOutOfMemory(kFieldTrialAllocationSize);
-  }
 
   global_->field_trial_allocator_.reset(
       new FieldTrialAllocator(std::move(shm), 0, kAllocatorName, false));

diff --git a/chrome/app/chrome_crash_reporter_client_win.cc b/chrome/app/chrome_crash_reporter_client_win.cc
@@ -182,10 +182,6 @@ size_t RegisterCrashKeysHelper() {
       {"engine_params", crash_keys::kMediumSize},
       {"engine1_params", crash_keys::kMediumSize},
       {"engine2_params", crash_keys::kMediumSize},
-
-      // Temporary for http://crbug.com/703649.
-      {"field_trial_shmem_create_error", crash_keys::kSmallSize},
-      {"field_trial_shmem_map_error", crash_keys::kSmallSize},
   };
 
   // This dynamic set of keys is used for sets of key value pairs when gathering

diff --git a/chrome/common/crash_keys.cc b/chrome/common/crash_keys.cc
@@ -218,10 +218,6 @@ size_t RegisterChromeCrashKeys() {
     {"engine_params", crash_keys::kMediumSize},
     {"engine1_params", crash_keys::kMediumSize},
     {"engine2_params", crash_keys::kMediumSize},
-
-    // Temporary for http://crbug.com/703649.
-    {"field_trial_shmem_create_error", crash_keys::kSmallSize},
-    {"field_trial_shmem_map_error", crash_keys::kSmallSize},
   };
 
   // This dynamic set of keys is used for sets of key value pairs when gathering