Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
867363003:
win: Implementation of CrashReportDatabase for Windows (Closed)
Descriptionwin: Implementation of CrashReportDatabase for Windows
As there are no extended file attributes available on all Windows file
systems (NTFS supports alternate data streams, but Chrome still supports
running on FAT), instead of using metadata attached to the file, metadata
is stored separately in a simple record-based file and keyed by UUID.
Initially, I attempted a metadata file beside each report, each locked
separately more closely mirroring the Mac implementation. But given the
expected number of of active reports (in the 10s to 100s range?) and the
size of the metadata for each, simply storing it all in one file is much
less complicated when considering error situations.
If the serialization/deserialization becomes a measurable problem, it
could be optimized at some complexity by reading/writing only as
necessary, or optimizing the storage.
R=mark@chromium.org, rsesek@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/0849154aedd9262d5a38a4a0c9c019e644bce906
Patch Set 1 #Patch Set 2 : some impl #Patch Set 3 : . #Patch Set 4 : load/save a metadata file #Patch Set 5 : wip #Patch Set 6 : rewrite with single metadata file #Patch Set 7 : tidy, docstrings #
Total comments: 43
Patch Set 8 : some fixes #Patch Set 9 : fixed size binary data file #
Total comments: 11
Patch Set 10 : fixes #
Total comments: 15
Patch Set 11 : Remove unused function and \returns #Patch Set 12 : validation on strings #
Total comments: 61
Patch Set 13 : fixes #Patch Set 14 : simpler scoped-like behaviour for Metadata #Patch Set 15 : relocate database test #Patch Set 16 : comment #Patch Set 17 : move lock inside #Patch Set 18 : filepath relative to reports dirs #Patch Set 19 : ScopedTempDir::Rename implementation for posix #
Total comments: 8
Patch Set 20 : fixes #
Total comments: 18
Patch Set 21 : add CS to metadata #Patch Set 22 : Create -> protected #Patch Set 23 : revert cs #Patch Set 24 : comment on upload crashes #Patch Set 25 : CreateFile #
Total comments: 3
Patch Set 26 : log on Read() failure #
Total comments: 39
Patch Set 27 : fixes2 #
Total comments: 10
Patch Set 28 : . #
Messages
Total messages: 55 (13 generated)
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Nice! Two points 1) I remember in Chrome on Windows we had an issue with on-disk file corruption of the preferences file. What's the recovery path for if/when something like that happens to the database here? I note the attempt to avoid incomplete writes with writing first to a temp file and then moving it into place, but is that sufficient to guard against corruption? 2) We'll probably want an implementation similar to this on Linux, and maybe on Mac for a non-xattr-based fallback. Something to worry about for later. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:62: //! If the path points to a file, rather than a directory, or the directory \return ? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:125: //! to disk, and queries. Mention that this class shouldn't be instantiated directly but through AcquireMetadata() instead? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:128: //! \brief mutex_to_release must already be acquired. The lock is passed to Use \param instead? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:138: //! method the reports were not mutated, however, Read() may not be called Missing a word in this sentence. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:142: //! \brief Adds a new report to the set. "Must call Write() to persist" ? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:154: //! for calling Write() before destruction. Is the caller also responsible for destruction? \param comments would help. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:174: : base_dir_(base_dir), loaded_(false), mutex_(mutex_to_release) { , reports_() for consistency with full member initialization. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:196: ScopedFileHandle input_file(ScopedFileHandle(LoggingOpenFileForRead(path))); Why do you need to invoke ScopedFileHandle()'s copy constructor here? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:216: int64_t version; int32_t isn't big enough for the version, but uint32_t is for the file size? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:224: return false; Would it be useful to LOG(ERROR) in these conditions? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:239: upload_attempts > std::numeric_limits<int>::max()) { || upload_attempts < 0 , too? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:247: CHECK(r.state != ReportState::kAny); This CHECKs but the rest return false. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:262: std::string to_output = "crashpad metadata\n1\n"; 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. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:278: base::UTF16ToUTF8(report.file_path.value()).c_str(), Is it legal for Windows filepaths/names to contain \n or \t? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:312: OperationStatus Metadata::VerifyReport(const ReportDisk& report_win, This should be below FindSingleReport, based on the declaration order. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:355: virtual ~CrashReportDatabaseWin(); virtual -> override https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:385: : CrashReportDatabase(), base_dir_(path) { , mutex_() https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:400: mutex_.reset(CreateMutex(nullptr, false, kMetadataMutexName)); What will happen if multiple, different Crashpad-based products try to do this? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:412: return kFileSystemError; The documentation for kFileSystemError says that additional information will be logged, so that ties in with the comment at line 224. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:420: static_assert(sizeof(system_uuid) == 16, "unexpected system uuid size"); Would these be better in uuid.cc ? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:526: // Take ownership, allocated in GetReportForUploading. Should this happen before the potential early return on 524?
https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:214: if (lines[0].as_string() != "crashpad metadata") 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.
Thanks! Partially addressed pending further discussion on the file format. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:62: //! If the path points to a file, rather than a directory, or the directory On 2015/02/02 23:06:08, Robert Sesek wrote: > \return ? Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:125: //! to disk, and queries. On 2015/02/02 23:06:08, Robert Sesek wrote: > Mention that this class shouldn't be instantiated directly but through > AcquireMetadata() instead? Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:128: //! \brief mutex_to_release must already be acquired. The lock is passed to On 2015/02/02 23:06:08, Robert Sesek wrote: > Use \param instead? Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:138: //! method the reports were not mutated, however, Read() may not be called On 2015/02/02 23:06:08, Robert Sesek wrote: > Missing a word in this sentence. Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:142: //! \brief Adds a new report to the set. On 2015/02/02 23:06:08, Robert Sesek wrote: > "Must call Write() to persist" ? Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:154: //! for calling Write() before destruction. On 2015/02/02 23:06:08, Robert Sesek wrote: > Is the caller also responsible for destruction? \param comments would help. Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:174: : base_dir_(base_dir), loaded_(false), mutex_(mutex_to_release) { On 2015/02/02 23:06:08, Robert Sesek wrote: > , reports_() for consistency with full member initialization. Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:196: ScopedFileHandle input_file(ScopedFileHandle(LoggingOpenFileForRead(path))); On 2015/02/02 23:06:08, Robert Sesek wrote: > Why do you need to invoke ScopedFileHandle()'s copy constructor here? 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. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:214: if (lines[0].as_string() != "crashpad metadata") On 2015/02/03 16:29:29, Robert Sesek wrote: > 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. 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? https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:216: int64_t version; On 2015/02/02 23:06:08, Robert Sesek wrote: > int32_t isn't big enough for the version, but uint32_t is for the file size? Yeah, was just to avoid another string->int function. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:239: upload_attempts > std::numeric_limits<int>::max()) { On 2015/02/02 23:06:08, Robert Sesek wrote: > || upload_attempts < 0 , too? Good catch, done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:247: CHECK(r.state != ReportState::kAny); On 2015/02/02 23:06:08, Robert Sesek wrote: > This CHECKs but the rest return false. Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:278: base::UTF16ToUTF8(report.file_path.value()).c_str(), On 2015/02/02 23:06:08, Robert Sesek wrote: > Is it legal for Windows filepaths/names to contain \n or \t? No, nothing less than integer value 31 is valid. https://msdn.microsoft.com/en-ca/library/windows/desktop/aa365247(v=vs.85).as... https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:312: OperationStatus Metadata::VerifyReport(const ReportDisk& report_win, On 2015/02/02 23:06:08, Robert Sesek wrote: > This should be below FindSingleReport, based on the declaration order. Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:355: virtual ~CrashReportDatabaseWin(); On 2015/02/02 23:06:07, Robert Sesek wrote: > virtual -> override Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:385: : CrashReportDatabase(), base_dir_(path) { On 2015/02/02 23:06:08, Robert Sesek wrote: > , mutex_() Done. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:400: mutex_.reset(CreateMutex(nullptr, false, kMetadataMutexName)); On 2015/02/02 23:06:08, Robert Sesek wrote: > What will happen if multiple, different Crashpad-based products try to do this? 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. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:420: static_assert(sizeof(system_uuid) == 16, "unexpected system uuid size"); On 2015/02/02 23:06:08, Robert Sesek wrote: > Would these be better in uuid.cc ? It's not strictly required by uuid.cc I guess (i.e. would could convert to a byte buffer here if required). https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:526: // Take ownership, allocated in GetReportForUploading. On 2015/02/02 23:06:08, Robert Sesek wrote: > Should this happen before the potential early return on 524? Indeed! Done.
Patchset #9 (id:220001) has been deleted
On 2015/02/03 18:56:40, scottmg wrote: > Thanks! Partially addressed pending further discussion on the file format. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > File client/crash_report_database_win.cc (right): > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:62: //! If the path points to a file, rather > than a directory, or the directory > On 2015/02/02 23:06:08, Robert Sesek wrote: > > \return ? > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:125: //! to disk, and queries. > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Mention that this class shouldn't be instantiated directly but through > > AcquireMetadata() instead? > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:128: //! \brief mutex_to_release must > already be acquired. The lock is passed to > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Use \param instead? > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:138: //! method the reports were not > mutated, however, Read() may not be called > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Missing a word in this sentence. > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:142: //! \brief Adds a new report to the > set. > On 2015/02/02 23:06:08, Robert Sesek wrote: > > "Must call Write() to persist" ? > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:154: //! for calling Write() before > destruction. > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Is the caller also responsible for destruction? \param comments would help. > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:174: : base_dir_(base_dir), loaded_(false), > mutex_(mutex_to_release) { > On 2015/02/02 23:06:08, Robert Sesek wrote: > > , reports_() for consistency with full member initialization. > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:196: ScopedFileHandle > input_file(ScopedFileHandle(LoggingOpenFileForRead(path))); > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Why do you need to invoke ScopedFileHandle()'s copy constructor here? > > 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. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:214: if (lines[0].as_string() != "crashpad > metadata") > On 2015/02/03 16:29:29, Robert Sesek wrote: > > 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. > > 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 did a pass at this and I think it's an improvement. > I think I'd lean towards the last option. What do you think? > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:216: int64_t version; > On 2015/02/02 23:06:08, Robert Sesek wrote: > > int32_t isn't big enough for the version, but uint32_t is for the file size? > > Yeah, was just to avoid another string->int function. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:239: upload_attempts > > std::numeric_limits<int>::max()) { > On 2015/02/02 23:06:08, Robert Sesek wrote: > > || upload_attempts < 0 , too? > > Good catch, done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:247: CHECK(r.state != ReportState::kAny); > On 2015/02/02 23:06:08, Robert Sesek wrote: > > This CHECKs but the rest return false. > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:278: > base::UTF16ToUTF8(report.file_path.value()).c_str(), > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Is it legal for Windows filepaths/names to contain \n or \t? > > No, nothing less than integer value 31 is valid. > https://msdn.microsoft.com/en-ca/library/windows/desktop/aa365247(v=vs.85).as... > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:312: OperationStatus > Metadata::VerifyReport(const ReportDisk& report_win, > On 2015/02/02 23:06:08, Robert Sesek wrote: > > This should be below FindSingleReport, based on the declaration order. > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:355: virtual ~CrashReportDatabaseWin(); > On 2015/02/02 23:06:07, Robert Sesek wrote: > > virtual -> override > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:385: : CrashReportDatabase(), > base_dir_(path) { > On 2015/02/02 23:06:08, Robert Sesek wrote: > > , mutex_() > > Done. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:400: mutex_.reset(CreateMutex(nullptr, > false, kMetadataMutexName)); > On 2015/02/02 23:06:08, Robert Sesek wrote: > > What will happen if multiple, different Crashpad-based products try to do > this? > > 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. > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:420: static_assert(sizeof(system_uuid) == > 16, "unexpected system uuid size"); > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Would these be better in uuid.cc ? > > It's not strictly required by uuid.cc I guess (i.e. would could convert to a > byte buffer here if required). > > https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... > client/crash_report_database_win.cc:526: // Take ownership, allocated in > GetReportForUploading. > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Should this happen before the potential early return on 524? > > Indeed! Done.
Hi Robert, could you take another look?
On 2015/02/04 23:15:55, scottmg wrote: > Hi Robert, could you take another look? Sorry, I've been in meetings most of the day. This is on my list, but I may not get to it before EOD today.
I like it! The all-binary version is much cleaner. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:196: ScopedFileHandle input_file(ScopedFileHandle(LoggingOpenFileForRead(path))); On 2015/02/03 18:56:39, scottmg wrote: > On 2015/02/02 23:06:08, Robert Sesek wrote: > > Why do you need to invoke ScopedFileHandle()'s copy constructor here? > > 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. Ah, I missed that the ctor was explicit. You can use auto here to make this a little shorter. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:119: //! CrashReportDatabaseWin::AcquireMetadata, rather than direct nit: () in method calls https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:246: header.version != kMetadataFileVersion) { For v2 we'll need to figure out how to handle a database migration/backwards database compatibility, but that's for a different CL. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:254: The two fields num_records and remainig_file_size share a relationship, but we don't validate it. We should. At the very least we should check the result of the multiplication below. 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. 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.file_size_remaining) return false; https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:256: buffer.get() + header.num_records * sizeof(MetadataFileReportRecord)); And then use records_size.ValueOrDie() here. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:311: num_records * sizeof(MetadataFileReportRecord) + string_table.size(); A pathologically large num_records could be problematic here.
Patchset #10 (id:260001) has been deleted
Thanks! Added implementation of new ErrorWritingCrashReport too. https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/180001/client/crash_report_dat... client/crash_report_database_win.cc:196: ScopedFileHandle input_file(ScopedFileHandle(LoggingOpenFileForRead(path))); On 2015/02/05 18:43:09, Robert Sesek wrote: > On 2015/02/03 18:56:39, scottmg wrote: > > On 2015/02/02 23:06:08, Robert Sesek wrote: > > > Why do you need to invoke ScopedFileHandle()'s copy constructor here? > > > > 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. > > Ah, I missed that the ctor was explicit. You can use auto here to make this a > little shorter. Done. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:119: //! CrashReportDatabaseWin::AcquireMetadata, rather than direct On 2015/02/05 18:43:09, Robert Sesek wrote: > nit: () in method calls Done. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:246: header.version != kMetadataFileVersion) { On 2015/02/05 18:43:09, Robert Sesek wrote: > For v2 we'll need to figure out how to handle a database migration/backwards > database compatibility, but that's for a different CL. Acknowledged. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:254: On 2015/02/05 18:43:09, Robert Sesek wrote: > The two fields num_records and remainig_file_size share a relationship, but we > don't validate it. We should. At the very least we should check the result of > the multiplication below. > > 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. > 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.file_size_remaining) > return false; Done. (with > instead of < in last if) https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:256: buffer.get() + header.num_records * sizeof(MetadataFileReportRecord)); On 2015/02/05 18:43:09, Robert Sesek wrote: > And then use records_size.ValueOrDie() here. Done. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:311: num_records * sizeof(MetadataFileReportRecord) + string_table.size(); On 2015/02/05 18:43:09, Robert Sesek wrote: > A pathologically large num_records could be problematic here. Done.
I'm being paranoid, but might as well be safe. FYI: I'm out Fri&Mon so Mark should take a look after this. https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/240001/client/crash_report_dat... client/crash_report_database_win.cc:254: On 2015/02/05 19:27:55, scottmg wrote: > On 2015/02/05 18:43:09, Robert Sesek wrote: > > The two fields num_records and remainig_file_size share a relationship, but we > > don't validate it. We should. At the very least we should check the result of > > the multiplication below. > > > > 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. > > 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.file_size_remaining) > > return false; > > Done. (with > instead of < in last if) Ooops! https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:89: std::vector<base::StringPiece> SplitString(base::StringPiece str, char on) { Dead code now. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:131: //! interceding call to Write(). \return ? https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:136: //! called again until Write() is used. \return ? https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:278: const char* string_table = Ensure that string_table is NUL terminated, otherwise std::string conversion could read past the end of the buffer on a large single string or a non-terminated last string. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:288: base::UTF8ToUTF16(&string_table[record->file_path_index])); We should probably also test that file_path_index and id_index are both less than |remaining_file_size - records_size - 1|, since we're not using bounds-checked access to string_table. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:290: r.creation_time = record->creation_time; > 0? https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:291: r.uploaded = record->uploaded; Verify that this is only 0 or 1. There's 7 other bits that could be 0xbad. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:293: r.upload_attempts = record->upload_attempts; > 0 ? https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:355: // aren't racing ourself here as we're holding the mutex here, we use My understanding is that this will also protect against another handler process operating on the same database, right?
I don't mind adding the extra verification on Read() but it does seem a bit cluttery to check things for validity when the only writer is Write(). Is there some particular situation that you're envisioning this protecting against? Mark, could you take a look otherwise? https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:89: std::vector<base::StringPiece> SplitString(base::StringPiece str, char on) { On 2015/02/05 22:46:18, Robert Sesek wrote: > Dead code now. Oops! Done. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:131: //! interceding call to Write(). On 2015/02/05 22:46:18, Robert Sesek wrote: > \return ? Done. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:136: //! called again until Write() is used. On 2015/02/05 22:46:18, Robert Sesek wrote: > \return ? Done. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:355: // aren't racing ourself here as we're holding the mutex here, we use On 2015/02/05 22:46:18, Robert Sesek wrote: > My understanding is that this will also protect against another handler process > operating on the same database, right? It would protect against file corruption, but without the mutex not against a record being lost in the case of: A reading, B reading, B mutating, B writing, A writing.
On 2015/02/05 23:43:11, scottmg wrote: > I don't mind adding the extra verification on Read() but it does seem a bit > cluttery to check things for validity when the only writer is Write(). Is there > some particular situation that you're envisioning this protecting against? No, just Safety First™ in our memory-unsafe language. I'm less concerned about the numeric range checks than I am about a bad file causing the program to dereference invalid memory (c.f. the string bounds checking).
OK, sounds good. I did the two below which are the ones that could be Memory Unsafe. :) https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:278: const char* string_table = On 2015/02/05 22:46:18, Robert Sesek wrote: > Ensure that string_table is NUL terminated, otherwise std::string conversion > could read past the end of the buffer on a large single string or a > non-terminated last string. Done. https://codereview.chromium.org/867363003/diff/280001/client/crash_report_dat... client/crash_report_database_win.cc:288: base::UTF8ToUTF16(&string_table[record->file_path_index])); On 2015/02/05 22:46:18, Robert Sesek wrote: > We should probably also test that file_path_index and id_index are both less > than |remaining_file_size - records_size - 1|, since we're not using > bounds-checked access to string_table. Done.
https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:23: #include "base/files/file_path.h" 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:129: //! after adding new records. 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:132: //! \brief Finds all reports in a given state. The reports vector is only “The \a reports vector” https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:133: //! valid when #kNoError is returned. \param[out] reports should mention that reports must be empty on call. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:138: //! \brief Finds the report matching the given UUID and the desired state, 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:198: uint32_t remaining_file_size; 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? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:204: uint32_t file_path_index; 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? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:206: int64_t creation_time; time_t epoch or something else? Same on line 208. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:207: uint8_t uploaded; 0 or 1, or other values? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:208: int64_t last_upload_attempt_time; 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:210: int32_t state; Reference ReportState if that’s what this means. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:244: return false; 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? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:246: scoped_ptr<uint8_t> buffer(new uint8_t[header.remaining_file_size]); This needs to be scoped_ptr<uint8_t[]> because it’s supervising something allocated with new[]. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:257: // Each record will have two NUL-terminated strings in the table. 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:275: const MetadataFileReportRecord* record = 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:357: MOVEFILE_REPLACE_EXISTING); 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(). https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:414: OperationStatus Metadata::VerifyReport(const ReportDisk& report_win, Name this report_disk to match the type. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:418: if (GetFileAttributes(report_win.file_path.value().c_str()) == Since you’re already getting the attributes anyway, you might as well make sure it’s not a directory. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:474: mutex_.reset(CreateMutex(nullptr, false, kMetadataMutexName)); Is this one systemwide mutex for all Crashpad databases? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:482: OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport( 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:503: FileWriteMode::kTruncateOrCreate, Shouldn’t we trust the uuid’s uniqueness and use kCreateOrFail? That’s what we do on the Mac (O_CREAT | O_EXCL). https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:519: scoped_ptr<NewReportDisk> scoped_report(static_cast<NewReportDisk*>(report)); Too bad we don’t have down_cast<>. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:657: if (WaitForSingleObject(mutex_.get(), INFINITE) != WAIT_OBJECT_0) Is WAIT_ABANDONED OK too? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:658: return nullptr; PLOG something? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:673: new CrashReportDatabaseWin(path.Append(kDatabaseDirectoryName))); I think this append is crazy, but it matches what we have on Mac.
Thanks. After your comments, I'm going to see if I can avoid returning mutable ReportDisk*s from Metadata, which would mean it could know if it needs a Write which should in turn simplify its interface. (Monday is a holiday here, so I'll try to get something ready today, but won't be back to discuss until Tuesday.) https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:23: #include "base/files/file_path.h" On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:129: //! after adding new records. On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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. Done with DCHECKs. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:132: //! \brief Finds all reports in a given state. The reports vector is only On 2015/02/06 21:13:23, Mark Mentovai wrote: > “The \a reports vector” Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:133: //! valid when #kNoError is returned. On 2015/02/06 21:13:23, Mark Mentovai wrote: > \param[out] reports should mention that reports must be empty on call. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:138: //! \brief Finds the report matching the given UUID and the desired state, On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:198: uint32_t remaining_file_size; On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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? Removed. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:204: uint32_t file_path_index; On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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? 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.) https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:206: int64_t creation_time; On 2015/02/06 21:13:23, Mark Mentovai wrote: > time_t epoch or something else? Same on line 208. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:207: uint8_t uploaded; On 2015/02/06 21:13:24, Mark Mentovai wrote: > 0 or 1, or other values? Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:208: int64_t last_upload_attempt_time; On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:210: int32_t state; On 2015/02/06 21:13:22, Mark Mentovai wrote: > Reference ReportState if that’s what this means. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:244: return false; On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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? 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:246: scoped_ptr<uint8_t> buffer(new uint8_t[header.remaining_file_size]); On 2015/02/06 21:13:23, Mark Mentovai wrote: > This needs to be scoped_ptr<uint8_t[]> because it’s supervising something > allocated with new[]. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:257: // Each record will have two NUL-terminated strings in the table. On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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. True, removed. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:275: const MetadataFileReportRecord* record = On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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. Sure. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:357: MOVEFILE_REPLACE_EXISTING); On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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(). 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:414: OperationStatus Metadata::VerifyReport(const ReportDisk& report_win, On 2015/02/06 21:13:23, Mark Mentovai wrote: > Name this report_disk to match the type. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:418: if (GetFileAttributes(report_win.file_path.value().c_str()) == On 2015/02/06 21:13:23, Mark Mentovai wrote: > Since you’re already getting the attributes anyway, you might as well make sure > it’s not a directory. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:474: mutex_.reset(CreateMutex(nullptr, false, kMetadataMutexName)); On 2015/02/06 21:13:23, Mark Mentovai wrote: > Is this one systemwide mutex for all Crashpad databases? 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). https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:482: OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport( On 2015/02/06 21:13:23, Mark Mentovai wrote: > 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. 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.) https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:503: FileWriteMode::kTruncateOrCreate, On 2015/02/06 21:13:23, Mark Mentovai wrote: > Shouldn’t we trust the uuid’s uniqueness and use kCreateOrFail? That’s what we > do on the Mac (O_CREAT | O_EXCL). Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:657: if (WaitForSingleObject(mutex_.get(), INFINITE) != WAIT_OBJECT_0) On 2015/02/06 21:13:23, Mark Mentovai wrote: > Is WAIT_ABANDONED OK too? 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:658: return nullptr; On 2015/02/06 21:13:22, Mark Mentovai wrote: > PLOG something? Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:673: new CrashReportDatabaseWin(path.Append(kDatabaseDirectoryName))); On 2015/02/06 21:13:23, Mark Mentovai wrote: > I think this append is crazy, but it matches what we have on Mac. I don't understand what's crazy about the append.
OK, let me know when you’ve got the next round ready. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:204: uint32_t file_path_index; scottmg wrote: > 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. Can we keep the paths relative to the database root or the reports directory? The database should be resilient to being moved around trivially. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:244: return false; scottmg wrote: > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > 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? > > 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. 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:474: mutex_.reset(CreateMutex(nullptr, false, kMetadataMutexName)); scottmg wrote: > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > Is this one systemwide mutex for all Crashpad databases? > > 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). I thought Windows provided good file-level locking, could we use that? https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:657: if (WaitForSingleObject(mutex_.get(), INFINITE) != WAIT_OBJECT_0) scottmg wrote: > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > Is WAIT_ABANDONED OK too? > > 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. 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:673: new CrashReportDatabaseWin(path.Append(kDatabaseDirectoryName))); scottmg wrote: > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > I think this append is crazy, but it matches what we have on Mac. > > I don't understand what's crazy about the append. The argument to this function is the path to the thing’s parent, not the path to the thing itself.
Patchset #15 (id:380001) has been deleted
Primary changes since ps#12 are to make Metadata be Scoped and have responsibility for any modifications, and to use file locking on the metadata file instead of a separate mutex. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:204: uint32_t file_path_index; On 2015/02/06 23:29:52, Mark Mentovai wrote: > scottmg wrote: > > 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. > > Can we keep the paths relative to the database root or the reports directory? > The database should be resilient to being moved around trivially. OK. I've added a (failing) test, but haven't yet modified the path manipulation to relativize and absolutize. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:244: return false; On 2015/02/06 23:29:52, Mark Mentovai wrote: > scottmg wrote: > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > 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? > > > > 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. > > 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. Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:474: mutex_.reset(CreateMutex(nullptr, false, kMetadataMutexName)); On 2015/02/06 23:29:52, Mark Mentovai wrote: > scottmg wrote: > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > Is this one systemwide mutex for all Crashpad databases? > > > > 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). > > I thought Windows provided good file-level locking, could we use that? Done. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:657: if (WaitForSingleObject(mutex_.get(), INFINITE) != WAIT_OBJECT_0) On 2015/02/06 23:29:52, Mark Mentovai wrote: > scottmg wrote: > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > Is WAIT_ABANDONED OK too? > > > > 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. > > 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. 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. https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:673: new CrashReportDatabaseWin(path.Append(kDatabaseDirectoryName))); On 2015/02/06 23:29:52, Mark Mentovai wrote: > scottmg wrote: > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > I think this append is crazy, but it matches what we have on Mac. > > > > I don't understand what's crazy about the append. > > The argument to this function is the path to the thing’s parent, not the path to > the thing itself. I see. Well, Metadata takes only the file handle now, so I guess that's a bit better.
On 2015/02/07 01:21:58, scottmg (ooo mon feb 9th) wrote: > Primary changes since ps#12 are to make Metadata be Scoped and have > responsibility for any modifications, and to use file locking on the metadata > file instead of a separate mutex. > > https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... > File client/crash_report_database_win.cc (right): > > https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... > client/crash_report_database_win.cc:204: uint32_t file_path_index; > On 2015/02/06 23:29:52, Mark Mentovai wrote: > > scottmg wrote: > > > 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. > > > > Can we keep the paths relative to the database root or the reports directory? > > The database should be resilient to being moved around trivially. > > OK. I've added a (failing) test, but haven't yet modified the path manipulation > to relativize and absolutize. Implemented now, but will need to fix Mac test code. > > https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... > client/crash_report_database_win.cc:244: return false; > On 2015/02/06 23:29:52, Mark Mentovai wrote: > > scottmg wrote: > > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > > 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? > > > > > > 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. > > > > 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. > > Done. > > https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... > client/crash_report_database_win.cc:474: mutex_.reset(CreateMutex(nullptr, > false, kMetadataMutexName)); > On 2015/02/06 23:29:52, Mark Mentovai wrote: > > scottmg wrote: > > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > > Is this one systemwide mutex for all Crashpad databases? > > > > > > 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). > > > > I thought Windows provided good file-level locking, could we use that? > > Done. > > https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... > client/crash_report_database_win.cc:657: if (WaitForSingleObject(mutex_.get(), > INFINITE) != WAIT_OBJECT_0) > On 2015/02/06 23:29:52, Mark Mentovai wrote: > > scottmg wrote: > > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > > Is WAIT_ABANDONED OK too? > > > > > > 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. > > > > 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. > > 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. > > https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... > client/crash_report_database_win.cc:673: new > CrashReportDatabaseWin(path.Append(kDatabaseDirectoryName))); > On 2015/02/06 23:29:52, Mark Mentovai wrote: > > scottmg wrote: > > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > > I think this append is crazy, but it matches what we have on Mac. > > > > > > I don't understand what's crazy about the append. > > > > The argument to this function is the path to the thing’s parent, not the path > to > > the thing itself. > > I see. Well, Metadata takes only the file handle now, so I guess that's a bit > better.
Mac test code implemented now, ready for another look.
https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:257: // Each record will have two NUL-terminated strings in the table. On 2015/02/06 22:58:18, scottmg (ooo mon feb 9th) wrote: > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > 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. > > True, removed. Even if the strings are pooled, though, the table still must be NUL terminated. That check was lost. https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:135: std::function<void(ReportDisk*)> mutator); Does this require C++11 library support? Is that OK? https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:140: static scoped_ptr<Metadata> Create(const base::FilePath& metadata_file, Move Create and the friend to protected: so that it does not have to access to the ctor, Read, and Write? https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:292: base::UTF8ToUTF16(&string_table[record->file_path_index])); You can retain the checks that both indices are less than string_buffer->size() - 1.
Patchset #20 (id:500001) has been deleted
https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/320001/client/crash_report_dat... client/crash_report_database_win.cc:257: // Each record will have two NUL-terminated strings in the table. On 2015/02/10 20:32:25, Robert Sesek wrote: > On 2015/02/06 22:58:18, scottmg (ooo mon feb 9th) wrote: > > On 2015/02/06 21:13:23, Mark Mentovai wrote: > > > 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. > > > > True, removed. > > Even if the strings are pooled, though, the table still must be NUL terminated. > That check was lost. Done. https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:135: std::function<void(ReportDisk*)> mutator); On 2015/02/10 20:32:26, Robert Sesek wrote: > Does this require C++11 library support? Is that OK? Sigh, apparently. Done. https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:140: static scoped_ptr<Metadata> Create(const base::FilePath& metadata_file, On 2015/02/10 20:32:26, Robert Sesek wrote: > Move Create and the friend to protected: so that it does not have to access to > the ctor, Read, and Write? Sorry, I'm not sure what you mean. (Create calls the ctor and Read at the moment.) https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:292: base::UTF8ToUTF16(&string_table[record->file_path_index])); On 2015/02/10 20:32:26, Robert Sesek wrote: > You can retain the checks that both indices are less than string_buffer->size() > - 1. Done.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, If two threads in the same process try to get the metadata simultaneously, will there be a problem? I anticipate that the handler thread that writes dumps and the uploader thread will both want access to the metadata. https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:579: OperationStatus CrashReportDatabaseWin::GetReportForUploading( If two handlers wind up running out of the same directory, what protects against two processes trying to upload the same report? The Mac database deals with this with a lock on the report file.
https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:140: static scoped_ptr<Metadata> Create(const base::FilePath& metadata_file, On 2015/02/10 20:58:45, scottmg wrote: > On 2015/02/10 20:32:26, Robert Sesek wrote: > > Move Create and the friend to protected: so that it does not have to access to > > the ctor, Read, and Write? > > Sorry, I'm not sure what you mean. (Create calls the ctor and Read at the > moment.) Sorry, yes "it" was vague there. The CrashReportDatabaseWin does not need access to the ctor, Read(), or Write(), so if you move the friend declaration and Create to protected access, then it cannot access those methods.
Patchset #21 (id:540001) has been deleted
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, On 2015/02/10 22:16:29, Mark Mentovai wrote: > If two threads in the same process try to get the metadata simultaneously, will > there be a problem? > > I anticipate that the handler thread that writes dumps and the uploader thread > will both want access to the metadata. Hmm, I had only been considering separate processes, assuming a single process would coordinate above. I guess a critical section here is sufficient. Could switch back to a mutex (but has the problem with multiple crashpad instances), and the LockFileEx seems to better match what's being guarded. The CS should almost never be contended. https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:579: OperationStatus CrashReportDatabaseWin::GetReportForUploading( On 2015/02/10 22:16:29, Mark Mentovai wrote: > If two handlers wind up running out of the same directory, what protects against > two processes trying to upload the same report? The Mac database deals with this > with a lock on the report file. If understand the situation you're describing, the metadata file is protected by the LockFileEx, and only a report that is in state=kPending here will be returned (with its state then changed to kUploading). So the second requestor would fail to find a report here and return kBusyError.
https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/480001/client/crash_report_dat... client/crash_report_database_win.cc:140: static scoped_ptr<Metadata> Create(const base::FilePath& metadata_file, On 2015/02/10 22:39:35, Robert Sesek wrote: > On 2015/02/10 20:58:45, scottmg wrote: > > On 2015/02/10 20:32:26, Robert Sesek wrote: > > > Move Create and the friend to protected: so that it does not have to access > to > > > the ctor, Read, and Write? > > > > Sorry, I'm not sure what you mean. (Create calls the ctor and Read at the > > moment.) > > Sorry, yes "it" was vague there. The CrashReportDatabaseWin does not need access > to the ctor, Read(), or Write(), so if you move the friend declaration and > Create to protected access, then it cannot access those methods. Ah, I see. Done.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, scottmg wrote: > Hmm, I had only been considering separate processes, assuming a single process > would coordinate above. > > I guess a critical section here is sufficient. Could switch back to a mutex (but > has the problem with multiple crashpad instances), and the LockFileEx seems to > better match what's being guarded. The CS should almost never be contended. A critical section that’s held for the same duration that the file’s locked? That could work. https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:579: OperationStatus CrashReportDatabaseWin::GetReportForUploading( scottmg wrote: > If understand the situation you're describing, the metadata file is protected by > the LockFileEx, and only a report that is in state=kPending here will be > returned (with its state then changed to kUploading). So the second requestor > would fail to find a report here and return kBusyError. OK, that’s good. Next question: what happens if the uploader crashes? Is there any way to reap reports that say they’re being uploaded but really aren’t?
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, On 2015/02/10 22:49:29, Mark Mentovai wrote: > scottmg wrote: > > Hmm, I had only been considering separate processes, assuming a single process > > would coordinate above. > > > > I guess a critical section here is sufficient. Could switch back to a mutex > (but > > has the problem with multiple crashpad instances), and the LockFileEx seems to > > better match what's being guarded. The CS should almost never be contended. > > A critical section that’s held for the same duration that the file’s locked? > That could work. Done in ps#21. Er, wait! Writing the comment below I realized the CS is unnecessary. The specific handle holds the lock, so a second in-process LockFileEx will block -- it's only accessible through this handle. So, reverted the critical section change. https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:579: OperationStatus CrashReportDatabaseWin::GetReportForUploading( On 2015/02/10 22:49:29, Mark Mentovai wrote: > scottmg wrote: > > If understand the situation you're describing, the metadata file is protected > by > > the LockFileEx, and only a report that is in state=kPending here will be > > returned (with its state then changed to kUploading). So the second requestor > > would fail to find a report here and return kBusyError. > > OK, that’s good. Next question: what happens if the uploader crashes? Is there > any way to reap reports that say they’re being uploaded but really aren’t? Not at the moment. If there's only one uploader it could use a (not-yet-existing) GetUploadingReports() to either retry or blindly SkipReportUpload. Similarly, the CrashReportDatabase could do this itself at a "startup point" by resetting kUploading to kPending, or dropping reports in that state. LockFileEx on the report to make it similar to Mac doesn't directly work here because the handle holds the lock (i.e. a second handle in the same process doesn't have access). So the uploader would have to be in process and use only that handle, rather than the current file_path interface.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, scottmg wrote: > Er, wait! Writing the comment below I realized the CS is unnecessary. The > specific handle holds the lock, so a second in-process LockFileEx will block -- > it's only accessible through this handle. > > So, reverted the critical section change. Sorry, maybe I should have framed the question better. Really, I was trying to get at whether LockFileEx() guards threads against other threads, or if it just guards processes against other processes. As long as it guards threads in the same process against one another, LockFileEx() alone is ideal. https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:579: OperationStatus CrashReportDatabaseWin::GetReportForUploading( I’d actually like to change to an interface where we pass around a FileHandle. We may still need a FilePath too, so that the uploader can know the filename. I don’t think that the various parts of the system should have to open the same file multiple times, and I’m happy for the interface to be a FileHandle instead of a FilePath. That’s a lot of churn to tack on to this already-big change, so let’s save it for now, as long as a broad picture of how this should change is captured here in a comment.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, On 2015/02/10 23:28:37, scottmg wrote: > On 2015/02/10 22:49:29, Mark Mentovai wrote: > > scottmg wrote: > > > Hmm, I had only been considering separate processes, assuming a single > process > > > would coordinate above. > > > > > > I guess a critical section here is sufficient. Could switch back to a mutex > > (but > > > has the problem with multiple crashpad instances), and the LockFileEx seems > to > > > better match what's being guarded. The CS should almost never be contended. > > > > A critical section that’s held for the same duration that the file’s locked? > > That could work. > > Done in ps#21. > > Er, wait! Writing the comment below I realized the CS is unnecessary. The > specific handle holds the lock, so a second in-process LockFileEx will block -- > it's only accessible through this handle. > > So, reverted the critical section change. Oh dear. And in writing a test for this, I realized that the dwShareMode that LoggingOpenFileForWrite uses (no sharing) means that the LockFileEx is unnecessary, because other attempts to open the file will fail and return kBusyError (before they even get to LockFileEx). This might be OK (in which case we can delete the LockFileEx code) but it's differently granular than Mac because kBusyError only applies to concurrent access to a single report. OTOH, on Windows, the lock is not outstanding for any "long" operations like writing the crash dump or uploading it, only for operations inside the Metadata which are of the form read/change state/write.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, On 2015/02/10 23:37:03, Mark Mentovai wrote: > scottmg wrote: > > Er, wait! Writing the comment below I realized the CS is unnecessary. The > > specific handle holds the lock, so a second in-process LockFileEx will block > -- > > it's only accessible through this handle. > > > > So, reverted the critical section change. > > Sorry, maybe I should have framed the question better. Really, I was trying to > get at whether LockFileEx() guards threads against other threads, or if it just > guards processes against other processes. As long as it guards threads in the > same process against one another, LockFileEx() alone is ideal. Got it. LockFileEx doesn't strictly protect access amongst threads. It does in this usage though because the lock is attached to the handle, not the file, and we always (attempt) to open a fresh handle here. So, if the open were to succeed, this LockFileEx would block vs. all other users (whether in different threads or different processes). https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:579: OperationStatus CrashReportDatabaseWin::GetReportForUploading( On 2015/02/10 23:37:03, Mark Mentovai wrote: > I’d actually like to change to an interface where we pass around a FileHandle. > We may still need a FilePath too, so that the uploader can know the filename. I > don’t think that the various parts of the system should have to open the same > file multiple times, and I’m happy for the interface to be a FileHandle instead > of a FilePath. > > That’s a lot of churn to tack on to this already-big change, so let’s save it > for now, as long as a broad picture of how this should change is captured here > in a comment. OK. Attempted to describe what we will need to do here.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, scottmg wrote: > Oh dear. And in writing a test for this, I realized that the dwShareMode that > LoggingOpenFileForWrite uses (no sharing) means that the LockFileEx is > unnecessary, because other attempts to open the file will fail and return > kBusyError (before they even get to LockFileEx). > > This might be OK (in which case we can delete the LockFileEx code) but it's > differently granular than Mac because kBusyError only applies to concurrent > access to a single report. I’m not sure if you’re suggesting just allowing this to fail. I think it would be pretty bad if an attempt to operate on metadata could result in a failure to do so because someone else has the lock. Whatever will be doing the dump-writing and uploading will be expecting to be able to do metadata operations, and I think that in both cases, if there’s any lock contention, we’d want for them to block. > OTOH, on Windows, the lock is not outstanding for any "long" operations like > writing the crash dump or uploading it, only for operations inside the Metadata > which are of the form read/change state/write. Yeah, it’s good that we’re not expecting anyone to hold the lock for very long. https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, scottmg wrote: > Got it. LockFileEx doesn't strictly protect access amongst threads. It does in > this usage though because the lock is attached to the handle, not the file, and > we always (attempt) to open a fresh handle here. So, if the open were to > succeed, this LockFileEx would block vs. all other users (whether in different > threads or different processes). OK, so the lock isn’t really process-based or thread-based, it’s handle-based. That makes sense. https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:579: OperationStatus CrashReportDatabaseWin::GetReportForUploading( scottmg wrote: > OK. Attempted to describe what we will need to do here. The new comment is good.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, On 2015/02/11 00:31:35, Mark Mentovai wrote: > scottmg wrote: > > Oh dear. And in writing a test for this, I realized that the dwShareMode that > > LoggingOpenFileForWrite uses (no sharing) means that the LockFileEx is > > unnecessary, because other attempts to open the file will fail and return > > kBusyError (before they even get to LockFileEx). > > > > This might be OK (in which case we can delete the LockFileEx code) but it's > > differently granular than Mac because kBusyError only applies to concurrent > > access to a single report. > > I’m not sure if you’re suggesting just allowing this to fail. I think it would > be pretty bad if an attempt to operate on metadata could result in a failure to > do so because someone else has the lock. Whatever will be doing the dump-writing > and uploading will be expecting to be able to do metadata operations, and I > think that in both cases, if there’s any lock contention, we’d want for them to > block. That makes sense. I dithered a little on this, but rather than switch LoggingOpenFileForWrite to a shared mode (it feels unusual to have a generic writing function do that), I used CreateFile directly here. This means that the file open will succeed now, and a second concurrent access will block until the lock is released. [[ Does that mean CrashReportDatabaseMac::ObtainReportLock's open shouldn't be O_NONBLOCK? e.g. Would concurrent LookUpCrashReport()s would fail with kBusyError currently? ]] > > > OTOH, on Windows, the lock is not outstanding for any "long" operations like > > writing the crash dump or uploading it, only for operations inside the > Metadata > > which are of the form read/change state/write. > > Yeah, it’s good that we’re not expecting anyone to hold the lock for very long.
https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/520001/client/crash_report_dat... client/crash_report_database_win.cc:245: if (!LockFileEx(handle, On 2015/02/11 01:13:40, scottmg wrote: > [[ > Does that mean CrashReportDatabaseMac::ObtainReportLock's open shouldn't be > O_NONBLOCK? e.g. Would concurrent LookUpCrashReport()s would fail with > kBusyError currently? > ]] It would only return kBusyError if the concurrent accesses were for the same report, since the metadata is stored per-report rather than in aggregate as a single file. If we think this is a problem, then ObtainReportLock could have an option to use either O_SHLOCK or O_EXLOCK to control reader vs reader/writer semantics in the database. https://codereview.chromium.org/867363003/diff/640001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/640001/client/crash_report_dat... client/crash_report_database_win.cc:264: // If Read() fails, for whatever reason (corruption, etc.) metadata will not A log when this happens would be helpful. Should all of the checks in Metadata::Read() log when they early return?
https://codereview.chromium.org/867363003/diff/640001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/640001/client/crash_report_dat... client/crash_report_database_win.cc:264: // If Read() fails, for whatever reason (corruption, etc.) metadata will not On 2015/02/11 03:16:07, Robert Sesek wrote: > A log when this happens would be helpful. Should all of the checks in > Metadata::Read() log when they early return? My understanding was that the idea was to survive in the face of broken disks, drivers, etc. in the field. So if we're planning on capturing logs from the field that seems useful. For local usage, it doesn't seem worthwhile to add explicit messages for each case. Switched back to returning a success bool to use here.
LGTM https://codereview.chromium.org/867363003/diff/640001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/640001/client/crash_report_dat... client/crash_report_database_win.cc:264: // If Read() fails, for whatever reason (corruption, etc.) metadata will not On 2015/02/11 04:39:29, scottmg wrote: > On 2015/02/11 03:16:07, Robert Sesek wrote: > > A log when this happens would be helpful. Should all of the checks in > > Metadata::Read() log when they early return? > > My understanding was that the idea was to survive in the face of broken disks, > drivers, etc. in the field. So if we're planning on capturing logs from the > field that seems useful. For local usage, it doesn't seem worthwhile to add > explicit messages for each case. Switched back to returning a success bool to > use here. I was thinking Crashpad may want to log its output to a file, so that users can send it in for review, in the event of an error. I'll leave that for Mark to comment on, though. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:216: DCHECK(read_from == original); DCHECK_EQ
https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:21: #include <limits> Unused? https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:54: //! directory could not be created, returns false. Otherwise, returns true, Tiny minor nit: true and false in their literal sense as used here are surrounded by `backticks` for documentation formatting. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:143: friend class CrashReportDatabaseWin; Friends go in the private section. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:175: OVERLAPPED overlapped = {0}; Duplicate the “not actually async” comment from where you LockFileEx(). https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:202: }; I think it’s worth making the padding explicit with reserved/unused fields as needed. Looks like you’re 7 bytes away from filling the structure up to the next 64-bit boundary, so a uint8_t[7] should work. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:216: DCHECK(read_from == original); DCHECK_EQ(a, b) instead of DCHECK(a == b). Also, as long as you’re DCHECKing, you should DCHECK_GE(end, read_from) prior to the subtraction. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:218: std::vector<uint8_t> buffer; These two lines can be combined into one: std::vector<uint8_t> buffer(data_length). https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:267: // crash reports have been lost. Orphaned. The reports are still on disk (for now), right? They’re just unknown to the database. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:281: if (LoggingSeekFile(handle_.get(), 0, SEEK_SET) == -1) Add a private helper method or function to rewind the file, since you do the same rewind in Metadata::Write(). In the helper, DCHECK() that the position actually went to 0. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:289: return false; Log something about the mismatch, and about any other failure condition. When the metadata database gets tossed out, it’ll be useful to have one more breadcrumb than “failed to read metadata; database reset”. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:305: const char* string_table = reinterpret_cast<const char*>(&string_buffer[0]); This is fine if you have a reason to maintain the uint8_t[] interface, but since you want to get at the data as a const char*, why not use a std::vector<char> for string_buffer instead of a std::vector<uint8_t>? Or how about just a std::string, which matches the type you use for the string table in Write()? (A scoped_ptr<char[]> would also avoid having to initialize all of the memory pointlessly before overwriting it with the string table, but it’s also more cumbersome to use because it doesn’t carry its own size, so there’s no real insistence on using that instead of these other options.) https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:341: if (!LoggingWriteFile(handle_.get(), &header, sizeof(header))) You’re recycling the existing file, right? Shouldn’t you truncate it before writing? I’m afraid that if Write() only partially succeeds, you’ll have entries pointing to the wrong file offsets if, say, the records are written but the string table isn’t. It would be best if the metadata could be replaced atomically in one fell swoop, but barring that, we should avoid writing something that could be incorrectly interpreted. If we truncate the file first and Write() doesn’t complete, the next Read() can unambiguously recognize that the file is garbage because at least one record will point to something that’s out of bounds in the string table. Without truncating, it’s anyone’s best guess—quite possibly, Read() will read garbage without realizing it, and the garbage will become the new database of record. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:353: if (path.substr(0, report_dir_.value().size()) != report_dir_.value()) { For this to be correct, you need to ensure that report_dir_ ends in '\'. If it doesn’t, you need to pretend that it did. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:359: &string_table, path.substr(report_dir_.value().size() + 1)); Take extra care with that concern on this line too. There must not be any leading backslashes in the string that gets written to the metadata file, even if the path you were working with was non-canonical but usable, like c:\crashdb\reports\\\uuid.dmp. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:435: (fileattr & FILE_ATTRIBUTE_DIRECTORY) != 0) { If it’s a directory, that can be kFilesystemError. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:554: // Take ownership of the report, and cast to our private version with UUID. Do this first, to ensure that the resource is owned (and disposed of) even if an early return shows up in some later version of this method. https://codereview.chromium.org/867363003/diff/660001/util/test/scoped_temp_d... File util/test/scoped_temp_dir_win.cc (right): https://codereview.chromium.org/867363003/diff/660001/util/test/scoped_temp_d... util/test/scoped_temp_dir_win.cc:44: for (int count = 0; count < 50; ++count) { 50 should be a constant local to this file, in the unnamed namespace above. https://codereview.chromium.org/867363003/diff/660001/util/test/scoped_temp_d... util/test/scoped_temp_dir_win.cc:60: for (int count = 0; count < 50; ++count) { because you use it here too.
Robert Sesek wrote: > I was thinking Crashpad may want to log its output to a file, so that users can > send it in for review, in the event of an error. I'll leave that for Mark to > comment on, though. A future direction is for Crashpad to capture its own log messages and make them available to us (possibly by including them in the dump) to give us a better idea of how good a job it’s doing. Even before we get to that point, I envision a desire to know why Crashpad is making the decisions it’s making even if it’s based on users manually providing logs. If we find in the future that the database is being reset frequently and we want to make it more robust, we’ll want to know which of the 10 or so possible causes are the ones to investigate.
New patchsets have been uploaded after l-g-t-m from rsesek@chromium.org
Patchset #27 (id:680001) has been deleted
Patchset #27 (id:700001) has been deleted
Patchset #27 (id:710001) has been deleted
https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:21: #include <limits> On 2015/02/11 17:25:50, Mark Mentovai wrote: > Unused? Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:54: //! directory could not be created, returns false. Otherwise, returns true, On 2015/02/11 17:25:50, Mark Mentovai wrote: > Tiny minor nit: true and false in their literal sense as used here are > surrounded by `backticks` for documentation formatting. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:143: friend class CrashReportDatabaseWin; On 2015/02/11 17:25:50, Mark Mentovai wrote: > Friends go in the private section. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:175: OVERLAPPED overlapped = {0}; On 2015/02/11 17:25:50, Mark Mentovai wrote: > Duplicate the “not actually async” comment from where you LockFileEx(). Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:202: }; On 2015/02/11 17:25:49, Mark Mentovai wrote: > I think it’s worth making the padding explicit with reserved/unused fields as > needed. Looks like you’re 7 bytes away from filling the structure up to the next > 64-bit boundary, so a uint8_t[7] should work. Done (padded header to 64b too). Any particular reason? https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:216: DCHECK(read_from == original); On 2015/02/11 17:25:50, Mark Mentovai wrote: > DCHECK_EQ(a, b) instead of DCHECK(a == b). Also, as long as you’re DCHECKing, > you should DCHECK_GE(end, read_from) prior to the subtraction. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:216: DCHECK(read_from == original); On 2015/02/11 17:24:31, Robert Sesek wrote: > DCHECK_EQ Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:218: std::vector<uint8_t> buffer; On 2015/02/11 17:25:50, Mark Mentovai wrote: > These two lines can be combined into one: std::vector<uint8_t> > buffer(data_length). Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:267: // crash reports have been lost. On 2015/02/11 17:25:50, Mark Mentovai wrote: > Orphaned. The reports are still on disk (for now), right? They’re just unknown > to the database. Right. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:281: if (LoggingSeekFile(handle_.get(), 0, SEEK_SET) == -1) On 2015/02/11 17:25:50, Mark Mentovai wrote: > Add a private helper method or function to rewind the file, since you do the > same rewind in Metadata::Write(). In the helper, DCHECK() that the position > actually went to 0. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:289: return false; On 2015/02/11 17:25:50, Mark Mentovai wrote: > Log something about the mismatch, and about any other failure condition. When > the metadata database gets tossed out, it’ll be useful to have one more > breadcrumb than “failed to read metadata; database reset”. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:305: const char* string_table = reinterpret_cast<const char*>(&string_buffer[0]); On 2015/02/11 17:25:50, Mark Mentovai wrote: > This is fine if you have a reason to maintain the uint8_t[] interface, but since > you want to get at the data as a const char*, why not use a std::vector<char> > for string_buffer instead of a std::vector<uint8_t>? Or how about just a > std::string, which matches the type you use for the string table in Write()? > > (A scoped_ptr<char[]> would also avoid having to initialize all of the memory > pointlessly before overwriting it with the string table, but it’s also more > cumbersome to use because it doesn’t carry its own size, so there’s no real > insistence on using that instead of these other options.) Done (as std::string) https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:341: if (!LoggingWriteFile(handle_.get(), &header, sizeof(header))) On 2015/02/11 17:25:50, Mark Mentovai wrote: > You’re recycling the existing file, right? Shouldn’t you truncate it before > writing? I’m afraid that if Write() only partially succeeds, you’ll have entries > pointing to the wrong file offsets if, say, the records are written but the > string table isn’t. > > It would be best if the metadata could be replaced atomically in one fell swoop, > but barring that, we should avoid writing something that could be incorrectly > interpreted. If we truncate the file first and Write() doesn’t complete, the > next Read() can unambiguously recognize that the file is garbage because at > least one record will point to something that’s out of bounds in the string > table. Without truncating, it’s anyone’s best guess—quite possibly, Read() will > read garbage without realizing it, and the garbage will become the new database > of record. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:353: if (path.substr(0, report_dir_.value().size()) != report_dir_.value()) { On 2015/02/11 17:25:50, Mark Mentovai wrote: > For this to be correct, you need to ensure that report_dir_ ends in '\'. If it > doesn’t, you need to pretend that it did. I'm not sure why I didn't use FilePath members. Fixed. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:359: &string_table, path.substr(report_dir_.value().size() + 1)); On 2015/02/11 17:25:50, Mark Mentovai wrote: > Take extra care with that concern on this line too. There must not be any > leading backslashes in the string that gets written to the metadata file, even > if the path you were working with was non-canonical but usable, like > c:\crashdb\reports\\\uuid.dmp. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:435: (fileattr & FILE_ATTRIBUTE_DIRECTORY) != 0) { On 2015/02/11 17:25:50, Mark Mentovai wrote: > If it’s a directory, that can be kFilesystemError. Done. https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:554: // Take ownership of the report, and cast to our private version with UUID. On 2015/02/11 17:25:49, Mark Mentovai wrote: > Do this first, to ensure that the resource is owned (and disposed of) even if an > early return shows up in some later version of this method. Done. https://codereview.chromium.org/867363003/diff/660001/util/test/scoped_temp_d... File util/test/scoped_temp_dir_win.cc (right): https://codereview.chromium.org/867363003/diff/660001/util/test/scoped_temp_d... util/test/scoped_temp_dir_win.cc:44: for (int count = 0; count < 50; ++count) { On 2015/02/11 17:25:50, Mark Mentovai wrote: > 50 should be a constant local to this file, in the unnamed namespace above. Done. https://codereview.chromium.org/867363003/diff/660001/util/test/scoped_temp_d... util/test/scoped_temp_dir_win.cc:60: for (int count = 0; count < 50; ++count) { On 2015/02/11 17:25:50, Mark Mentovai wrote: > because you use it here too. Done.
LGTM. Nice job! https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/660001/client/crash_report_dat... client/crash_report_database_win.cc:202: }; scottmg wrote: > Done (padded header to 64b too). Any particular reason? Yeah, when you’re writing something to disk, I always try to be mindful of running on architectures that enforce stricter alignment requirements. The pack=1 would have taken care of the padding, but it still would have resulted in reading an array of MetadataFileReportRecords where subsequent elements might have had unaligned fields. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:316: if (string_table.empty() || string_table.back() != 0) { Use '\0' since this is looking at a char now. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:350: SetEndOfFile(handle_.get()); Check the return value and PLOG and return on failure. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:368: memset(records.get(), 0, sizeof(MetadataFileReportRecord) * num_records); Good, I was hoping to see this. #include <string.h>. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:375: LOG(ERROR) << path.value().c_str() << " expected to start with " I didn’t think you needed the c_str() here or on the next line, but if you do (because you can use wchar_t* but not wstring?), it’s fine to keep. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:376: << report_dir_.value().c_str() << " and does not"; “and does not” is kind of obvious, save a few bytes.
New patchsets have been uploaded after l-g-t-m from mark@chromium.org
Thanks for ploughing through. :) https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:316: if (string_table.empty() || string_table.back() != 0) { On 2015/02/11 19:39:56, Mark Mentovai wrote: > Use '\0' since this is looking at a char now. Done. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:350: SetEndOfFile(handle_.get()); On 2015/02/11 19:39:56, Mark Mentovai wrote: > Check the return value and PLOG and return on failure. Done. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:368: memset(records.get(), 0, sizeof(MetadataFileReportRecord) * num_records); On 2015/02/11 19:39:56, Mark Mentovai wrote: > Good, I was hoping to see this. #include <string.h>. Done. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:375: LOG(ERROR) << path.value().c_str() << " expected to start with " On 2015/02/11 19:39:56, Mark Mentovai wrote: > I didn’t think you needed the c_str() here or on the next line, but if you do > (because you can use wchar_t* but not wstring?), it’s fine to keep. Yeah, there's no operator<< for wstring in ostream, and logging doesn't have its own. https://codereview.chromium.org/867363003/diff/730001/client/crash_report_dat... client/crash_report_database_win.cc:376: << report_dir_.value().c_str() << " and does not"; On 2015/02/11 19:39:56, Mark Mentovai wrote: > “and does not” is kind of obvious, save a few bytes. Done.
LGTM
Message was sent while issue was closed.
Committed patchset #28 (id:750001) manually as 0849154aedd9262d5a38a4a0c9c019e644bce906 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
