win: Implementation of CrashReportDatabase for Windows (for C++ Windows readability review)

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,
@@ skipping 17 matching lines @@
 
 namespace {
 
 const wchar_t kDatabaseDirectoryName[] = L"Crashpad";
 
 const wchar_t kReportsDirectory[] = L"reports";
 const wchar_t kMetadataFileName[] = L"metadata";
 
 const wchar_t kCrashReportFileExtension[] = L"dmp";
 
 enum class ReportState : int {

[Peter Kasting, 2015/02/13 00:32:39]

Seems like this should be "int32_t" rather than "int" based on
MetadataFileReportRecord below.

[scottmg, 2015/02/13 06:34:05]

This one isn't used directly for serialization, so removed the explicit type.

   //! \brief Created and filled out by caller, owned by database.
   kPending,
   //! \brief In the process of uploading, owned by caller.
   kUploading,
   //! \brief Upload completed or skipped, owned by database.
   kCompleted,
 };
 
 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) {

[Peter Kasting, 2015/02/13 00:32:39]

Nit: "EnsureDirectoryExists" or "CreateDirectoryIfNecessary" seem like
less-confusing names.

[scottmg, 2015/02/13 06:34:02]

Done.

   if (CreateDirectory(path.value().c_str(), nullptr)) {
     return true;
   } else if (GetLastError() == ERROR_ALREADY_EXISTS) {

[Peter Kasting, 2015/02/13 00:32:39]

Don't use "else" after "return"; see
http://dev.chromium.org/developers/coding-style#Code_Formatting .

Consider reversing this conditional so you can unindent most of the rest of the
function:

  // Try to create the directory.
  if (CreateDirectory(path.value().c_str(), nullptr))
    return true;
  if (GetLastError() == ERROR_ALREADY_EXISTS) {
    PLOG(ERROR) << "Failed to create directory " << path.value();
    return;
  }

  // Check that the result is actually a directory.
  ...

[scottmg, 2015/02/13 06:34:03]

Done. (!= ERROR_ALREADY_EXISTS above)

     DWORD fileattr = GetFileAttributes(path.value().c_str());
     if (fileattr == INVALID_FILE_ATTRIBUTES) {
       PLOG(ERROR) << "GetFileAttributes";

[Peter Kasting, 2015/02/13 00:32:39]

Unless you have a way of collecting and analyzing the log data, Chromium code
should generally try to avoid logging statements, at least ones that aren't
debug-only; they bloat the binary and users don't normally see them anyway. 
It's also potentially inconsistent that in most error cases you use LOG + return
(which will basically be a silent no-op in release) but for things like
checked_cast, the program will crash if the cast fails.  I'm not certain, but
it's possible more of these should be DCHECK ("never happens") or CHECK ("never
happens, and if it were to happen, it'd be a security hole, so force a crash")
versus LOG + return ("can happen in exceptional circumstances; no-op").

Nit: Consider making your error statements more like complete sentences ("Failed
to create temporary directory '<dir>'" or similar), to minimize any possible
confusion when someone sees one of these messages.

(Several places across different files)

[scottmg, 2015/02/13 06:34:02]

Noted the general preference to avoid bloat in logging in Chromium.

We had a discussion about this in the original review:

https://codereview.chromium.org/867363003/#msg42
https://codereview.chromium.org/867363003/#msg45

The preference was to have more extensive logging in this component, perhaps
partially because it's related to crashing, so problems within it might require
more heroic measures from users to track down problems (like gathering logs).
There was also a vague plan to redirect the LOGs to something we could gather
and upload in the future.

[Peter Kasting, 2015/02/13 22:30:33]

Ack.  I wonder if that decision should be captured in a comment somewhere, but
I'm not sure where it would go.

       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

[Peter Kasting, 2015/02/13 00:32:39]

Nit: "and" -> "so we"?  (Unsure)

[scottmg, 2015/02/13 06:34:03]

Done.

 //!     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

[Peter Kasting, 2015/02/13 00:32:40]

Consider pulling this Metadata class and the associated helpers out to another
file.  It's a little easier to read .cc files when they're basically one class
per file, and CrashReportDatabaseWin is arguably the focus of this file, given
the filename.

[scottmg, 2015/02/13 06:34:04]

I would prefer not to. I agree it would be a bit easier to read, but the two
classes only make sense together, Metadata being an internal detail of how
CrashReportDatabaseWin is implemented. If Metadata were in a separate file, I
think it would be slightly less clear to the user of the code, who might think
that it was available for standalone use.

[Peter Kasting, 2015/02/13 22:30:33]

In principle, why couldn't it be available for standalone use?  If there was a
second, separate legitimate use for it it seems like that would be appropriate. 
Your argument suggests to me that it's important to prevent anyone else from
ever accessing this class, and I'm not sure why that would be.

At the very minimum, when defining multiple classes in the same file, I suggest
using divider comments like those in e.g.
chrome/browser/ui/omnibox/omnibox_edit_model.cc.

[scottmg, 2015/02/13 23:37:16]

It's not important to prevent it, it just wasn't intended for separate use.
Someone opening up this directory wondering how to access a crash report
database should go here. I've added some dividers to perhaps help make it more
clear.

 //!     to disk, and queries. Instances of this class should be created by using
 //!     CrashReportDatabaseWin::AcquireMetadata().
 class Metadata {
  public:
   //! \brief Writes any changes if necessary, unlocks and closes the file
   //!     handle.
   ~Metadata();
 
   //! \brief Adds a new report to the set.
   //!
   //! \param[in] new_report_disk The record to add. The #state field must be set
   //!     to kPending.
   void AddNewRecord(const ReportDisk& new_report_disk);
 
   //! \brief Finds all reports in a given state. The \a reports vector is only
   //!     valid when CrashReportDatabase::kNoError is returned.
   //!
   //! \param[in] desired_state The state to match.
   //! \param[out] reports Matching reports, must be empty on entry.
   OperationStatus FindReports(
       ReportState desired_state,
       std::vector<const CrashReportDatabase::Report>* reports);

[Peter Kasting, 2015/02/13 00:32:38]

This function can be const.

[scottmg, 2015/02/13 06:34:04]

Done.

 
   //! \brief Finds the report matching the given UUID.
   //!
   //! The returned report is only valid if CrashReportDatabase::kNoError is
   //! returned.
   //!
   //! \param[in] uuid The report identifier.
   //! \param[out] report_disk The found report, valid only if
   //!     CrashReportDatabase::kNoError is returned. Ownership is not
   //!     transferred to the caller, and the report may not be modified.
   OperationStatus FindSingleReport(const UUID& uuid,
                                    const ReportDisk** report_disk);

[Peter Kasting, 2015/02/13 00:32:39]

This function can be const (but see comment below).

[scottmg, 2015/02/13 06:34:02]

Done.

 
   //! \brief Finds a single report matching the given UUID and in the desired
   //!     state and calls the client-supplied mutator to modify the report.
   //!
   //! The mutator object must have an operator()(ReportDisk*) which makes the
   //! desired changes.
   //!
   //! \return #kNoError on success. #kReportNotFound if there was no report with
   //!     the specified UUID. #kBusyError if the report was not in the specified
   //!     state.
   template <class T>
   OperationStatus MutateSingleReport(const UUID& uuid,
                                      ReportState desired_state,
                                      const T& mutator);

[Peter Kasting, 2015/02/13 00:32:39]

Taking a mutator and passing in lambdas is very clever, but I can't help
thinking this would all be even clearer as just a pair of functions,
FindReport() and FindReportAnyState(), which take a UUID (and, in the former
case, a ReportState) and return the ReportDisk* as an outparam.  (If you want
FindReportAnyState() to be a const function with a const outparam you could do
so, although I might rename as "FindConstReportAnyState" if so.)

This would match the construction of VerifyReport, and allow callers to avoid
the slight verbosity of the lambda syntax.  It would also arguably be a little
easier to understand, as (in std:: algorithm terms) it's closer to a "find_if(),
then do stuff" paradigm than some sort of strange version of transform().

[scottmg, 2015/02/13 06:34:04]

Aha! That was how I initially wrote it. (Around ps#13 to ps#14 in the original
CL). The drawback of returning non-const ReportDisk* was that the Finds couldn't
be generic because then Metadata had to either assume that the metadata was
always dirtied (causing unnecessary Write()s in e.g. LookUpCrashReport()) or had
to delegate the responsibility to call Write() when the ReportDisk was changed
to the caller, which seemed error prone (and more verbose for callers in any
case).

If you think the lambdas are too cute, we could simply swap MutateSingleReport
for FindSingleReportAndMarkDirty. I think that matches what you intended, so I
did that.

[Peter Kasting, 2015/02/13 22:30:32]

Yeah, I think that's fine.  It's OK with me for these helpers to be
non-generic/not-completely-parallel in structure since their callers are fairly
specialized.  We can always do something better if we need to add more callsites
later.

[scottmg, 2015/02/13 23:37:17]

Acknowledged.

 
  private:
   static scoped_ptr<Metadata> Create(const base::FilePath& metadata_file,

[Peter Kasting, 2015/02/13 00:32:38]

Static methods go below the constructor; see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
.

[scottmg, 2015/02/13 06:34:01]

Done.

                                      const base::FilePath& report_dir);
   friend class CrashReportDatabaseWin;

[Peter Kasting, 2015/02/13 00:32:40]

Is there really much benefit in making the constructor private and then
friending CrashReportDatabaseWin?  Since this is a utility class used only by
that class, it seems like you should just make the constructor public and assume
the creator of this object knows how to use it.

This also avoids the need to discuss AcquireMetadata() in the class comments:
the class effectively "doesn't care" who uses it.

[scottmg, 2015/02/13 06:34:04]

Done.

 
   Metadata(FileHandle handle, const base::FilePath& report_dir);
 
   bool Rewind();
 
   void Read();
   void Write();
 
   //! \brief Confirms that the corresponding report actually exists on disk
   //!     (that is, the dump file has not been removed), that the report is in

[Peter Kasting, 2015/02/13 00:32:39]

Nit: that the -> and that the

[scottmg, 2015/02/13 06:34:02]

Done.

   //!     the given state.
   static OperationStatus VerifyReport(const ReportDisk& report_disk,
                                       ReportState desired_state);
   //! \brief Confirms that the corresponding report actually exists on disk
   //!     (that is, the dump file has not been removed).
   static OperationStatus VerifyReportAnyState(const ReportDisk& report_disk);
 
   ScopedFileHandle handle_;
   const base::FilePath report_dir_;
   bool dirty_;  //! \brief Is a Write() required on destruction?

[Peter Kasting, 2015/02/13 00:32:40]

Nit: Clearer that you're not uncertain yourself: "True when a Write() is
required on destruction."

[scottmg, 2015/02/13 06:34:03]

Done.

   std::vector<ReportDisk> reports_;
 
   DISALLOW_COPY_AND_ASSIGN(Metadata);
 };
 
 Metadata::Metadata(FileHandle handle, const base::FilePath& report_dir)

[Peter Kasting, 2015/02/13 00:32:39]

Define functions (including class members) in the same order they were declared;
see http://dev.chromium.org/developers/coding-style#Code_Formatting .

[scottmg, 2015/02/13 06:34:05]

Done.

     : handle_(handle), report_dir_(report_dir), dirty_(false), reports_() {
 }
 
 Metadata::~Metadata() {
   if (dirty_)
     Write();
   // Not actually async, UnlockFileEx requires the Offset fields.
   OVERLAPPED overlapped = {0};
   if (!UnlockFileEx(handle_.get(), 0, MAXDWORD, MAXDWORD, &overlapped))
     PLOG(ERROR) << "UnlockFileEx";
 }
 
 // The format of the metadata file is a MetadataFileHeader, followed by a

[Peter Kasting, 2015/02/13 00:32:40]

This is a good comment but it seems like it should be somewhere other than the
middle of the file.  Perhaps it belongs in the Metadata class header comment, or
in a file comment atop the file?  Likewise, the struct definitions themselves
are somewhat oddly placed in the midst of the Metadata member definitions. 
Maybe the whole block should move above Metadata?  I'm not sure.

[scottmg, 2015/02/13 06:34:05]

The idea was that there's a small subsection here, from the block comment that
describes the format at a high level, the structures the define it, followed by
Read() and Write() which are the only methods that use MetadataFile*.

[Peter Kasting, 2015/02/13 22:30:32]

Hmm.  I think that connection is not terribly apparent to a reader.  I would
probably prefer to move these somewhere else to keep all the Metadata:: method
definitions together, even at the cost of making the consumer of these objects
be further away from their definitions.  If you follow my divider comment
suggestion above, these would probably still be fairly easy to scan up and find.

[scottmg, 2015/02/13 23:37:16]

Done.

 // 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)

[Peter Kasting, 2015/02/13 00:32:39]

I don't think this should actually do anything given the struct layouts below?

[scottmg, 2015/02/13 06:34:03]

Done.

 
 struct MetadataFileHeader {
   uint32_t magic;
   uint32_t version;
   uint32_t num_records;
   uint32_t padding;
 };
 
 struct MetadataFileReportRecord {
   UUID uuid;  // UUID is a 16 byte, standard layout structure.
   uint32_t file_path_index;  // Index into string table. File name is relative
                              // to the reports directory when on disk.
   uint32_t id_index;  // Index into string table.
   int64_t creation_time;  // Holds a time_t.
   int64_t last_upload_attempt_time;  // Holds a time_t.
   int32_t upload_attempts;
   int32_t state;  // A ReportState.
   uint8_t uploaded;  // Boolean, 0 or 1.
   uint8_t padding[7];
 };
 
 const uint32_t kMetadataFileHeaderMagic = 'CPAD';
 const uint32_t kMetadataFileVersion = 1;

[Peter Kasting, 2015/02/13 00:32:39]

Nit: Seems like these should perhaps be up with kMetadataFileName and the other
constants; outside the #pragma pack section regardless.

[scottmg, 2015/02/13 06:34:04]

I would prefer to keep them next to MetadataFileHeader. Everything is moved to
the top now due to the converter/constructor functions.

 
 #pragma pack(pop)
 
 // Reads from the current file position to EOF and returns as uint8_t[].

[Peter Kasting, 2015/02/13 00:32:39]

Nit: Since this doesn't actually return a uint8_t[], perhaps "returns as a
string of bytes".

[scottmg, 2015/02/13 06:34:02]

Oops, done.

 std::string ReadRestOfFileAsString(FileHandle file) {
   FileOffset read_from = LoggingSeekFile(file, 0, SEEK_CUR);
   FileOffset end = LoggingSeekFile(file, 0, SEEK_END);
   FileOffset original = LoggingSeekFile(file, read_from, SEEK_SET);
   if (read_from == -1 || end == -1 || original == -1)
     return std::string();
   DCHECK_EQ(read_from, original);
   DCHECK_GE(end, read_from);

[Peter Kasting, 2015/02/13 00:32:40]

Nit: If there's 0 space left in the file, you can bail early here; this avoids
any potential problems if e.g. creating a string or calling LoggingReadFile with
zero length make things unhappy:

  if (read_from == -1 || end == -1 || original == -1 || read_from == end)
    return std::string();
  DCHECK_EQ(read_from, original);
  DCHECK_GT(end, read_from);
  ...

[scottmg, 2015/02/13 06:34:02]

Done.

   size_t data_length = static_cast<size_t>(end - read_from);
   std::string buffer(data_length, '\0');
   if (!LoggingReadFile(file, &buffer[0], data_length))
     return std::string();
   return buffer;

[Peter Kasting, 2015/02/13 00:32:38]

Nit: Optional shorter method:

  return LoggingReadFile(file, &buffer[0], data_length) ?
      buffer : std::string();

[scottmg, 2015/02/13 06:34:02]

Done.

 }
 
 uint32_t AddStringToTable(std::string* string_table, const std::string& str) {

[Peter Kasting, 2015/02/13 00:32:39]

Please add a comment above this function indicating what it returns.

[scottmg, 2015/02/13 06:34:05]

Done.

   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) {

[Peter Kasting, 2015/02/13 00:32:39]

Nit: I'm not 100% certain but I believe we're now trying to use string16 in
place of wstring even in Windows-specific code (since one is just an alias for
the other).  (Several places across multiple files)

[scottmg, 2015/02/13 06:34:02]

Done.

   return AddStringToTable(string_table, base::UTF16ToUTF8(str));
 }
 
 // static
 scoped_ptr<Metadata> Metadata::Create(const base::FilePath& metadata_file,
                                       const base::FilePath& report_dir) {
   // It is important that dwShareMode be non-zero so that concurrent access to
   // this file results in a successful open. This allows us to get to LockFileEx
   // which then blocks to guard access.
   FileHandle handle = CreateFile(metadata_file.value().c_str(),
@@ skipping 21 matching lines @@
   // If Read() fails, for whatever reason (corruption, etc.) metadata will not
   // have been modified and will be in a clean empty state. We continue on and
   // return an empty database to hopefully recover. This means that existing
   // crash reports have been orphaned.
   metadata->Read();
   return metadata;
 }
 
 bool Metadata::Rewind() {
   FileOffset result = LoggingSeekFile(handle_.get(), 0, SEEK_SET);
   DCHECK_EQ(result, 0);

[Peter Kasting, 2015/02/13 00:32:39]

Nit: While not required, typical Chromium style for [D]CHECK_EQ/NE is (expected,
actual) to match the gtest macro order.

[scottmg, 2015/02/13 06:34:03]

Done.

[Mark Mentovai, 2015/02/13 14:12:46]

Peter Kasting wrote:
Crashpad-wide, all non-gtest _EQ macros are structured “naturally”, (actual,
expected).

[scottmg, 2015/02/13 17:26:41]

Rolled back.

[Peter Kasting, 2015/02/13 22:30:33]

Good to know.  That's what should be done here too, then.

   return result == 0;
 }
 
 void Metadata::Read() {
   FileOffset length = LoggingSeekFile(handle_.get(), 0, SEEK_END);
   if (length <= 0)  // Failed, or empty: Abort.
     return;
   if (!Rewind()) {
     LOG(ERROR) << "failed to rewind to read";
     return;
   }
 
   MetadataFileHeader header;
   if (!LoggingReadFile(handle_.get(), &header, sizeof(header))) {
     LOG(ERROR) << "failed to read header";
     return;
   }
   if (header.magic != kMetadataFileHeaderMagic ||
       header.version != kMetadataFileVersion) {
     LOG(ERROR) << "unexpected header";
     return;
   }
 
   auto records_size = base::CheckedNumeric<uint32_t>(header.num_records) *

[Peter Kasting, 2015/02/13 00:32:40]

I'm not sure "auto" is best here, mainly because it's not necessarily obvious
that CheckedNumeric * size_t == CheckedNumeric, or what the type of the result
is, and to figure out one has to peer into the template muck in CheckedNumeric's
definitions.  I would probably err on the side of declaring this as a
CheckedNumeric<uint32_t>.

[scottmg, 2015/02/13 06:34:04]

That was by request. :) But done.

                       sizeof(MetadataFileReportRecord);
   if (!records_size.IsValid()) {
     LOG(ERROR) << "record size out of range";
     return;
   }
 
   scoped_ptr<MetadataFileReportRecord[]> records(
       new MetadataFileReportRecord[header.num_records]);

[Peter Kasting, 2015/02/13 00:32:38]

Can this be done with a vector<MetadataFileReportRecord> instead?  You can pass
the size to the constructor and then pass &records[0] to LoggingReadFile(). 
This allows you to use range-based for below as well.

[scottmg, 2015/02/13 06:34:03]

Done.

   if (!LoggingReadFile(
           handle_.get(), records.get(), records_size.ValueOrDie())) {
     LOG(ERROR) << "failed to read records";
     return;
   }
 
   std::string string_table = ReadRestOfFileAsString(handle_.get());
   if (string_table.empty() || string_table.back() != '\0') {
     LOG(ERROR) << "bad string table";
     return;
   }
   for (uint32_t i = 0; i < header.num_records; ++i) {

[Peter Kasting, 2015/02/13 00:32:39]

Assuming you changed to a vector above, prefer to use range-based for for
brevity and being minimally error-prone:

  for (const auto& record : records) {
    ...

[scottmg, 2015/02/13 06:34:03]

Done.

     ReportDisk r;

[Peter Kasting, 2015/02/13 00:32:40]

Perhaps ReportDisk should define a constructor that takes a const
MetadataFileReportRecord& and copies over the values, with appropriate
conversion/casting.

You would then simply do something like:

    if (record->file_path_index >= string_table.size() || ...) {
      ...
    }
    reports_.push_back(ReportDisk(record));

This would be easier to read here and would minimize the amount of code external
to ReportDisk that has to understand its fields.

[scottmg, 2015/02/13 06:34:03]

It needs the string table and report_dir FilePath to convert indices to
pointers. It feels odd to pass those to a constructor; how about a static
converter:

    reports.push_back(ReportDisk::FromMetataFileReportRecord(
        record, report_dir_, string_table));

[Peter Kasting, 2015/02/13 22:30:32]

I'm OK with either.  I don't feel any oddness about passing a constructor the
arguments it needs to do its work, even if some of those aren't stored on the
object itself -- we do this a lot across the codebase -- and keeping something
as a constructor is slightly better in terms of preserving encapsulation of the
class members, but there's not such a big difference between the two methods
that I would demand one over the other.

[scottmg, 2015/02/13 23:37:16]

OK, switched to constructor.

     const MetadataFileReportRecord* record = &records[i];

[Peter Kasting, 2015/02/13 00:32:40]

Nit: Why not a const&?

[scottmg, 2015/02/13 06:34:04]

Done (via const auto&).

     r.uuid = record->uuid;
     if (record->file_path_index >= string_table.size() ||
         record->id_index >= string_table.size()) {
       reports_.clear();

[Peter Kasting, 2015/02/13 00:32:40]

Avoid this clear() call by using a temporary of the same type as |reports_| and
then doing a reports_.swap() call before exit, which is constant-time.  This
ensures no one who modifies the code will introduce another early exit path that
fails to clear |reports_|.  (Use reserve() on the temporary to ensure that the
loop never needs to allocate more memory.)

[scottmg, 2015/02/13 06:34:04]

Done.

       LOG(ERROR) << "invalid string table index";
       return;
     }
     r.file_path = report_dir_.Append(
         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;
@@ skipping 23 matching lines @@
   header.version = kMetadataFileVersion;
   header.num_records = base::checked_cast<uint32_t>(num_records);
   if (!LoggingWriteFile(handle_.get(), &header, sizeof(header))) {
     LOG(ERROR) << "failed to write header";
     return;
   }
 
   // Build the records and string table we're going to write.
   std::string string_table;
   scoped_ptr<MetadataFileReportRecord[]> records(
       new MetadataFileReportRecord[num_records]);

[Peter Kasting, 2015/02/13 00:32:40]

Again, if you can use a vector here, do so; this allows you to reserve() the
appropriate size and then use push_back() in the loop below, which in turn
allows switching the loop to use range-based for.

[scottmg, 2015/02/13 06:34:03]

Done.

   memset(records.get(), 0, sizeof(MetadataFileReportRecord) * num_records);
   for (size_t i = 0; i < num_records; ++i) {
     const ReportDisk& report = reports_[i];
     MetadataFileReportRecord& record = records[i];

[Peter Kasting, 2015/02/13 00:32:40]

Nit: While not entirely banned, non-const refs are very uncommon in Chromium
code.  In general, prefer pointers to non-const refs for maximum clarity.

If you follow the vector suggestion above, this will turn into a
MetadataFileReportRecord temp instead.  In that case, I suggest defining a
constructor for that object that takes a const ReportDisk&, for the same reasons
(and to the same effect) described previously above.

[scottmg, 2015/02/13 06:34:02]

Done.

     record.uuid = report.uuid;
     const base::FilePath& path = report.file_path;
     if (path.DirName() != report_dir_) {
       LOG(ERROR) << path.value().c_str() << " expected to start with "
                  << report_dir_.value().c_str();

[Peter Kasting, 2015/02/13 00:32:39]

Don't use c_str() with <<; it's unnecessary.

[scottmg, 2015/02/13 06:34:03]

There's no operator<< for wstring/string16 in <ostream>. Perhaps something in
Chromium (illegally) adds one to std::?

...

Huh. Yup:
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=882

Crashpad is currently building against a very stripped down version of base, and
doesn't have this.

[Peter Kasting, 2015/02/13 22:30:33]

Hmm.  The overload for string16 (in string16.h) is not in namespace std.  I
wonder if we could eliminate the wstring overload in base and do the string16
overload in string16.h regardless of the size of wchar_t.  This would break
anyone who currently tries to log a wstring directly and doesn't include
string16.h, I think, which I doubt would be very many people.

It seems like that's beyond the scope of this change, but possibly worth doing. 
Would you be willing to look into it?

[scottmg, 2015/02/13 23:37:16]

Yeah, I could take a look. In this case though, I think it wouldn't help without
further work on changing wstring to string16. e.g. FilePath::StringType is
wstring which I think (?) won't match the operator<<(string16).

[Peter Kasting, 2015/02/13 23:45:10]

Yeah, we should fix FilePath to use string16 in place of wstring.

I'm pretty sure wstring should just be excised from the codebase entirely.

       return;
     }
     record.file_path_index =
         AddStringToTable(&string_table, path.BaseName().value().c_str());

[Peter Kasting, 2015/02/13 00:32:39]

Remove this c_str() call, which will simply add an extra string temporary
generation to the process.

[scottmg, 2015/02/13 06:34:04]

Done.

     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);
   }
 
   if (!LoggingWriteFile(handle_.get(),
                         records.get(),
                         num_records * sizeof(MetadataFileReportRecord))) {

[Peter Kasting, 2015/02/13 00:32:39]

Nit: If you use a vector as suggested above, change this to use records.size()
instead of |num_records|.

[scottmg, 2015/02/13 06:34:03]

Done.

     LOG(ERROR) << "failed to write records";
     return;
   }
   if (!LoggingWriteFile(
           handle_.get(), string_table.c_str(), string_table.size())) {
     LOG(ERROR) << "failed to write string table";
     return;
   }
 }
 
 void Metadata::AddNewRecord(const ReportDisk& new_report_disk) {
   DCHECK(new_report_disk.state == ReportState::kPending);

[Peter Kasting, 2015/02/13 00:32:40]

Nit: DCHECK_EQ

[scottmg, 2015/02/13 06:34:05]

That doesn't compile because ReportState is "enum class" (not enum) and so
doesn't match an operator<<.

[Peter Kasting, 2015/02/13 22:30:32]

Since you removed the explicit type on the enum class in the latest patch, it
seems like it might be better to just make this a non-class enum (which would
allow this to work), unless I'm missing something on what the "class" part buys
your code (I confess I haven't used enum class much yet).

[scottmg, 2015/02/13 23:37:16]

The primary benefit here is that the enumeration values are scoped. That is,
it's named "ReportState::kPending", not "kPending", so the names need not have
long(er) uniquifying names.

[Peter Kasting, 2015/02/13 23:45:10]

But the enum is file-scoped anyway, so we're unlikely to introduce conflicting
names, aren't we?

[Mark Mentovai, 2015/02/13 23:58:51]

I do like the naming advantage of enum class, but there are others too. An enum
class type is much more a proper type than its predecessor enum. enum class
forbids undesirable conversions.

I think you'd actually like them a lot.

[scottmg, 2015/02/14 00:00:47]

That's true. I think it's also preferable for them to have a type (other than
int) so that there's some guard against ending up with a report in state 5.
They're still convertible to ints, but only when explicitly casted. The only
reason against it seems to be DCHECK_EQ? That feels like a bug in logging.h more
than anything.

   reports_.push_back(new_report_disk);
   dirty_ = true;
 }
 
 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);

[Peter Kasting, 2015/02/13 00:32:40]

Nit: Why not just:

    if ((report.state == desired_state) &&
        (VerifyReport(report, desired_state) == CrashReportDatabase::kNoError))
      reports->push_back(report);

(If only we had C++11 library functions available, I'd suggest further
simplifying to a single call to std::copy_if() with a lambda.)

[scottmg, 2015/02/13 06:34:03]

Done.

     }
   }
   return CrashReportDatabase::kNoError;
 }
 
 OperationStatus Metadata::FindSingleReport(const UUID& uuid,
                                            const ReportDisk** out_report) {
   for (size_t i = 0; i < reports_.size(); ++i) {

[Peter Kasting, 2015/02/13 00:32:40]

Nit: Now that we have C++11 lambdas, use std::find_if() to tell the reader your
goal is to find a single record matching a particular predicate:

  auto report_iter = std::find_if(
      reports_.begin(), reports_.end(),
      [uuid](const ReportDisk& report) { return report.uuid = uuid; });
  if (report_iter == reports_.end())
    return CrashReportDatabase::kReportNotFound;
  ...

Same comment applies in MutateSingleReport() below.

[scottmg, 2015/02/13 06:34:05]

Done.

     if (reports_[i].uuid == uuid) {
       OperationStatus os = VerifyReportAnyState(reports_[i]);
       if (os != CrashReportDatabase::kNoError)
         return os;
       *out_report = &reports_[i];
       return CrashReportDatabase::kNoError;

[Peter Kasting, 2015/02/13 00:32:39]

Nit: Simpler:

      if (os == CrashReportDatabase::kNoError)
        *out_report = &reports_[i];
      return os;

Again, same comment applies to MutateSingleReport() below, though there this
doesn't actually save lines, just a tiny bit of verbiage.

[scottmg, 2015/02/13 06:34:02]

To me that feels more complicated. Previously it reads as "check condition,
early out, check condition, early out. When everything is OK, prepare the data
to be returned and return success".

Maybe it's just me. Done, anyway.

[Peter Kasting, 2015/02/13 22:30:32]

That's a legit reading.  I read the intent of the code as "return whether the
report successfully verified; populate |out_report| if the function succeeds",
which is what led to my suggestion.  Both are plausible.

[scottmg, 2015/02/13 23:37:16]

Acknowledged.

     }
   }
   return CrashReportDatabase::kReportNotFound;
 }
 
 template <class T>
 OperationStatus Metadata::MutateSingleReport(
     const UUID& uuid,
     ReportState desired_state,
     const T& mutator) {
@@ skipping 10 matching lines @@
   return CrashReportDatabase::kReportNotFound;
 }
 
 // static
 OperationStatus Metadata::VerifyReportAnyState(const ReportDisk& report_disk) {
   DWORD fileattr = GetFileAttributes(report_disk.file_path.value().c_str());
   if (fileattr == INVALID_FILE_ATTRIBUTES)
     return CrashReportDatabase::kReportNotFound;
   if ((fileattr & FILE_ATTRIBUTE_DIRECTORY) != 0)
     return CrashReportDatabase::kFileSystemError;
   return CrashReportDatabase::kNoError;

[Peter Kasting, 2015/02/13 00:32:40]

Nit: Shorter:

  return (fileattr & FILE_ATTRIBUTE_DIRECTORY) ?
      CrashReportDatabase::kFileSystemError : CrashReportDatabase::kNoError;

[scottmg, 2015/02/13 06:34:02]

Done.

 }
 
 // static
 OperationStatus Metadata::VerifyReport(const ReportDisk& report_disk,
                                        ReportState desired_state) {
   if (report_disk.state != desired_state)
     return CrashReportDatabase::kBusyError;
   return VerifyReportAnyState(report_disk);

[Peter Kasting, 2015/02/13 00:32:40]

Nit: Shorter:

  return (report_disk.state == desired_state) ?
      VerifyReportAnyState(report_disk) : CrashReportDatabase::kBusyError;

[scottmg, 2015/02/13 06:34:04]

Done.

 }
 
 class CrashReportDatabaseWin : public CrashReportDatabase {
  public:
   explicit CrashReportDatabaseWin(const base::FilePath& path);
   ~CrashReportDatabaseWin() override;
 
   bool Initialize();
 
   // CrashReportDatabase:
   OperationStatus PrepareNewCrashReport(NewReport** report) override;

[Peter Kasting, 2015/02/13 00:32:40]

This is out-of-scope for the current CL, but consider changing the parent class
declaration to return this by scoped_ptr<NewReport>* instead of NewReport**, to
force the caller to take ownership, avoiding a potential leak.

Same comment on GetReportForUploading().

[scottmg, 2015/02/13 06:34:05]

That seems like a good idea.

   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;
@@ skipping 16 matching lines @@
 
 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)))

[Peter Kasting, 2015/02/13 00:32:38]

Nit: These conditionals (and their comments) could be combined.

[scottmg, 2015/02/13 06:34:05]

Done.

     return false;
 
   // TODO(scottmg): When are completed reports pruned from disk? Delete here or
   // maybe on AcquireMetadata().
 
   return true;
 }
 
 OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport(
     NewReport** out_report) {

[Peter Kasting, 2015/02/13 00:32:40]

Argument names should match between function declaration and definition.

[scottmg, 2015/02/13 06:34:05]

Done.

   scoped_ptr<NewReportDisk> report(new NewReportDisk());

[Peter Kasting, 2015/02/13 00:32:39]

Declare this below, as close to the first use as possible.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables
.

[scottmg, 2015/02/13 06:34:05]

Done.

 
   ::UUID system_uuid;

[Peter Kasting, 2015/02/13 00:32:39]

Oh man, are ::UUID and UUID different types?  How confusing :(

[scottmg, 2015/02/13 06:34:05]

Yeah, partially due to initial implementation being on Mac where the names to
not match. We could consider renaming Crashpad's.

   if (UuidCreate(&system_uuid) != RPC_S_OK) {

[Peter Kasting, 2015/02/13 00:32:40]

Be consistent about {} on one-line conditional bodies or not; the rest of the
file omits them.

[scottmg, 2015/02/13 06:34:04]

Done.

     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);
@@ skipping 10 matching lines @@
     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 kDatabaseError;
   ReportDisk report_disk;

[Peter Kasting, 2015/02/13 00:32:38]

Nit: Consider adding a constructor for this that will allow you to pass the
values below as arguments, or even better, a constructor that takes a const
NewReportDisk&.

[scottmg, 2015/02/13 06:34:03]

I did this as a ReportDisk constructor, but then undid it. It's doing more than
conversion from NewReportDisk->ReportDisk, it's also getting the current
(system) time, and setting the state to kPending (which it isn't clear why that
constructor should set to that specific value). When it only sets the uuid and
file_path, it seems more cluttery to have it out-of-line than just in the one
place where we do this.

[Peter Kasting, 2015/02/13 22:30:32]

What about just something like:

  ReportDisk::ReportDisk(UUID, FilePath, time_t, ReportState);

This way the caller would still be the one passing in time(nullptr) and
ReportState::kPending.

[Peter Kasting, 2015/02/13 23:45:10]

(I think this is the only comment you didn't reply to.)

[scottmg, 2015/02/14 00:00:47]

Sorry, missed clicking done. Done as suggested.

   report_disk.uuid = scoped_report->uuid;
   report_disk.file_path = scoped_report->path;
   report_disk.creation_time = time(nullptr);
   report_disk.state = ReportState::kPending;
   metadata->AddNewRecord(report_disk);
   *uuid = report_disk.uuid;

[Peter Kasting, 2015/02/13 00:32:38]

Nit: If you use new_report->uuid here, and follow the ReportDisk(const
NewReportDisk&) constructor suggestion above, you can construct the ReportDisk()
object directly in the AddNewRecord() call above.

[scottmg, 2015/02/13 06:34:02]

Acknowledged.

   return kNoError;
 }
 
 OperationStatus CrashReportDatabaseWin::ErrorWritingCrashReport(
     NewReport* report) {
   // Take ownership of the report, and cast to our private version with UUID.
   scoped_ptr<NewReportDisk> scoped_report(static_cast<NewReportDisk*>(report));
 
   // Close the outstanding handle.
   LoggingCloseFile(report->handle);
 
   // We failed to write, so remove the dump file. There's no entry in the
   // metadata table yet.
   if (!DeleteFile(scoped_report->path.value().c_str())) {
     PLOG(ERROR) << "DeleteFile " << scoped_report->path.value().c_str();

[Peter Kasting, 2015/02/13 00:32:40]

Don't use c_str() here.

[scottmg, 2015/02/13 06:34:02]

See above.

     return CrashReportDatabase::kFileSystemError;
   }
 
   return kNoError;
 }
 
 OperationStatus CrashReportDatabaseWin::LookUpCrashReport(const UUID& uuid,
                                                           Report* report) {
   scoped_ptr<Metadata> metadata(AcquireMetadata());
   if (!metadata)
     return kDatabaseError;
   // Find and return a copy of the matching report.
   const ReportDisk* report_disk;
   OperationStatus os = metadata->FindSingleReport(uuid, &report_disk);
   if (os != kNoError)
     return os;
   *report = *report_disk;
   return kNoError;

[Peter Kasting, 2015/02/13 00:32:38]

Nit: Simpler:

  if (os == kNoError)
    *report = *report_disk;
  return os;

[scottmg, 2015/02/13 06:34:04]

Done.

 }
 
 OperationStatus CrashReportDatabaseWin::GetPendingReports(
     std::vector<const Report>* reports) {
   scoped_ptr<Metadata> metadata(AcquireMetadata());
   if (!metadata)
     return kDatabaseError;
   return metadata->FindReports(ReportState::kPending, reports);

[Peter Kasting, 2015/02/13 00:32:38]

Nit: Shorter:

  return metadata ?
      metadata->FindReports(ReportState::kPending, reports) : kDatabaseError;

[scottmg, 2015/02/13 06:34:02]

Done.

 }
 
 OperationStatus CrashReportDatabaseWin::GetCompletedReports(
     std::vector<const Report>* reports) {
   scoped_ptr<Metadata> metadata(AcquireMetadata());
   if (!metadata)
     return kDatabaseError;
   return metadata->FindReports(ReportState::kCompleted, reports);

[Peter Kasting, 2015/02/13 00:32:38]

Nit: Shorter:

  return metadata ?
      metadata->FindReports(ReportState::kCompleted, reports) : kDatabaseError;

[scottmg, 2015/02/13 06:34:04]

Done.

 }
 
 OperationStatus CrashReportDatabaseWin::GetReportForUploading(
     const UUID& uuid,
     const Report** report) {
   scoped_ptr<Metadata> metadata(AcquireMetadata());
   if (!metadata)
     return kDatabaseError;
   // TODO(scottmg): After returning this report to the client, there is no way
   // to reap this report if the uploader fails to call RecordUploadAttempt() or
@@ skipping 19 matching lines @@
     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 kDatabaseError;
   return metadata->MutateSingleReport(
       report->uuid,
       ReportState::kUploading,
       [successful, id](ReportDisk* report_disk) {

[Peter Kasting, 2015/02/13 00:32:39]

This lambda is skirting the border of "too long" -- another reason why changing
MutateSingleReport() to a find function and then doing the remaining mutation
code after it returns might be better.

[scottmg, 2015/02/13 06:34:03]

Done.

         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;
       });
 }
 
 OperationStatus CrashReportDatabaseWin::SkipReportUpload(const UUID& uuid) {
@@ skipping 12 matching lines @@
 }
 
 }  // 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();

[Peter Kasting, 2015/02/13 00:32:39]

Nit: If you do this:

  if (!database_win->Initialize())
    return scoped_ptr<CrashReportDatabaseWin>();

...Then if you also follow the comment below, you can shorten this further by
combining with ?: as in some suggestions above.

[scottmg, 2015/02/13 06:34:05]

Done.

 
   return scoped_ptr<CrashReportDatabase>(database_win.release());

[Peter Kasting, 2015/02/13 00:32:40]

Simply:

  return database_win.Pass();

Pass() can be used to auto-upcast, there's no need to do it explicitly.  This
should save a temp as well.

[scottmg, 2015/02/13 06:34:02]

Done.

 }
 
 }  // namespace crashpad
+// EOF comment added for readability review.