win: Implementation of CrashReportDatabase for Windows

client/crash_report_database_win.cc

+// 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 <time.h>
+#include <windows.h>
+
+#include <limits>
+
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "util/misc/uuid.h"
+
+namespace crashpad {
+
+namespace {
+
+const wchar_t kDatabaseDirectoryName[] = L"Crashpad";
+
+const wchar_t kReportsDirectory[] = L"reports";
+const wchar_t kMetadataFileName[] = L"metadata";
+const wchar_t kMetadataTempFileName[] = L"metadata.tmp";
+const wchar_t kMetadataMutexName[] = L"crashpad.metadata.lock";
+
+const wchar_t kCrashReportFileExtension[] = L"dmp";
+
+enum class ReportState : int {
+  //! \brief Just created, owned by caller and being written.
+  kNew,
+  //! \brief Fully filled out, owned by database.
+  kPending,
+  //! \brief In the process of uploading, owned by caller.
+  kUploading,
+  //! \brief Upload completed or skipped, owned by database.
+  kCompleted,
+
+  //! \brief Used during query to indicate a report in any state is acceptable.
+  //!     Not a valid state for a report to be in.
+  kAny,
+};
+
+using OperationStatus = CrashReportDatabase::OperationStatus;
+
+//! \brief Ensures that the node at path is a directory, and creates it if it
+//!     does not exist.
+//!
+//! If the path points to a file, rather than a directory, or the directory

[Robert Sesek, 2015/02/02 23:06:08]

\return ?

[scottmg, 2015/02/03 18:56:39]

Done.

+//! could not be created, returns false. Otherwise, returns true, indicating
+//! that path already was or now is a directory.
+bool CreateOrEnsureDirectoryExists(const base::FilePath& path) {
+  if (CreateDirectory(path.value().c_str(), nullptr)) {
+    return true;
+  } else if (GetLastError() == ERROR_ALREADY_EXISTS) {
+    DWORD fileattr = GetFileAttributes(path.value().c_str());
+    if (fileattr == INVALID_FILE_ATTRIBUTES) {
+      PLOG(ERROR) << "GetFileAttributes";
+      return false;
+    }
+    if ((fileattr & FILE_ATTRIBUTE_DIRECTORY) != 0)
+      return true;
+    LOG(ERROR) << "not a directory";
+    return false;
+  } else {
+    PLOG(ERROR) << "CreateDirectory";
+    return false;
+  }
+}
+
+//! \brief Attempts to convert the given string to an int64. Only succeeds if
+//!     the entire string is consumed.
+bool GetInt64(base::StringPiece str, int64_t* as_int) {
+  char* endptr;
+  std::string as_string = str.as_string();
+  *as_int = _strtoi64(as_string.c_str(), &endptr, 10);
+  return endptr == &as_string.end()[0];
+}
+
+//! \brief Splits the given string on a the given character, returning a vector
+//!     of StringPieces. The lifetime of the input string must therefore outlive
+//!     the call to this function.
+std::vector<base::StringPiece> SplitString(base::StringPiece str, char on) {
+  std::vector<base::StringPiece> result;
+  size_t last = 0;
+  size_t end = str.size();
+  for (size_t i = 0; i <= end; ++i) {
+    if (i == end || str[i] == on) {
+      result.push_back(base::StringPiece(str.begin() + last, i - last));
+      last = i + 1;
+    }
+  }
+  return result;
+}
+
+//! \brief A private extension of the Report class that includes additional data
+//!     that's stored on disk in the metadata file.
+struct ReportDisk : public CrashReportDatabase::Report {
+  //! \brief The current state of the report.
+  ReportState state;
+};
+
+//! \brief A private extension of the NewReport class to hold the UUID during
+//!     initial write. We don't store metadata in dump's file attributes, and
+//!     use the UUID to identify the dump on write completion.
+struct NewReportDisk : public CrashReportDatabase::NewReport {
+  //! \brief The UUID for this crash report.
+  UUID uuid;
+};
+
+//! \brief Manages the metadata for the set of reports, handling serialization
+//!     to disk, and queries.

[Robert Sesek, 2015/02/02 23:06:08]

Mention that this class shouldn't be instantiated directly but through
AcquireMetadata() instead?

[scottmg, 2015/02/03 18:56:40]

Done.

+class Metadata {
+ public:
+  //! \brief mutex_to_release must already be acquired. The lock is passed to

[Robert Sesek, 2015/02/02 23:06:08]

Use \param instead?

[scottmg, 2015/02/03 18:56:39]

Done.

+  //!     this object and will be released on destruction.
+  Metadata(const base::FilePath& base_dir, HANDLE mutex_to_release);
+  ~Metadata();
+
+  //! \brief Reads the database from disk. May only be called once without an
+  //!     interceding call to Write().
+  bool Read();
+
+  //! \brief Flushes the database back to disk. It is not necessary to call this
+  //!     method the reports were not mutated, however, Read() may not be called

[Robert Sesek, 2015/02/02 23:06:08]

Missing a word in this sentence.

[scottmg, 2015/02/03 18:56:39]

Done.

+  //!     again until Write() is used.
+  bool Write();
+
+  //! \brief Adds a new report to the set.

[Robert Sesek, 2015/02/02 23:06:08]

"Must call Write() to persist" ?

[scottmg, 2015/02/03 18:56:39]

Done.

+  void AddNewRecord(const NewReportDisk& new_report);
+
+  //! \brief Finds all reports in a given state. The reports vector is only
+  //!     valid when #kNoError is returned.
+  OperationStatus FindReports(
+      ReportState desired_state,
+      std::vector<const CrashReportDatabase::Report>* reports);
+
+  //! \brief Finds the report matching the given UUID and the desired state,
+  //!     returns a mutable pointer to it. Only valid if #kNoError is returned.
+  //!     If the report is mutated by the caller, the caller is also responsible
+  //!     for calling Write() before destruction.

[Robert Sesek, 2015/02/02 23:06:08]

Is the caller also responsible for destruction? \param comments would help.

[scottmg, 2015/02/03 18:56:39]

Done.

+  OperationStatus Metadata::FindSingleReport(const UUID& uuid,
+                                             ReportState desired_state,
+                                             ReportDisk** report_disk);
+
+ private:
+  //! \brief Confirms that the corresponding report actually exists on disk
+  //!     (that is, the dump file has not been removed), and if desired_state
+  //!     is not ReportState::kAny, confirms that the report is in the given
+  //!     state.
+  static OperationStatus VerifyReport(const ReportDisk& report_win,
+                                      ReportState desired_state);
+
+  base::FilePath base_dir_;
+  bool loaded_;
+  HANDLE mutex_;
+  std::vector<ReportDisk> reports_;
+};
+
+Metadata::Metadata(const base::FilePath& base_dir, HANDLE mutex_to_release)
+    : base_dir_(base_dir), loaded_(false), mutex_(mutex_to_release) {

[Robert Sesek, 2015/02/02 23:06:08]

, reports_() for consistency with full member initialization.

[scottmg, 2015/02/03 18:56:39]

Done.

+}
+
+Metadata::~Metadata() {
+  ReleaseMutex(mutex_);
+}
+
+// The format of the metadata file is:
+//
+// uint32_t num_bytes_in_file
+// "crashpad metadata\n"
+// decimal integer "\n" -- file format version, currently "1".
+//
+// The remainder of the file is a variable number of lines, one for each report.
+// Each line is terminated by a single \n. Elements in each line are separated
+// by \t, and each element corresponds to the fields of `ReportDisk`. There is
+// no terminator field, as the initial file length is sufficient to determine
+// the number of records.
+
+bool Metadata::Read() {
+  DCHECK(!loaded_);
+  base::FilePath path = base_dir_.Append(kMetadataFileName);
+  ScopedFileHandle input_file(ScopedFileHandle(LoggingOpenFileForRead(path)));

[Robert Sesek, 2015/02/02 23:06:08]

Why do you need to invoke ScopedFileHandle()'s copy constructor here?

[scottmg, 2015/02/03 18:56:39]

Do you mean why is it not written as ScopedFileHandle input_file =
ScopedFileHandle(...);?

The ScopedFileHandle constructor is explicit but LoggingOpenFileForRead returns
a FileHandle, so there needs to be an explicit construction. But no good reason
why I used copy construction rather than assignment.

[Robert Sesek, 2015/02/05 18:43:09]

Ah, I missed that the ctor was explicit. You can use auto here to make this a
little shorter.

[scottmg, 2015/02/05 19:27:55]

Done.

+  // The file not existing is OK, we may not have created it yet.
+  if (input_file.get() == INVALID_HANDLE_VALUE) {
+    loaded_ = true;
+    return true;
+  }
+
+  uint32_t num_bytes;
+  if (!LoggingReadFile(input_file.get(), &num_bytes, sizeof(num_bytes)))
+    return false;
+  scoped_ptr<char[]> buffer(new char[num_bytes]);
+  if (!LoggingReadFile(input_file.get(), buffer.get(), num_bytes))
+    return false;
+
+  base::StringPiece contents(buffer.get(), num_bytes);
+  std::vector<base::StringPiece> lines(SplitString(contents, '\n'));
+  // TODO(scottmg): Add StringPiece(string), and ==, != to avoid some of the
+  // as_string() calls.
+  if (lines[0].as_string() != "crashpad metadata")

[Robert Sesek, 2015/02/03 16:29:29]

I thought about this a little more. It's a bit odd to mix binary (for the file
size) and text (for the data) encoding schemes, but I can see how that'd be
handy. If we are going to keep a mixed binary-ASCII format, then I think this
magic cookie should actually come first and, followed by the version, and then
the filesize—all binary packed, too. Something like:

[magic uint32][version uint16][filesize uint32]
'CPAD''01''nnnn'

And then speaking towards the corruption comment bit: maybe the another uint32
follows that, which computes CRC32 over the data that comes after. That's still
pretty rudimentary, and it'd probably be better to compute CRC32s over chunks of
the file (so that if one line is corrupt, the entire file isn't junked, but only
that one chunk of N lines would be), but I'm not sure how much of an issue
corruption really would be.

[scottmg, 2015/02/03 18:56:39]

I don't think there's a risk of corruption given the write-and-swap.

I would have preferred a completely binary fixed-size record format, but that
would necessitate an arbitrary limit on the filepath (probably OK on Windows as
the limit is already relatively short) and the id (less fine, as I'm not
entirely certain what that will be used for.)

If we pick _MAX_PATH for both == 260, we get a record metadata size of 
16+260+260+8+4+8+4+4 == 564 bytes per record. For say 100 crash reports, maybe a
~50k file is fine, and then it could be completely binary and this code could
probably be a bit simpler. On the other hand, there'd still need to be
deserialization code to map between structures because the disk structure would
be fixed size but the in memory derived from Report with instantiated objects.

Another possibility would be writing a header with

[magic][version][file size][num_records]

then num_records fixed size records with file_path and id being uint32_t indices

followed by a string table referenced by the above indices, each entry \0
terminated.

Maybe that's the cleanest as it's all binary, has no short string length limits,
avoids str->int parsing, and avoids the \t \n edge cases. The only drawback is
the file is somewhat less inspectable, but it wasn't really that readable
anyway.

I think I'd lean towards the last option. What do you think?

+    return false;
+  int64_t version;

[Robert Sesek, 2015/02/02 23:06:08]

int32_t isn't big enough for the version, but uint32_t is for the file size?

[scottmg, 2015/02/03 18:56:39]

Yeah, was just to avoid another string->int function.

+  if (!GetInt64(lines[1], &version) || version != 1)
+    return false;
+  // To lines - 1 because there'll be an empty line due to trailing \n.
+  for (size_t i = 2; i < lines.size() - 1; ++i) {
+    ReportDisk r;
+    std::vector<base::StringPiece> parts(SplitString(lines[i], '\t'));
+    if (parts.size() != 8)
+      return false;

[Robert Sesek, 2015/02/02 23:06:08]

Would it be useful to LOG(ERROR) in these conditions?

+    if (!r.uuid.InitializeFromString(parts[0]))
+      return false;
+    r.file_path = base::FilePath(base::UTF8ToUTF16(parts[1].as_string()));
+    r.id = parts[2].as_string();
+    if (!GetInt64(parts[3], &r.creation_time))
+      return false;
+    int64_t uploaded;
+    if (!GetInt64(parts[4], &uploaded) || (uploaded != 0 && uploaded != 1))
+      return false;
+    r.uploaded = static_cast<bool>(uploaded);
+    if (!GetInt64(parts[5], &r.last_upload_attempt_time))
+      return false;
+    int64_t upload_attempts;
+    if (!GetInt64(parts[6], &upload_attempts) ||
+        upload_attempts > std::numeric_limits<int>::max()) {

[Robert Sesek, 2015/02/02 23:06:08]

|| upload_attempts < 0 , too?

[scottmg, 2015/02/03 18:56:39]

Good catch, done.

+      return false;
+    }
+    r.upload_attempts = static_cast<int>(upload_attempts);
+    int64_t state;
+    if (!GetInt64(parts[7], &state))
+      return false;
+    r.state = static_cast<ReportState>(state);
+    CHECK(r.state != ReportState::kAny);

[Robert Sesek, 2015/02/02 23:06:08]

This CHECKs but the rest return false.

[scottmg, 2015/02/03 18:56:39]

Done.

+    reports_.push_back(r);
+  }
+  loaded_ = true;
+  return true;
+}
+
+bool Metadata::Write() {
+  DCHECK(loaded_);
+  base::FilePath path = base_dir_.Append(kMetadataTempFileName);
+
+  // Write data to a temporary file.
+  {
+    ScopedFileHandle output_file(ScopedFileHandle(LoggingOpenFileForWrite(
+        path, FileWriteMode::kTruncateOrCreate, FilePermissions::kOwnerOnly)));
+    std::string to_output = "crashpad metadata\n1\n";

[Robert Sesek, 2015/02/02 23:06:08]

Move "crashpad metadata" and the version number to constants at the top of the
file? Then you can use them on lines 214 and 217.

+    for (const auto& report : reports_) {
+      // All the other fields are internal, but make sure the client does not
+      // use our separators in |id|.
+      DCHECK(report.id.find_first_of("\t\n") == std::string::npos)
+          << "id may not contain \\t or \\n";
+      to_output += base::StringPrintf(
+          "%s\t"
+          "%s\t"
+          "%s\t"
+          "%I64d\t"
+          "%d\t"
+          "%I64d\t"
+          "%d\t"
+          "%d\n",
+          report.uuid.ToString().c_str(),
+          base::UTF16ToUTF8(report.file_path.value()).c_str(),

[Robert Sesek, 2015/02/02 23:06:08]

Is it legal for Windows filepaths/names to contain \n or \t?

[scottmg, 2015/02/03 18:56:40]

No, nothing less than integer value 31 is valid.
https://msdn.microsoft.com/en-ca/library/windows/desktop/aa365247(v=vs.85).as...

+          report.id.c_str(),
+          report.creation_time,
+          report.uploaded,
+          report.last_upload_attempt_time,
+          report.upload_attempts,
+          report.state);
+    }
+    uint32_t num_bytes = static_cast<uint32_t>(to_output.size());
+    if (!LoggingWriteFile(output_file.get(), &num_bytes, sizeof(num_bytes)))
+      return false;
+    if (!LoggingWriteFile(output_file.get(), to_output.c_str(), num_bytes))
+      return false;
+  }
+
+  // While this is not guaranteed to be atomic, it is in practical cases. We
+  // aren't racing ourself here as we're holding the mutex here, we use
+  // MoveFileEx only to avoid corrupting data in case of failure during write.
+  bool result = MoveFileEx(path.value().c_str(),
+                           base_dir_.Append(kMetadataFileName).value().c_str(),
+                           MOVEFILE_REPLACE_EXISTING);
+  loaded_ = !result;
+  return result;
+}
+
+void Metadata::AddNewRecord(const NewReportDisk& new_report) {
+  ReportDisk report;
+  report.uuid = new_report.uuid;
+  report.file_path = new_report.path;
+  report.state = ReportState::kNew;
+  reports_.push_back(report);
+}
+
+// static
+OperationStatus Metadata::VerifyReport(const ReportDisk& report_win,

[Robert Sesek, 2015/02/02 23:06:08]

This should be below FindSingleReport, based on the declaration order.

[scottmg, 2015/02/03 18:56:39]

Done.

+                                       ReportState desired_state) {
+  if (desired_state != ReportState::kAny && report_win.state != desired_state)
+    return CrashReportDatabase::kBusyError;
+  if (GetFileAttributes(report_win.file_path.value().c_str()) ==
+      INVALID_FILE_ATTRIBUTES) {
+    return CrashReportDatabase::kReportNotFound;
+  }
+  return CrashReportDatabase::kNoError;
+}
+
+OperationStatus Metadata::FindReports(
+    ReportState desired_state,
+    std::vector<const CrashReportDatabase::Report>* reports) {
+  DCHECK(reports->empty());
+  for (const auto& report : reports_) {
+    if (report.state == desired_state) {
+      if (VerifyReport(report, desired_state) != CrashReportDatabase::kNoError)
+        continue;
+      reports->push_back(report);
+    }
+  }
+  return CrashReportDatabase::kNoError;
+}
+
+OperationStatus Metadata::FindSingleReport(const UUID& uuid,
+                                           ReportState desired_state,
+                                           ReportDisk** out_report) {
+  for (size_t i = 0; i < reports_.size(); ++i) {
+    if (reports_[i].uuid == uuid) {
+      OperationStatus os = VerifyReport(reports_[i], desired_state);
+      if (os != CrashReportDatabase::kNoError)
+        return os;
+      *out_report = &reports_[i];
+      return CrashReportDatabase::kNoError;
+    }
+  }
+  return CrashReportDatabase::kReportNotFound;
+}
+
+class CrashReportDatabaseWin : public CrashReportDatabase {
+ public:
+  explicit CrashReportDatabaseWin(const base::FilePath& path);
+  virtual ~CrashReportDatabaseWin();

[Robert Sesek, 2015/02/02 23:06:07]

virtual -> override

[scottmg, 2015/02/03 18:56:40]

Done.

+
+  bool Initialize();
+
+  // CrashReportDatabase::
+  OperationStatus PrepareNewCrashReport(NewReport** report) override;
+  OperationStatus FinishedWritingCrashReport(NewReport* report,
+                                             UUID* uuid) override;
+  OperationStatus LookUpCrashReport(const UUID& uuid, Report* report) override;
+  OperationStatus GetPendingReports(
+      std::vector<const Report>* reports) override;
+  OperationStatus GetCompletedReports(
+      std::vector<const Report>* reports) override;
+  OperationStatus GetReportForUploading(const UUID& uuid,
+                                        const Report** report) override;
+  OperationStatus RecordUploadAttempt(const Report* report,
+                                      bool successful,
+                                      const std::string& id) override;
+  OperationStatus SkipReportUpload(const UUID& uuid) override;
+
+ private:
+  scoped_ptr<Metadata> AcquireMetadata();
+
+  base::FilePath base_dir_;
+  ScopedKernelHANDLE mutex_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrashReportDatabaseWin);
+};
+
+CrashReportDatabaseWin::CrashReportDatabaseWin(const base::FilePath& path)
+    : CrashReportDatabase(), base_dir_(path) {

[Robert Sesek, 2015/02/02 23:06:08]

, mutex_()

[scottmg, 2015/02/03 18:56:39]

Done.

+}
+
+CrashReportDatabaseWin::~CrashReportDatabaseWin() {
+}
+
+bool CrashReportDatabaseWin::Initialize() {
+  // Check if the database already exists.
+  if (!CreateOrEnsureDirectoryExists(base_dir_))
+    return false;
+
+  // Create our reports subdirectory.
+  if (!CreateOrEnsureDirectoryExists(base_dir_.Append(kReportsDirectory)))
+    return false;
+
+  mutex_.reset(CreateMutex(nullptr, false, kMetadataMutexName));

[Robert Sesek, 2015/02/02 23:06:08]

What will happen if multiple, different Crashpad-based products try to do this?

[scottmg, 2015/02/03 18:56:40]

This call will succeed and behaviour will be correct, but the mutual exclusion
will be shared across all products so there would be reduced concurrency.

+
+  // TODO(scottmg): When are completed reports pruned from disk? Delete here or
+  // maybe on Read().
+
+  return true;
+}
+
+OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport(
+    NewReport** out_report) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;

[Robert Sesek, 2015/02/02 23:06:08]

The documentation for kFileSystemError says that additional information will be
logged, so that ties in with the comment at line 224.

+
+  scoped_ptr<NewReportDisk> report(new NewReportDisk());
+
+  ::UUID system_uuid;
+  if (UuidCreate(&system_uuid) != RPC_S_OK) {
+    return kFileSystemError;
+  }
+  static_assert(sizeof(system_uuid) == 16, "unexpected system uuid size");

[Robert Sesek, 2015/02/02 23:06:08]

Would these be better in uuid.cc ?

[scottmg, 2015/02/03 18:56:39]

It's not strictly required by uuid.cc I guess (i.e. would could convert to a
byte buffer here if required).

+  static_assert(offsetof(::UUID, Data1) == 0, "unexpected uuid layout");
+  UUID uuid(reinterpret_cast<const uint8_t*>(&system_uuid.Data1));
+
+  report->uuid = uuid;
+  report->path =
+      base_dir_.Append(kReportsDirectory)
+          .Append(uuid.ToWideString() + L"." + kCrashReportFileExtension);
+  report->handle = LoggingOpenFileForWrite(report->path,
+                                           FileWriteMode::kTruncateOrCreate,
+                                           FilePermissions::kOwnerOnly);
+  if (report->handle == INVALID_HANDLE_VALUE)
+    return kFileSystemError;
+
+  metadata->AddNewRecord(*report);
+  if (!metadata->Write())
+    return kDatabaseError;
+  *out_report = report.release();
+  return kNoError;
+}
+
+OperationStatus CrashReportDatabaseWin::FinishedWritingCrashReport(
+    NewReport* report,
+    UUID* uuid) {
+  // Take ownership of the report, and cast to our private version with UUID.
+  scoped_ptr<NewReportDisk> scoped_report(static_cast<NewReportDisk*>(report));
+  // Take ownership of the file handle.
+  ScopedFileHandle handle(report->handle);
+
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+
+  ReportDisk* report_disk;
+  OperationStatus os = metadata->FindSingleReport(
+      scoped_report->uuid, ReportState::kNew, &report_disk);
+  if (os != kNoError)
+    return os;
+  report_disk->state = ReportState::kPending;
+  report_disk->creation_time = time(nullptr);
+  if (!metadata->Write())
+    return kDatabaseError;
+  *uuid = report_disk->uuid;
+  return kNoError;
+}
+
+OperationStatus CrashReportDatabaseWin::LookUpCrashReport(const UUID& uuid,
+                                                          Report* report) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+
+  ReportDisk* report_disk;
+  OperationStatus os =
+      metadata->FindSingleReport(uuid, ReportState::kAny, &report_disk);
+  if (os != kNoError)
+    return os;
+  *report = *report_disk;
+  return kNoError;
+}
+
+OperationStatus CrashReportDatabaseWin::GetPendingReports(
+    std::vector<const Report>* reports) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+  return metadata->FindReports(ReportState::kPending, reports);
+}
+
+OperationStatus CrashReportDatabaseWin::GetCompletedReports(
+    std::vector<const Report>* reports) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+  return metadata->FindReports(ReportState::kCompleted, reports);
+}
+
+OperationStatus CrashReportDatabaseWin::GetReportForUploading(
+    const UUID& uuid,
+    const Report** report) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+
+  ReportDisk* report_disk;
+  OperationStatus os =
+      metadata->FindSingleReport(uuid, ReportState::kPending, &report_disk);
+  if (os != kNoError)
+    return os;
+  report_disk->state = ReportState::kUploading;
+  // Create a copy for passing back to client. This will be freed in
+  // RecordUploadAttempt.
+  *report = new Report(*report_disk);
+  if (!metadata->Write())
+    return kDatabaseError;
+  return kNoError;
+}
+
+OperationStatus CrashReportDatabaseWin::RecordUploadAttempt(
+    const Report* report,
+    bool successful,
+    const std::string& id) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+
+  // Take ownership, allocated in GetReportForUploading.

[Robert Sesek, 2015/02/02 23:06:08]

Should this happen before the potential early return on 524?

[scottmg, 2015/02/03 18:56:39]

Indeed! Done.

+  scoped_ptr<const Report> upload_report(report);
+
+  ReportDisk* report_disk;
+  OperationStatus os = metadata->FindSingleReport(
+      report->uuid, ReportState::kUploading, &report_disk);
+  if (os != kNoError)
+    return os;
+  report_disk->uploaded = successful;
+  report_disk->id = id;
+  report_disk->last_upload_attempt_time = time(nullptr);
+  report_disk->upload_attempts++;
+  report_disk->state =
+      successful ? ReportState::kCompleted : ReportState::kPending;
+  if (!metadata->Write())
+    return kDatabaseError;
+  return kNoError;
+}
+
+OperationStatus CrashReportDatabaseWin::SkipReportUpload(const UUID& uuid) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+
+  ReportDisk* report_disk;
+  OperationStatus os =
+      metadata->FindSingleReport(uuid, ReportState::kPending, &report_disk);
+  if (os != kNoError)
+    return os;
+  report_disk->state = ReportState::kCompleted;
+  if (!metadata->Write())
+    return kDatabaseError;
+  return kNoError;
+}
+
+scoped_ptr<Metadata> CrashReportDatabaseWin::AcquireMetadata() {
+  if (WaitForSingleObject(mutex_.get(), INFINITE) != WAIT_OBJECT_0)
+    return nullptr;
+  scoped_ptr<Metadata> metadata(new Metadata(base_dir_, mutex_.get()));
+  if (!metadata->Read())
+    return nullptr;
+  return metadata;
+}
+
+}  // namespace
+
+// static
+scoped_ptr<CrashReportDatabase> CrashReportDatabase::Initialize(
+    const base::FilePath& path) {
+  scoped_ptr<CrashReportDatabaseWin> database_win(
+      new CrashReportDatabaseWin(path.Append(kDatabaseDirectoryName)));
+  if (!database_win->Initialize())
+    database_win.reset();
+
+  return scoped_ptr<CrashReportDatabase>(database_win.release());
+}
+
+}  // namespace crashpad