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 <rpc.h>
+#include <time.h>
+#include <windows.h>
+
+#include <limits>
+
+#include "base/files/file_path.h"

[Mark Mentovai, 2015/02/06 21:13:23]

Since crash_report_database.h is the top #include and it has this, scoped_ptr.h,
and uuid.h, you don’t need to duplicate the #includes here.

[scottmg, 2015/02/06 22:58:18]

Done.

+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/numerics/safe_math.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.
+//!
+//! \return If the path points to a file, rather than a directory, or the
+//!     directory 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 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. Instances of this class should be created by using
+//!     CrashReportDatabaseWin::AcquireMetadata(), rather than direct
+//!     instantiation.
+class Metadata {
+ public:
+  //! \param[in] base_dir Root of storage for data files.
+  //! \param[in] mutex_to_release An acquired mutex guarding access to the
+  //!     metadata file. The lock is owned by this object and will be released
+  //!     on destruction. The lifetime of the HANDLE is not owned.
+  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().
+  //!
+  //! \return true on success. If unsuccessful, the object is not in a usable
+  //!     state.
+  bool Read();
+
+  //! \brief Flushes the database back to disk. It is not necessary to call this
+  //!     method if the reports were not mutated, however, Read() may not be
+  //!     called again until Write() is used.
+  //!
+  //! \return true on success.
+  bool Write();
+
+  //! \brief Adds a new report to the set. Write() must be called to persist
+  //!     after adding new records.

[Mark Mentovai, 2015/02/06 21:13:23]

Does Read() need to have been called first? If so, document it here and add
DCHECK(loaded_) at the top of the method. Is it OK to call this after Write()?
It’s unclear. The same questions apply to the other methods in this class. If
the answers are the same for all of them, then the documentation should be on
Read()/Write() instead of on each individual method.

[scottmg, 2015/02/06 22:58:17]

Done with DCHECKs.

+  void AddNewRecord(const NewReportDisk& new_report);
+
+  //! \brief Finds all reports in a given state. The reports vector is only

[Mark Mentovai, 2015/02/06 21:13:23]

“The \a reports vector”

[scottmg, 2015/02/06 22:58:18]

Done.

+  //!     valid when #kNoError is returned.

[Mark Mentovai, 2015/02/06 21:13:23]

\param[out] reports should mention that reports must be empty on call.

[scottmg, 2015/02/06 22:58:18]

Done.

+  OperationStatus FindReports(
+      ReportState desired_state,
+      std::vector<const CrashReportDatabase::Report>* reports);
+
+  //! \brief Finds the report matching the given UUID and the desired state,

[Mark Mentovai, 2015/02/06 21:13:23]

This isn’t exactly brief, and the first sentence is missing something (maybe
“and?”) \briefs are usually just a summary of what something is supposed to do.
Nailing down the exact error conditions and caller expectations is best left to
its own paragraph. A few of the other comments should be revised similarly.

In our current configuration, Doxygen doesn’t look inside unnamed namespaces or
private: sections when producing formatted output. I’m still glad you wrote the
documentation, it helps understand the code.

[scottmg, 2015/02/06 22:58:18]

Done.

+  //!     returns a mutable pointer to it. Only valid if
+  //!     CrashReportDatabase::kNoError is returned. If the report is mutated by
+  //!     the caller, the caller is also responsible for calling Write() before
+  //!     destruction to persist the changes.
+  //!
+  //! \param[in] uuid The report identifier.
+  //! \param[in] desired_state A specific state that the given report must be
+  //!     in. If the report is located, but is in a different state,
+  //!     CrashReportDatabase::kBusyError will be returned. If no filtering by
+  //!     state is required, ReportState::kAny can be used.
+  //! \param[out] report_disk The found report, valid only if
+  //!     CrashReportDatabase::kNoError is returned. Ownership is not
+  //!     transferred to the caller.
+  OperationStatus FindSingleReport(const UUID& uuid,
+                                   ReportState desired_state,
+                                   ReportDisk** report_disk);
+
+  //! \brief Removes a single report identified by UUID. This removes both the
+  //!     entry in the metadata database, as well as the on-disk report, if any.
+  //!     Write() must be called to persist when CrashReportDatabase::kNoError
+  //!     is returned.
+  //!
+  //! \param[in] uuid The report identifier.
+  OperationStatus RemoveByUUID(const UUID& uuid);
+
+ 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),
+      reports_() {
+}
+
+Metadata::~Metadata() {
+  ReleaseMutex(mutex_);
+}
+
+// The format of the metadata file is a MetadataFileHeader, followed by a
+// number of fixed size records of MetadataFileReportRecord, followed by a
+// string table in UTF8 format, where each string is \0 terminated.
+
+#pragma pack(push, 1)
+
+struct MetadataFileHeader {
+  uint32_t magic;
+  uint32_t version;
+  uint32_t remaining_file_size;

[Mark Mentovai, 2015/02/06 21:13:23]

Remaining after this field or this structure? (It’s structure, should say so
here.) Is it really necessary to track this in the header at all?

[scottmg, 2015/02/06 22:58:18]

Removed.

+  uint32_t num_records;
+};
+
+struct MetadataFileReportRecord {
+  UUID uuid;  // UUID is a 16 byte, standard layout structure.
+  uint32_t file_path_index;

[Mark Mentovai, 2015/02/06 21:13:23]

Are these byte indices into the file, or indices into the string table? (They’re
into the string table.) Are file paths relative to the database directory? Based
on FileOffset in util/file/file_io.h, you have 64-bit file offsets, so why are
these uint32_t?

[scottmg, 2015/02/06 22:58:18]

The format of file paths depends on how the CrashReportDatabase is
Initialize()d, it appends filenames to that directory, but will be absolute or
relative depending on how it's used.

Yes, the file offsets are 64 bit but if the file was that big this approach
would be completely unusable anyway (repeatedly serializing/deserializing 4G is
not going to work.)

[Mark Mentovai, 2015/02/06 23:29:52]

scottmg wrote:
Can we keep the paths relative to the database root or the reports directory?
The database should be resilient to being moved around trivially.

[scottmg, 2015/02/07 01:21:58]

OK. I've added a (failing) test, but haven't yet modified the path manipulation
to relativize and absolutize.

+  uint32_t id_index;
+  int64_t creation_time;

[Mark Mentovai, 2015/02/06 21:13:23]

time_t epoch or something else? Same on line 208.

[scottmg, 2015/02/06 22:58:17]

Done.

+  uint8_t uploaded;

[Mark Mentovai, 2015/02/06 21:13:24]

0 or 1, or other values?

[scottmg, 2015/02/06 22:58:18]

Done.

+  int64_t last_upload_attempt_time;

[Mark Mentovai, 2015/02/06 21:13:23]

I know you’re pack(1), but let’s try to keep things aligned better in here.
You’ve got a uint8 sandwiched between a pair of uint64s.

[scottmg, 2015/02/06 22:58:17]

Done.

+  int32_t upload_attempts;
+  int32_t state;

[Mark Mentovai, 2015/02/06 21:13:22]

Reference ReportState if that’s what this means.

[scottmg, 2015/02/06 22:58:18]

Done.

+};
+
+const uint32_t kMetadataFileHeaderMagic = 'CPAD';
+const uint32_t kMetadataFileVersion = 1;
+
+#pragma pack(pop)
+
+uint32_t AddStringToTable(std::string* string_table, const std::string& str) {
+  uint32_t offset = base::checked_cast<uint32_t>(string_table->size());
+  *string_table += str;
+  *string_table += '\0';
+  return offset;
+}
+
+uint32_t AddStringToTable(std::string* string_table, const std::wstring& str) {
+  return AddStringToTable(string_table, base::UTF16ToUTF8(str));
+}
+
+bool Metadata::Read() {
+  DCHECK(!loaded_);
+  base::FilePath path = base_dir_.Append(kMetadataFileName);
+  auto input_file = ScopedFileHandle(LoggingOpenFileForRead(path));
+  // The file not existing is OK, we may not have created it yet.
+  if (input_file.get() == INVALID_HANDLE_VALUE) {
+    loaded_ = true;
+    return true;
+  }
+
+  MetadataFileHeader header;
+  if (!LoggingReadFile(input_file.get(), &header, sizeof(header)))
+    return false;
+  if (header.magic != kMetadataFileHeaderMagic ||
+      header.version != kMetadataFileVersion) {
+    return false;

[Mark Mentovai, 2015/02/06 21:13:23]

I wonder about these “return false”s. If the metadata file is corrupt, is crash
reporting totally hosed? Is there a recovery strategy for that case?

[scottmg, 2015/02/06 22:58:17]

There's no recovery strategy. Any existing crash reports would be lost. The user
of the database would need to restart with an empty database.

[Mark Mentovai, 2015/02/06 23:29:52]

scottmg wrote:
The metadata file’s going to get messed up just because, for reasons beyond our
control and best efforts, no matter how careful we are. Recovery should be
internal to this class, even if it means moving the old metadata file aside,
losing everything, and starting a new one from scratch. We can’t afford to stop
reporting crashes forever just because a file was clobbered.

[scottmg, 2015/02/07 01:21:57]

Done.

+  }
+  scoped_ptr<uint8_t> buffer(new uint8_t[header.remaining_file_size]);

[Mark Mentovai, 2015/02/06 21:13:23]

This needs to be scoped_ptr<uint8_t[]> because it’s supervising something
allocated with new[].

[scottmg, 2015/02/06 22:58:17]

Done.

+  if (!LoggingReadFile(
+          input_file.get(), buffer.get(), header.remaining_file_size)) {
+    return false;
+  }
+
+  auto records_size = base::CheckedNumeric<uint32_t>(header.num_records) *
+                      sizeof(MetadataFileReportRecord);
+  if (!records_size.IsValid())
+    return false;
+
+  // Each record will have two NUL-terminated strings in the table.

[Mark Mentovai, 2015/02/06 21:13:23]

They might be pooled, though. Maybe this writer doesn’t pool them, but nothing
else about this reader insists that they not be pooled, so this seems like kind
of a harsh check.

[scottmg, 2015/02/06 22:58:18]

True, removed.

[Robert Sesek, 2015/02/10 20:32:25]

Even if the strings are pooled, though, the table still must be NUL terminated.
That check was lost.

[scottmg, 2015/02/10 20:58:45]

Done.

+  auto min_string_table_size =
+      base::CheckedNumeric<uint32_t>(header.num_records) * 2;
+  auto min_file_size = records_size + min_string_table_size;
+  if (!min_file_size.IsValid())
+    return false;
+
+  if (min_file_size.ValueOrDie() > header.remaining_file_size)
+    return false;
+
+  const char* string_table =
+      reinterpret_cast<const char*>(buffer.get() + records_size.ValueOrDie());
+  // Ensure the string table is \0 terminated.
+  if (buffer.get()[header.remaining_file_size - 1] != 0)
+    return false;
+
+  for (uint32_t i = 0; i < header.num_records; ++i) {
+    ReportDisk r;
+    const MetadataFileReportRecord* record =

[Mark Mentovai, 2015/02/06 21:13:23]

Maybe it would have been better to have allocated a
vector<MetadataFileReportRecord> or scoped_ptr<MetadataFileReportRecord[]> and
then do one LoggingReadFile into that and another into something allocated for
string_table. Alignment’s an issue with that so you’d need to define a
tightly-packed struct with no “holes” instead of using pragma pack.

[scottmg, 2015/02/06 22:58:18]

Sure. Done.

+        reinterpret_cast<const MetadataFileReportRecord*>(
+            buffer.get() + i * sizeof(MetadataFileReportRecord));
+    r.uuid = record->uuid;
+    size_t last_valid_index =
+        header.remaining_file_size - records_size.ValueOrDie() - 1;
+    if (record->file_path_index > last_valid_index ||
+        record->id_index > last_valid_index) {
+      return false;
+    }
+    r.file_path = base::FilePath(
+        base::UTF8ToUTF16(&string_table[record->file_path_index]));
+    r.id = &string_table[record->id_index];
+    r.creation_time = record->creation_time;
+    r.uploaded = record->uploaded;
+    r.last_upload_attempt_time = record->last_upload_attempt_time;
+    r.upload_attempts = record->upload_attempts;
+    r.state = static_cast<ReportState>(record->state);
+    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.
+  {
+    auto output_file = ScopedFileHandle(LoggingOpenFileForWrite(
+        path, FileWriteMode::kTruncateOrCreate, FilePermissions::kOwnerOnly));
+
+    // Build the records and string table we're going to write.
+    size_t num_records = reports_.size();
+    std::string string_table;
+    scoped_ptr<MetadataFileReportRecord[]> records(
+        new MetadataFileReportRecord[num_records]);
+    for (size_t i = 0; i < num_records; ++i) {
+      const ReportDisk& report = reports_[i];
+      MetadataFileReportRecord& record = records[i];
+      record.uuid = report.uuid;
+      record.file_path_index =
+          AddStringToTable(&string_table, report.file_path.value());
+      record.id_index = AddStringToTable(&string_table, report.id);
+      record.creation_time = report.creation_time;
+      record.uploaded = report.uploaded;
+      record.last_upload_attempt_time = report.last_upload_attempt_time;
+      record.upload_attempts = report.upload_attempts;
+      record.state = static_cast<uint32_t>(report.state);
+    }
+
+    // Now we have the size of the file, fill out the header.
+    MetadataFileHeader header;
+    header.magic = kMetadataFileHeaderMagic;
+    header.version = kMetadataFileVersion;
+    auto file_size = base::CheckedNumeric<uint32_t>(num_records) *
+                         sizeof(MetadataFileReportRecord) +
+                     string_table.size();
+    if (!file_size.IsValid())
+      return false;
+    header.remaining_file_size = file_size.ValueOrDie();
+    header.num_records = base::checked_cast<uint32_t>(num_records);
+
+    if (!LoggingWriteFile(output_file.get(), &header, sizeof(header)))
+      return false;
+    if (!LoggingWriteFile(output_file.get(),
+                          records.get(),
+                          num_records * sizeof(MetadataFileReportRecord))) {
+      return false;
+    }
+    if (!LoggingWriteFile(
+            output_file.get(), string_table.c_str(), string_table.size())) {
+      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);

[Mark Mentovai, 2015/02/06 21:13:23]

Because you’ve documented it to be safe to call Metadata::Read() a second time
if Metadata::Write() has been called:

if (result) {
  reports_.clear();
}

and slash or

reports_.clear() at the top of Read().

[scottmg, 2015/02/06 22:58:18]

Oops, good point. Done.

I don't like the interface much, it's broader than it needs to be. Ideally
AcquireMetadata would return a Scoped object. Read is manual for error handling
during construction. Write is manual because the Find functions return a mutable
pointer, so the database doesn't know when Writes are necessary (and always
writing wouldn't be very good for the GetPending/CompletedReports calls). I'll
think about this a bit more. It could reduce the mess of over-documentation too.

+  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);
+}
+
+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;
+}
+
+OperationStatus Metadata::RemoveByUUID(const UUID& uuid) {
+  for (size_t i = 0; i < reports_.size(); ++i) {
+    if (reports_[i].uuid == uuid) {
+      if (!DeleteFile(reports_[i].file_path.value().c_str())) {
+        PLOG(ERROR) << "DeleteFile " << reports_[i].file_path.value().c_str();
+        return CrashReportDatabase::kFileSystemError;
+      }
+      reports_.erase(reports_.begin() + i);
+      return CrashReportDatabase::kNoError;
+    }
+  }
+  return CrashReportDatabase::kReportNotFound;
+}
+
+// static
+OperationStatus Metadata::VerifyReport(const ReportDisk& report_win,

[Mark Mentovai, 2015/02/06 21:13:23]

Name this report_disk to match the type.

[scottmg, 2015/02/06 22:58:18]

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()) ==

[Mark Mentovai, 2015/02/06 21:13:23]

Since you’re already getting the attributes anyway, you might as well make sure
it’s not a directory.

[scottmg, 2015/02/06 22:58:18]

Done.

+      INVALID_FILE_ATTRIBUTES) {
+    return CrashReportDatabase::kReportNotFound;
+  }
+  return CrashReportDatabase::kNoError;
+}
+
+class CrashReportDatabaseWin : public CrashReportDatabase {
+ public:
+  explicit CrashReportDatabaseWin(const base::FilePath& path);
+  ~CrashReportDatabaseWin() override;
+
+  bool Initialize();
+
+  // CrashReportDatabase::
+  OperationStatus PrepareNewCrashReport(NewReport** report) override;
+  OperationStatus FinishedWritingCrashReport(NewReport* report,
+                                             UUID* uuid) override;
+  OperationStatus ErrorWritingCrashReport(NewReport* report) 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), mutex_() {
+}
+
+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));

[Mark Mentovai, 2015/02/06 21:13:23]

Is this one systemwide mutex for all Crashpad databases?

[scottmg, 2015/02/06 22:58:17]

Yes, system-wide, so only one metadata file can be operated on at a time which
doesn't seem too onerous. Switched to using base_dir_ instead (the only drawback
would be separate instances using different non-canonicalized base_dir_s).

[Mark Mentovai, 2015/02/06 23:29:52]

scottmg wrote:
I thought Windows provided good file-level locking, could we use that?

[scottmg, 2015/02/07 01:21:57]

Done.

+
+  // TODO(scottmg): When are completed reports pruned from disk? Delete here or
+  // maybe on Read().
+
+  return true;
+}
+
+OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport(

[Mark Mentovai, 2015/02/06 21:13:23]

I would have preferred it if this stage of the game didn’t require a metadata
read and then write. Since we’ve got to do that for
FinishedWritingCrashReport(), we’re doing read-write-read-write on the metadata
for each crash report. And if we hit ErrorWritingCrashReport(), it’s
read-write-read-write when maybe we could have gotten away with zero metadata
operations.

Since we don’t have a need for a GetNewReports(), maybe we can structure this
the way that it exists on the Mac, where new reports are written to a different
directory and then moved into place by FinishedWritingCrashReport(), which would
be responsible for writing the metadata.

[scottmg, 2015/02/06 22:58:17]

I didn't like the extra error handling cases for moving files around as they
change states.

All the data required is actually in NewReportDisk so I just stopped doing the
unnecessary metadata ops. (It was a little less obvious when this was written
before ErrorWritingCrashReport existed.)

+    NewReport** out_report) {
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+
+  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");
+  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,

[Mark Mentovai, 2015/02/06 21:13:23]

Shouldn’t we trust the uuid’s uniqueness and use kCreateOrFail? That’s what we
do on the Mac (O_CREAT | O_EXCL).

[scottmg, 2015/02/06 22:58:18]

Done.

+                                           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));

[Mark Mentovai, 2015/02/06 21:13:22]

Too bad we don’t have down_cast<>.

+  // 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::ErrorWritingCrashReport(
+    NewReport* report) {
+  // Close the outstanding handle.
+  LoggingCloseFile(report->handle);
+
+  // Take ownership of the report, and cast to our private version with UUID.
+  scoped_ptr<NewReportDisk> scoped_report(static_cast<NewReportDisk*>(report));
+
+  // Remove the report from the metadata and disk.
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+  OperationStatus os = metadata->RemoveByUUID(scoped_report->uuid);
+  if (os != kNoError)
+    return os;
+  if (!metadata->Write())
+    return kDatabaseError;
+
+  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) {
+  // Take ownership, allocated in GetReportForUploading.
+  scoped_ptr<const Report> upload_report(report);
+
+  scoped_ptr<Metadata> metadata(AcquireMetadata());
+  if (!metadata)
+    return kFileSystemError;
+
+  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)

[Mark Mentovai, 2015/02/06 21:13:23]

Is WAIT_ABANDONED OK too?

[scottmg, 2015/02/06 22:58:17]

Could be. Because we're INFINITE it would mean that the previous holder of the
mutex crashed. In that case, we don't know the state (metadata corruption, etc.)
so failing seems safer.

[Mark Mentovai, 2015/02/06 23:29:52]

scottmg wrote:
If you try to take the mutex while someone else holds it and they crash, you get
WAIT_ABANDONED.

If someone else was holding the mutex and they crash and then you try to take
it, you get WAIT_OBJECT_0.

The metadata’s in the same indeterminate state either way, so why don’t we claim
the database then? If we don’t, the next operation is going to anyway.

[scottmg, 2015/02/07 01:21:58]

Switched to LockFileEx. Now the only way we'll fail here is if we can't open or
lock the file, and we return kDatabaseError to the client (as those are
situations that might "clear up" later). On unsuccessful read of the metadata
file, a valid but empty object is returned, so we'll lose existing reports, but
hopefully continue on to capture another day.

+    return nullptr;

[Mark Mentovai, 2015/02/06 21:13:22]

PLOG something?

[scottmg, 2015/02/06 22:58:17]

Done.

+  scoped_ptr<Metadata> metadata(new Metadata(base_dir_, mutex_.get()));
+  if (!metadata->Read()) {
+    LOG(ERROR) << "metadata file could not be loaded";
+    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)));

[Mark Mentovai, 2015/02/06 21:13:23]

I think this append is crazy, but it matches what we have on Mac.

[scottmg, 2015/02/06 22:58:17]

I don't understand what's crazy about the append.

[Mark Mentovai, 2015/02/06 23:29:52]

scottmg wrote:
The argument to this function is the path to the thing’s parent, not the path to
the thing itself.

[scottmg, 2015/02/07 01:21:58]

I see. Well, Metadata takes only the file handle now, so I guess that's a bit
better.

+  if (!database_win->Initialize())
+    database_win.reset();
+
+  return scoped_ptr<CrashReportDatabase>(database_win.release());
+}
+
+}  // namespace crashpad