Create CrashReportDatabase interface, a test, and a Mac implementation.

client/crash_report_database_mac.mm

Index: client/crash_report_database_mac.mm
diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..2c3ace8c575727f3b1eb0c9e8ea62792bafdeb32
--- /dev/null
+++ b/client/crash_report_database_mac.mm
@@ -0,0 +1,456 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "client/crash_report_database.h"
+
+#include <fcntl.h>
+#import <Foundation/Foundation.h>
+#include <stdio.h>
+#include <sys/errno.h>

[Mark Mentovai, 2015/01/08 22:38:10]

Just <errno.h>

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+#include "base/logging.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/scoped_generic.h"
+#include "base/strings/string_piece.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/sys_string_conversions.h"
+#include "util/mac/xattr.h"
+
+namespace crashpad {
+
+namespace {
+
+const char kDatabaseDirectoryName[] = "Crashpad Database";
+
+const char kWriteDirectory[] = "In Progress";

[Mark Mentovai, 2015/01/08 22:38:10]

If I saw these three directories side-by-side on the disk, I wouldn’t know the
difference between “reports” and “in progress.” All three directories contain
reports. It’s not obvious that “in progress” should mean “being written” or
“being uploaded” or “needs upload.”

Also, spaceless lowercase names are easier to deal with from a shell for things
that don’t really need to be user-visible.

[Robert Sesek, 2015/01/13 16:18:23]

This seems self-contradictory to say that it should be obvious yet it's also not
user-visible. But changed the names.

+
+const char kUploadPendingDirectory[] = "Reports";
+
+const char kUploadedDirectory[] = "Uploaded";
+
+const char kPlaceholderReportFileExtension[] = "placeholder";
+const char kCrashReportFileExtension[] = "dmp";
+
+const char kXattrUUID[] = "uuid";
+const char kXattrCollectorID[] = "id";
+const char kXattrIsUploaded[] = "uploaded";
+const char kXattrLastUploadTime[] = "last_upload_time";
+const char kXattrUploadAttemptCount[] = "upload_count";
+
+const char kXattrDatabaseInitialized[] = "initialized";
+
+struct ScopedFileLockTraits {
+  static int InvalidValue() {
+    return 0;
+  }
+
+  static void Free(int fd) {
+    PCHECK(IGNORE_EINTR(close(fd)) == 0) << "close";
+  }
+};
+
+using ScopedFileLock = base::ScopedGeneric<int, ScopedFileLockTraits>;
+
+class CrashReportDatabaseMac : public CrashReportDatabase {

[Mark Mentovai, 2015/01/08 22:38:10]

Either this file or this class should have some comments explaining interesting
bits of implementation in English, like the life-cycle of things on disk, what
the flocks do, etc.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+ public:
+  explicit CrashReportDatabaseMac(const base::FilePath& path);
+  virtual ~CrashReportDatabaseMac();
+
+  bool Initialize();
+
+  // CrashReportDatabase:
+  OperationStatus PrepareNewCrashReport(Report* report) override;
+  OperationStatus FinishedWritingCrashReport(const UUID& uuid) override;
+  OperationStatus LookUpCrashReport(const UUID& uuid,
+                                    Report* report) override;
+  OperationStatus GetNotUploadedReports(
+      std::vector<const Report>* reports) override;
+  OperationStatus GetUploadedReports(
+      std::vector<const Report>* reports) override;
+  OperationStatus RecordUploadAttempt(const UUID& uuid,
+                                      bool successful,
+                                      const std::string& id) override;
+
+ private:
+  // Locates a crash report by |uuid| in the database and returns the full path
+  // to the report file, or an empty path if it cannot be found.
+  base::FilePath LocateCrashReport(const UUID& uuid);
+
+  // Obtains an advisory lock on the file at |path|. The flock is used to
+  // prevent cross-process concurrent metadata reads or writes. While xattrs
+  // do not observe the lock, if the lock-then-mutate protocol is observed by
+  // all clients of the database, it still enforces synchronization.
+  ScopedFileLock ObtainReportLock(const base::FilePath& path);
+
+  // Reads all the database xattrs from a file at |path| into the |report|. The
+  // flock must be obtained before calling this.
+  bool ReadReportMetadataLocked(const base::FilePath& path,
+                                Report* report);
+
+  // Reads the metadata from all the reports in a database subdirectory.
+  // Invalid reports are skipped.
+  OperationStatus ReportsInDirectory(const base::FilePath& path,
+                                     std::vector<const Report>* reports);
+
+
+  // Creates a database xattr name from the short constant name.
+  static std::string XattrName(const base::StringPiece& name);
+
+  base::FilePath base_dir_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrashReportDatabaseMac);
+};
+
+CrashReportDatabaseMac::CrashReportDatabaseMac(const base::FilePath& path)
+    : CrashReportDatabase(), base_dir_(path) {
+}
+
+CrashReportDatabaseMac::~CrashReportDatabaseMac() {}
+
+bool CrashReportDatabaseMac::Initialize() {
+  NSFileManager* file_manager = [NSFileManager defaultManager];
+
+  // Check if the database already exists.
+  BOOL is_dir = NO;
+  if ([file_manager fileExistsAtPath:base::SysUTF8ToNSString(base_dir_.value())
+                         isDirectory:&is_dir]) {
+    if (!is_dir) {
+      LOG(ERROR) << "Database exists at " << base_dir_.value()
+                 << " but is not a directory";
+      return false;
+    }
+  } else {
+    // The database directory does not exist, so create it.
+    if (mkdir(base_dir_.value().c_str(), S_IRWXU | S_IRWXG)) {

[Mark Mentovai, 2015/01/08 22:38:10]

Why are you giving group (but not other) permission (especially write)? I could
see 0700 or 0755 here. Same on line 160.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+      PLOG(ERROR) << "mkdir " << base_dir_.value();
+      return false;
+    }
+  }
+
+  // Create the three processing directories for the database.
+  const char* paths[] = {
+    kWriteDirectory,
+    kUploadPendingDirectory,
+    kUploadedDirectory
+  };
+  for (size_t i = 0; i < arraysize(paths); ++i) {
+    base::FilePath path = base_dir_.Append(paths[i]);
+    BOOL is_dir = NO;
+    if ([file_manager fileExistsAtPath:base::SysUTF8ToNSString(path.value())

[Mark Mentovai, 2015/01/08 22:38:10]

You’ve got this pattern twice now, maybe you want to extract it into a helper
that ensures its path argument exists and is a directory, and attempts to create
it otherwise.

The safest way to implement it is, I think:

  if (mkdir(…) == 0) {
    return true;
  } else if (errno == EEXIST) {
    if (stat(...) == 0) {  // For this, call stat(), not lstat()
      if (S_ISDIR(stat_buf.st_mode)) {
        return true;
      }
      LOG(ERROR) << "not a dir";
    } else {
      PLOG(ERROR) << "stat";
    }
  } else {
    PLOG(ERROR) << "mkdir";
  }
  return false;

How is it different than checking existence and type first? Atomicity. This way
lets two guys come along and try to initialize the same directory
simultaneously.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+                           isDirectory:&is_dir] &&
+        is_dir) {
+      continue;
+    }
+
+    if (mkdir(path.value().c_str(), S_IRWXU | S_IRWXG)) {
+      PLOG(ERROR) << "mkdir " << path.value();
+      return false;
+    }
+  }
+
+  // Write an xattr as the last step, to ensure the filesystem has support for
+  // them. This attribute will never be read.

[Mark Mentovai, 2015/01/08 22:38:10]

Smart!

+  return WriteXattrBool(base_dir_, XattrName(kXattrDatabaseInitialized), true);
+}
+
+CrashReportDatabase::OperationStatus
+CrashReportDatabaseMac::PrepareNewCrashReport(
+    CrashReportDatabase::Report* report) {
+  *report = Report();
+  uuid_t uuid;
+  uuid_generate(uuid);
+  report->uuid.InitializeFromBytes(uuid);
+
+  base::FilePath write_dir = base_dir_.Append(kWriteDirectory);
+
+  // Create a placeholder file that will be used to store a record of this
+  // operation. The crash report file is written by a client of this class.
+  base::FilePath placeholder = write_dir.Append(
+      report->uuid.ToString() + "." + kPlaceholderReportFileExtension);
+  NSFileManager* manager = [NSFileManager defaultManager];
+  if (![manager createFileAtPath:base::SysUTF8ToNSString(placeholder.value())

[Mark Mentovai, 2015/01/08 22:38:10]

This doesn’t have O_EXCL semantics, but it probably ought to.

[Mark Mentovai, 2015/01/08 22:38:10]

Honestly, I was kinda hoping that the on-disk filenames would be a little
friendlier, with a timestamp, pid, and maybe something in the crashy app’s
control that Chrome would stuff with ptype, so that it’d be easy to look at a
dump_dir and know quickly which crash was responsible for yesterday morning’s
sad tab.

But these are hopes and dreams. Let’s go with the UUIDs for now. They do smell
like Breakpad, but we can revisit later.

[Robert Sesek, 2015/01/13 16:18:23]

I'm not sure the PID is useful. Timestamp definitely would be, but that'd be
recorded in the filesystem. I hadn't thought a user-supplied string. But it
seems like displaying these reports would be better handled by the embedding
application (e.g. with chrome://crashes).

+                        contents:[NSData data]
+                      attributes:nil]) {
+    LOG(ERROR) << "Failed to create report placeholder file "
+               << placeholder.value();
+    return kFileSystemError;
+  }
+
+  report->file_path = write_dir.Append(

[Mark Mentovai, 2015/01/08 22:38:10]

Why is there a separate placeholder file? Why don’t you just have a single file
in the write_dir and have the client write to that?

I think, ideally:

1. PrepareNewCrashReport() makes the new file with O_WRONLY | O_CREAT | O_EXCL |
O_EXLOCK.
2. The dumpers write straight to this file. (Ideally, the client would just get
a FD, not a path.)
3. FinishedWritingCrashReport() sets xattrs, renames it to the gotta-upload
directory, and unlocks it.
…
0. (for the future, no need to add it to this CL which is already long) On
startup, maybe on a background thread, look at everything in the write_dir.
Anything that’s in there and that isn’t locked (you can “trylock” with LOCK_EX |
LOCK_NB) should be removed, because it’s a leftover turd from a dumper that
never finished and no longer exists. It’s nice to clean up after ourselves.

[Robert Sesek, 2015/01/13 16:18:23]

I'm glad you suggested returning the FD. I went back and forth on whether this
should return the FileHandle or not, so will happily make it so.

+      report->uuid.ToString() + "." + kCrashReportFileExtension);
+
+  return kNoError;
+}
+
+CrashReportDatabase::OperationStatus
+CrashReportDatabaseMac::FinishedWritingCrashReport(const UUID& uuid) {
+  NSFileManager* manager = [NSFileManager defaultManager];
+  base::FilePath write_dir = base_dir_.Append(kWriteDirectory);
+
+  // Look up the placeholder file to ensure this UUID was prepared.
+  base::FilePath placeholder = write_dir.Append(
+      uuid.ToString() + "." + kPlaceholderReportFileExtension);
+  if (![manager fileExistsAtPath:base::SysUTF8ToNSString(placeholder.value())
+                     isDirectory:nullptr]) {
+    LOG(ERROR) << "Failed to find placeholder report " << placeholder.value();
+    return kReportNotFound;
+  }
+
+  // Delete the placeholder.
+  if (unlink(placeholder.value().c_str())) {
+    PLOG(ERROR) << "unlink " << placeholder.value();

[Mark Mentovai, 2015/01/08 22:38:09]

For things that you’re tolerating, WARNING is probably better.

+    // While not a good sign, continue processing if possible.
+  }
+
+  // Now make sure the crash report was actually written.
+  base::FilePath report_path = write_dir.Append(
+      uuid.ToString() + "." + kCrashReportFileExtension);
+  if (![manager fileExistsAtPath:base::SysUTF8ToNSString(report_path.value())

[Mark Mentovai, 2015/01/08 22:38:10]

I don’t know if you need any of these file-exists checks. The WriteXattr and
rename would fail with obvious messages if the report isn’t there.

[Robert Sesek, 2015/01/13 16:18:22]

Done.

+                     isDirectory:nullptr]) {
+    LOG(ERROR) << "Failed to find crash report " << report_path.value();
+    return kReportNotFound;
+  }
+
+  ScopedFileLock lock(ObtainReportLock(report_path));
+
+  // Record the UUID of this crash report.
+  if (!WriteXattr(report_path, XattrName(kXattrUUID), uuid.ToString())) {
+    return kDatabaseError;
+  }
+
+  // Move the report to its new location for uploading.
+  base::FilePath new_path =
+      base_dir_.Append(kUploadPendingDirectory).Append(report_path.BaseName());
+  if (rename(report_path.value().c_str(), new_path.value().c_str())) {
+    PLOG(ERROR) << "rename " << report_path.value() << " to "
+                << new_path.value();
+    return kFileSystemError;
+  }
+
+  return kNoError;
+}
+
+CrashReportDatabase::OperationStatus
+CrashReportDatabaseMac::LookUpCrashReport(const UUID& uuid,
+                                          CrashReportDatabase::Report* report) {
+  base::FilePath path = LocateCrashReport(uuid);
+  if (path.empty())
+    return kReportNotFound;
+
+  ScopedFileLock lock(ObtainReportLock(path));
+
+  *report = Report();
+  report->file_path = path;
+  if (!ReadReportMetadataLocked(path, report))
+    return kDatabaseError;

[Mark Mentovai, 2015/01/08 22:38:10]

The interface documentation said that this return code is supposed to log
something, but it doesn’t. (ReadReportMetadataLocked() never logs anything.)

[Robert Sesek, 2015/01/13 16:18:22]

ReadXattr and friends, do, though. Do you want more explicit logging than that?

+
+  return kNoError;
+}
+
+CrashReportDatabase::OperationStatus
+CrashReportDatabaseMac::GetNotUploadedReports(
+    std::vector<const CrashReportDatabase::Report>* reports) {
+  return ReportsInDirectory(base_dir_.Append(kUploadPendingDirectory), reports);
+}
+
+CrashReportDatabase::OperationStatus
+CrashReportDatabaseMac::GetUploadedReports(
+    std::vector<const CrashReportDatabase::Report>* reports) {
+  return ReportsInDirectory(base_dir_.Append(kUploadedDirectory), reports);
+}
+
+CrashReportDatabase::OperationStatus
+CrashReportDatabaseMac::RecordUploadAttempt(const UUID& uuid,
+                                            bool successful,
+                                            const std::string& id) {
+  DCHECK(successful || id.empty());
+
+  base::FilePath report_path = LocateCrashReport(uuid);
+  if (report_path.empty())
+    return kReportNotFound;
+
+  ScopedFileLock lock(ObtainReportLock(report_path));

[Mark Mentovai, 2015/01/08 22:38:10]

The file needs to stay locked while the uploader is trying to upload it,
otherwise someone else can come along and try to upload it at the same time.
This scoper here makes me think that the locking isn’t quite right.

[Robert Sesek, 2015/01/13 16:18:23]

Done. Now the caller has a two-phase call like with generating a new crash
report.

+
+  if (successful) {
+    base::FilePath new_path =
+        base_dir_.Append(kUploadedDirectory).Append(report_path.BaseName());
+    if (rename(report_path.value().c_str(), new_path.value().c_str())) {
+      PLOG(ERROR) << "rename " << report_path.value() << " to "
+                  << new_path.value();
+      return kFileSystemError;
+    }
+    report_path = new_path;
+  }
+
+  if (!WriteXattrBool(report_path, XattrName(kXattrIsUploaded), successful)) {
+    return kDatabaseError;
+  }
+  if (!WriteXattr(report_path, XattrName(kXattrCollectorID), id)) {
+    return kDatabaseError;
+  }
+
+  int upload_attempts = 0;
+  std::string name = XattrName(kXattrUploadAttemptCount);
+  if (HasXattr(report_path, name) &&
+      !ReadXattrInt(report_path, name, &upload_attempts)) {
+    return kDatabaseError;
+  }
+  if (!WriteXattrInt(report_path, name, ++upload_attempts)) {
+    return kDatabaseError;
+  }
+
+  if (!WriteXattrTimeT(report_path,

[Mark Mentovai, 2015/01/08 22:38:10]

I’d set the time_t before doing the upload_attempts maintenance. The
upload_attempts thing can fail if the existing upload_attempts xattr isn’t an
int, and that’s fine, but at least you’ll have stored the time_t first in case
anyone cares.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+                       XattrName(kXattrLastUploadTime),
+                       time(nullptr))) {
+    return kDatabaseError;
+  }
+
+  return kNoError;
+}
+
+base::FilePath CrashReportDatabaseMac::LocateCrashReport(const UUID& uuid) {
+  const std::string target_uuid = uuid.ToString();
+  NSString* path_ns_string = base::SysUTF8ToNSString(base_dir_.value());
+  NSString* dump_extension =
+      [NSString stringWithUTF8String:kCrashReportFileExtension];
+  NSDirectoryEnumerator* dir_it =
+      [[NSFileManager defaultManager] enumeratorAtPath:path_ns_string];

[Mark Mentovai, 2015/01/08 22:38:09]

Directory enumeration is kind of heavy. Why isn’t this just looking for the file
at its two known locations?

(That’d also make it easy to have a consistency check here: it shouldn’t exist
in both locations.)

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+  NSString* file = nil;
+  while ((file = [dir_it nextObject])) {
+    if (![[file pathExtension] isEqualToString:dump_extension])
+      continue;
+
+    base::FilePath full_path =
+        base_dir_.Append([file fileSystemRepresentation]);
+
+    ScopedFileLock lock(ObtainReportLock(full_path));
+    std::string uuid_string;
+    if (ReadXattr(full_path, XattrName(kXattrUUID), &uuid_string) &&
+        uuid_string == target_uuid) {
+      return full_path;
+    }
+  }
+  return base::FilePath();
+}
+
+ScopedFileLock CrashReportDatabaseMac::ObtainReportLock(
+    const base::FilePath& path) {
+  int fd = HANDLE_EINTR(open(path.value().c_str(),
+                             O_RDONLY | O_EXLOCK | O_CLOEXEC));
+  PCHECK(fd >= 0) << "open lock " << path.value();

[Mark Mentovai, 2015/01/08 22:38:10]

CHECK is a little harsh here:

1. The filesystem might not support locks. OK, so maybe you’d say “we don’t
support running on that filesystem right now, so doing anything other than a
CHECK wouldn’t help.” Maybe.

More importantly…

2. The whole point of the locking is to keep multiple invocations of this code
from stepping on each other, but there’s no guarantee on anything about |path|
until the lock is actually obtained. There isn’t even a guarantee that anything
will continue to exist at |path|. So if two of these guys are working in the
same directory at once, and one of them is actively working on a report and
decides to rename it right as the other one tries to take a lock on it, then
they’ll race and the second one could fail and abort.

There’s another problem here: what happens when you do an open with O_EXLOCK?
Does the kernel treat the open and lock as a single atomic operation, or does it
open the file at its path and then block to take a lock? I think it’s the
latter, and that’s also bad. Here’s why: if the same two guys are working in the
same directory, and are trying to upload a report, the first one’s holding a
lock and uploading the report, and then the second one comes along and also
tries to take a lock. It blocks. The first guy then renames the file, let’s say
because the upload succeeded, and it does some bookkeeping and unlocks it. Now
the second guy wins the lock he was waiting for, but he’s under the mistaken
impression that this report hasn’t been uploaded yet.

[Robert Sesek, 2015/01/13 16:18:23]

1) Callers now check the result of ObtainReportLock() and return an error if
that it fails.

2) I don't think the atomicity of open with O_EXLOCK is actually relevant, but
the blocking nature is. In the scenario you outline, the process would error out
when reading extended attributes because the file would have moved, but it's
wrong to force this (indirect) sanity check on the caller. I switched this open
to be O_NONBLOCK, which, when combined with the new result checking, causes this
to error out early in a safe manner.

+  return ScopedFileLock(fd);
+}
+
+bool CrashReportDatabaseMac::ReadReportMetadataLocked(

[Mark Mentovai, 2015/01/08 22:38:09]

This one can be static.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+    const base::FilePath& path, Report* report) {
+  std::string uuid_string;
+  if (!ReadXattr(path, XattrName(kXattrUUID), &uuid_string) ||
+      !report->uuid.InitializeFromString(uuid_string)) {
+    return false;
+  }
+  if (!HasXattr(path, XattrName(kXattrCollectorID))) {

[Mark Mentovai, 2015/01/08 22:38:10]

HasXattr(), ReadXattr(), HasXattr(), ReadXattr(), and so on.

“test for something” and “get something” is a sign that the interface isn’t
doing all that it should.

The xattr interface should have a way to handle this. It’d be better if you only
had to call getxattr() once and were able to distinguish between success,
ENOATTR, and other failures. Maybe it should return a 3-state enum.

Perhaps this would render HasXattr() unnecessary altogether.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+    report->id = std::string();
+  } else if (!ReadXattr(path, XattrName(kXattrCollectorID), &report->id)) {
+    return false;
+  }
+  if (!HasXattr(path, XattrName(kXattrIsUploaded))) {
+    report->uploaded = false;
+  } else if (!ReadXattrBool(path, XattrName(kXattrIsUploaded),
+                            &report->uploaded)) {
+    return false;
+  }
+  if (!HasXattr(path, XattrName(kXattrLastUploadTime))) {
+    report->last_upload_attempt_time = 0;
+  } else if (!ReadXattrTimeT(path, XattrName(kXattrLastUploadTime),
+                             &report->last_upload_attempt_time)) {
+    return false;
+  }
+  if (!HasXattr(path, XattrName(kXattrUploadAttemptCount))) {
+    report->upload_attempts = 0;
+  } else if (!ReadXattrInt(path, XattrName(kXattrUploadAttemptCount),
+                           &report->upload_attempts)) {
+    return false;
+  }
+
+  return true;
+}
+
+CrashReportDatabase::OperationStatus
+CrashReportDatabaseMac::ReportsInDirectory(

[Mark Mentovai, 2015/01/08 22:38:09]

This one can be static too.

[Robert Sesek, 2015/01/13 16:18:22]

Done.

+    const base::FilePath& path,
+    std::vector<const CrashReportDatabase::Report>* reports) {
+  DCHECK(reports->empty());
+
+  NSError* error = nil;
+  NSArray* paths = [[NSFileManager defaultManager]
+      contentsOfDirectoryAtPath:base::SysUTF8ToNSString(path.value())
+                          error:&error];

[Mark Mentovai, 2015/01/08 22:38:10]

Not really sure that this is any better than opendir()/readdir()/closedir(),
since you need a loop to stick things into the vector anyway.

[Robert Sesek, 2015/01/13 16:18:23]

I don't have to manage the DIR* or handle dot-dirs.

+  if (error) {
+    LOG(ERROR) << "Failed to enumerate reports in directory " << path.value()
+               << ": " << [[error description] UTF8String];
+    return kFileSystemError;
+  }
+
+  reports->reserve([paths count]);
+  for (NSString* entry in paths) {
+    base::FilePath report_path = path.Append([entry fileSystemRepresentation]);
+    ScopedFileLock lock(ObtainReportLock(report_path));
+    Report report;
+    if (!ReadReportMetadataLocked(report_path, &report)) {
+      LOG(ERROR) << "Failed to read report metadata for "

[Mark Mentovai, 2015/01/08 22:38:09]

Since you continue on past this, it’s more of a WARNING than an ERROR.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+                 << report_path.value();
+      continue;
+    }
+    reports->push_back(report);
+  }
+
+  return kNoError;
+}
+
+// static
+std::string CrashReportDatabaseMac::XattrName(const base::StringPiece& name) {
+  return base::StringPrintf("com.google.crashpad.%s", name.data());

[Mark Mentovai, 2015/01/08 22:38:10]

org.googlecode

[Mark Mentovai, 2015/01/09 14:26:09]

I wrote:
com.googlecode, I mean.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+}
+
+}  // namespace
+
+// static
+scoped_ptr<CrashReportDatabase> CrashReportDatabase::Initialize(
+    const base::FilePath& path) {
+  scoped_ptr<CrashReportDatabase> database;
+
+  NSFileManager* file_manager = [NSFileManager defaultManager];

[Mark Mentovai, 2015/01/08 22:38:10]

I don’t think you need this check, because CrashReportDatabaseMac will do the
same thing.

[Robert Sesek, 2015/01/13 16:18:23]

Done.

+  BOOL is_dir = NO;
+  if ([file_manager fileExistsAtPath:base::SysUTF8ToNSString(path.value())
+                         isDirectory:&is_dir] &&
+      !is_dir) {
+    LOG(ERROR) << "File exists at " << path.value()
+               << " but is not a directory";
+    return database;
+  }
+
+  scoped_ptr<CrashReportDatabaseMac> database_mac(
+      new CrashReportDatabaseMac(path.Append(kDatabaseDirectoryName)));
+  if (!database_mac->Initialize())
+    database_mac.reset();
+
+  database.reset(database_mac.release());
+  return database;
+}
+
+}  // namespace crashpad