Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2327043002:
A simple minidump writer for postmortem stability reports. (Closed)
|
Created:
4 years, 3 months ago by manzagop (departed) Modified:
4 years, 3 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionA simple minidump writer for postmortem stability reports.
Enables writing a minimal minidump for wrapping a StabilityReport protocol buffer. The minidump is meant to be valid for registration with the Crashpad reporter (includes the Crashpad info stream).
BUG=620813
Committed: https://crrev.com/a28851ca731cf926f1c7949e4c1293a059606958
Cr-Commit-Position: refs/heads/master@{#418863}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address comments #Patch Set 3 : Merge #
Total comments: 10
Patch Set 4 : Address comments #
Total comments: 14
Patch Set 5 : Address comments #
Total comments: 4
Patch Set 6 : Address Scott's last round #Patch Set 7 : Missing BUILD dependencies #
Total comments: 4
Patch Set 8 : Address Siggi's last round #Patch Set 9 : Signed/unsigned comparison fixups. #Patch Set 10 : More signed/unsigned caught by the clang builder #Patch Set 11 : Missed the hex number #
Dependent Patchsets: Messages
Total messages: 56 (32 generated)
manzagop@chromium.org changed reviewers: + siggi@chromium.org
Here's the minidump writer broken out of https://crrev.com/2128683002 . Note it could avoid the dependency on the proto by simply taking a buffer and a minidump stream id to stuff it in, but not sure it's worth making that change since this is throw away code. Please have a look!
Very nice - I don't care where the proto dependency ends up. If the argument list to WriteDump grows any, I think it'd be preferable to go to a member variable setter interface with a no-param Write(). Otherwise maybe a DumpParams struct would do it, but so far so good. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:33: using Position = uint32_t; nit: maybe FilePosition? Also, it doesn't look like this needs to be public? https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:95: Position cursor_; is this synonymous with the position of minidump_file_? https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:101: base::PlatformFile minidump_file_; // Not owned. why not use a base::File or File*? This provides the platform-independent shim functions to :WriteFile et al? https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:108: return WriteBytes(pos, sizeof(data), &data); TIL: you can static_assert(std::is_pod<DataType>::value, ...) to make sure nobody writes non PODs by accident. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer_win.cc:34: CHECK(minidump_file); compare to base::kInvalidPlatformFile, or better use base::File::IsValid https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer_win.cc:42: DCHECK_NE(base::kInvalidPlatformFile, minidump_file_); Check that the file pos is at head - maybe you also want the file empty here? https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer_win.cc:116: MINIDUMP_LOCATION_DESCRIPTOR simple_annotations; initialize with {}? https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... File components/browser_watcher/stability_report.proto (right): https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/stability_report.proto:60: repeated CodeModule modules = 1; nit: remark that this most likely will include only a small selection of modules, perhaps only our own binary info? https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/stability_report.proto:66: // single logical instance of a "chrome browser". It it comprised of information nit: It it*
Patchset #2 (id:20001) has been deleted
Thanks! Another look? https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:33: using Position = uint32_t; On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > nit: maybe FilePosition? Also, it doesn't look like this needs to be public? Done. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:95: Position cursor_; On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > is this synonymous with the position of minidump_file_? It is. Its only use was to DCHECK-validate there's no need to seek the file. Changed the DCHECK to query the file instead. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:101: base::PlatformFile minidump_file_; // Not owned. On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > why not use a base::File or File*? This provides the platform-independent shim > functions to :WriteFile et al? As discussed, switched to File but needed to duplicate the handle. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer.h:108: return WriteBytes(pos, sizeof(data), &data); On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > TIL: you can static_assert(std::is_pod<DataType>::value, ...) to make sure > nobody writes non PODs by accident. Oh cool! Hm, looks like I can't use that because MinidumpCrashpadInfo contains a UUID which is non-POD. :( https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/minidump/m... https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer_win.cc:34: CHECK(minidump_file); On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > compare to base::kInvalidPlatformFile, or better use base::File::IsValid Done. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer_win.cc:42: DCHECK_NE(base::kInvalidPlatformFile, minidump_file_); On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > Check that the file pos is at head - maybe you also want the file empty here? Done. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/postmortem_minidump_writer_win.cc:116: MINIDUMP_LOCATION_DESCRIPTOR simple_annotations; On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > initialize with {}? Done. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... File components/browser_watcher/stability_report.proto (right): https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/stability_report.proto:60: repeated CodeModule modules = 1; On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > nit: remark that this most likely will include only a small selection of > modules, perhaps only our own binary info? Done. https://chromiumcodereview.appspot.com/2327043002/diff/1/components/browser_w... components/browser_watcher/stability_report.proto:66: // single logical instance of a "chrome browser". It it comprised of information On 2016/09/09 14:21:00, Sigurður Ásgeirsson wrote: > nit: It it* Done.
Ping!
On 2016/09/12 20:37:32, manzagop wrote: > Ping! I'm missing the trailing ";)" ;).
nice - got some nits. https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:101: std::unique_ptr<base::File> minidump_file_; IMHO simpler to use a base::File* here, and scope the instance in the WriteDump function, as mentioned below. https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:107: crashpad::UUID client_id; nit: maybe a little bit of documentation? https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:122: // Note: we cannot static_assert that std::is_pod<DataType>::value, because No need for the note, if we can't do it, we can't do it. Are there weaker <type_traits> or assertions we can usefully make (don't sweat it overmuch, this is a nice-to-have)? https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:27: class ScopedFileCloser { this looks pretty contrived. A simpler thing to do is to have a naked base::File* in the class, and base::File instance in the function? https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:51: int64_t offset = file->Seek(base::File::FROM_CURRENT, 0ULL); I wouldn't bother with the gold plating here - just do the check and bail otherwise. If your preconditions are violated, you shouldn't try and repair the world. You could even name the function something like AssertFileIsEmpty...
Addressed comments. Another look? https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:101: std::unique_ptr<base::File> minidump_file_; On 2016/09/13 14:57:09, Sigurður Ásgeirsson wrote: > IMHO simpler to use a base::File* here, and scope the instance in the WriteDump > function, as mentioned below. Done. https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:107: crashpad::UUID client_id; On 2016/09/13 14:57:09, Sigurður Ásgeirsson wrote: > nit: maybe a little bit of documentation? Done. https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:122: // Note: we cannot static_assert that std::is_pod<DataType>::value, because On 2016/09/13 14:57:09, Sigurður Ásgeirsson wrote: > No need for the note, if we can't do it, we can't do it. > Are there weaker <type_traits> or assertions we can usefully make (don't sweat > it overmuch, this is a nice-to-have)? Found is_trivially_copyable. Done. https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:27: class ScopedFileCloser { On 2016/09/13 14:57:09, Sigurður Ásgeirsson wrote: > this looks pretty contrived. A simpler thing to do is to have a naked > base::File* in the class, and base::File instance in the function? Omg! The perils of iterative changes. Done. https://chromiumcodereview.appspot.com/2327043002/diff/60001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:51: int64_t offset = file->Seek(base::File::FROM_CURRENT, 0ULL); On 2016/09/13 14:57:09, Sigurður Ásgeirsson wrote: > I wouldn't bother with the gold plating here - just do the check and bail > otherwise. If your preconditions are violated, you shouldn't try and repair the > world. > You could even name the function something like AssertFileIsEmpty... Done.
manzagop@chromium.org changed reviewers: + scottmg@chromium.org
Hi Scott, Could you have a look? This is basically what you reviewed eons ago in https://crrev.com/2128683002/ but with changes to integrate with Crashpad, mainly: - takes a platformfile (note the approach of duplicating the handle in order to use base::File) - writes a crashpadinfo stream I'll need your OWNERS blessing for the DEPS change. Thanks! Pierre
nice, a couple of more nits... https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:102: base::File* minidump_file_; Do you need to null-initialize this in a constructor, or is does a "= default" constructor do that? https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:34: bool AssertFileEmpty(base::File* file) { Sorry, should have been clearer - I'd either DCHECK() inside this function, return void and name it AssertXXX. Else name it something like IsXXX and return bool. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:68: ::GetCurrentProcess(), minidump_platform_file, GetCurrentProcess(), nit: ::GetCurrentProcess https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:211: DCHECK_NE(nullptr, minidump_file_); yeah, these checks will never trip, as you never null the pointer. Null it in constructor, check on entry to WriteDump, and null it on exit? Maybe move the actual work to a WriteDumpImpl function, and leave the setup in the interface function?
Cool, thanks for breaking it up, the full change was overwhelming. :) Just a couple small comments. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/BUILD.gn (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/BUILD.gn:45: # TODO(manzagop): remove this lib once CrashPad writes the minidumps. nit; CrashPad -> Crashpad, and elsewhere. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:31: class PostmortemMinidumpWriter { This seems like it could just be a WriteDump() function, with the whole class in the .cc, since there's only one method and the constructor doesn't do anything. Then you could avoid the templated method bodies being in the header, too. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:64: // We do not own |minidump_platform_file|, but we want to rely on base::File's Bleh, never liked base::File. :( I think I'd probably avoid ::DuplicateHandle() by having a ScopedFileOwnershipGrant (or something) that creates the base::File and then TakePlatformFile()s it back in ~. But if that gets ugly this is fine too.
Thanks for the comments. Please have another look! https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/BUILD.gn (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/BUILD.gn:45: # TODO(manzagop): remove this lib once CrashPad writes the minidumps. On 2016/09/13 18:07:43, scottmg wrote: > nit; CrashPad -> Crashpad, and elsewhere. Done. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:31: class PostmortemMinidumpWriter { On 2016/09/13 18:07:43, scottmg wrote: > This seems like it could just be a WriteDump() function, with the whole class in > the .cc, since there's only one method and the constructor doesn't do anything. > Then you could avoid the templated method bodies being in the header, too. Done. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer.h:102: base::File* minidump_file_; On 2016/09/13 17:36:56, Sigurður Ásgeirsson wrote: > Do you need to null-initialize this in a constructor, or is does a "= default" > constructor do that? No you're right. =default is equivalent to {} except it doesn't prevent the type from being POD or trivial. I've added a constructor. Done. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:34: bool AssertFileEmpty(base::File* file) { On 2016/09/13 17:36:56, Sigurður Ásgeirsson wrote: > Sorry, should have been clearer - I'd either DCHECK() inside this function, > return void and name it AssertXXX. > Else name it something like IsXXX and return bool. I've gone with the external dcheck to avoid the seek in non-debug. Done. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:64: // We do not own |minidump_platform_file|, but we want to rely on base::File's On 2016/09/13 18:07:43, scottmg wrote: > Bleh, never liked base::File. :( I think I'd probably avoid ::DuplicateHandle() > by having a ScopedFileOwnershipGrant (or something) that creates the base::File > and then TakePlatformFile()s it back in ~. But if that gets ugly this is fine > too. That's actually what I started with, but it turns out it trips the handle checker. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:68: ::GetCurrentProcess(), minidump_platform_file, GetCurrentProcess(), On 2016/09/13 17:36:56, Sigurður Ásgeirsson wrote: > nit: ::GetCurrentProcess Done. https://chromiumcodereview.appspot.com/2327043002/diff/80001/components/brows... components/browser_watcher/postmortem_minidump_writer_win.cc:211: DCHECK_NE(nullptr, minidump_file_); On 2016/09/13 17:36:56, Sigurður Ásgeirsson wrote: > yeah, these checks will never trip, as you never null the pointer. > Null it in constructor, check on entry to WriteDump, and null it on exit? Maybe > move the actual work to a WriteDumpImpl function, and leave the setup in the > interface function? Done.
lgtm https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... components/browser_watcher/postmortem_minidump_writer.h:10: #include <map> Many of these includes can move into the .cc now. https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... File components/browser_watcher/postmortem_minidump_writer_win_unittest.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... components/browser_watcher/postmortem_minidump_writer_win_unittest.cc:118: TEST_F(WritePostmortemDumpTest, CrashPadCanReadTest) { one more Pad here :)
Description was changed from ========== A simple minidump writer for postmortem stability reports. BUG=620813 ========== to ========== A simple minidump writer for postmortem stability reports. Enables writing a minimal minidump for wrapping a StabilityReport protocol buffer. The minidump is meant to be valid for registration with the Crashpad reporter (includes the Crashpad info stream). BUG=620813 ==========
manzagop@chromium.org changed reviewers: + rkaplow@chromium.org
Scott: thanks for the review! Rob: could you have a look at the comma I added in the DEPS file and, if it looks nice to you, bless that DEPS change with your OWNERS benediction? Thanks! https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... File components/browser_watcher/postmortem_minidump_writer.h (right): https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... components/browser_watcher/postmortem_minidump_writer.h:10: #include <map> On 2016/09/13 21:32:37, scottmg wrote: > Many of these includes can move into the .cc now. Thanks for catching. Done. https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... File components/browser_watcher/postmortem_minidump_writer_win_unittest.cc (right): https://chromiumcodereview.appspot.com/2327043002/diff/100001/components/brow... components/browser_watcher/postmortem_minidump_writer_win_unittest.cc:118: TEST_F(WritePostmortemDumpTest, CrashPadCanReadTest) { On 2016/09/13 21:32:37, scottmg wrote: > one more Pad here :) Can't sneak one past you! ;) Done.
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm DEPS file looks fine. That's so strange you need a metrics approval for adding a comma to it /mindblown
lgtm may be worth filing a bug to fix the presubmit check because that seems wrong to require for adding a comma
lgtm with but a nit. Nice. https://codereview.chromium.org/2327043002/diff/140001/components/browser_wat... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://codereview.chromium.org/2327043002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_minidump_writer_win.cc:46: int64_t end = file->Seek(base::File::FROM_END, 0LL); nit: this is GetFileOffset? https://codereview.chromium.org/2327043002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_minidump_writer_win.cc:59: ~PostmortemMinidumpWriter() = default; nit: DCHECK_EQ(nullptr, minidump_file_) in destructor?
Thanks for the reviews! Going ahead with submission. Rob: I filed http://crbug.com/646885 for the DEPS presubmit check. https://codereview.chromium.org/2327043002/diff/140001/components/browser_wat... File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://codereview.chromium.org/2327043002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_minidump_writer_win.cc:46: int64_t end = file->Seek(base::File::FROM_END, 0LL); On 2016/09/14 15:11:48, Sigurður Ásgeirsson wrote: > nit: this is GetFileOffset? No because we seek relative to the end instead of the current location. This made me notice a signed/unsigned mismatch though. https://codereview.chromium.org/2327043002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_minidump_writer_win.cc:59: ~PostmortemMinidumpWriter() = default; On 2016/09/14 15:11:48, Sigurður Ásgeirsson wrote: > nit: DCHECK_EQ(nullptr, minidump_file_) in destructor? Done.
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org, siggi@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2327043002/#ps160001 (title: "Address Siggi's last round")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org, rkaplow@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2327043002/#ps220001 (title: "Missed the hex number")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== A simple minidump writer for postmortem stability reports. Enables writing a minimal minidump for wrapping a StabilityReport protocol buffer. The minidump is meant to be valid for registration with the Crashpad reporter (includes the Crashpad info stream). BUG=620813 ========== to ========== A simple minidump writer for postmortem stability reports. Enables writing a minimal minidump for wrapping a StabilityReport protocol buffer. The minidump is meant to be valid for registration with the Crashpad reporter (includes the Crashpad info stream). BUG=620813 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== A simple minidump writer for postmortem stability reports. Enables writing a minimal minidump for wrapping a StabilityReport protocol buffer. The minidump is meant to be valid for registration with the Crashpad reporter (includes the Crashpad info stream). BUG=620813 ========== to ========== A simple minidump writer for postmortem stability reports. Enables writing a minimal minidump for wrapping a StabilityReport protocol buffer. The minidump is meant to be valid for registration with the Crashpad reporter (includes the Crashpad info stream). BUG=620813 Committed: https://crrev.com/a28851ca731cf926f1c7949e4c1293a059606958 Cr-Commit-Position: refs/heads/master@{#418863} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a28851ca731cf926f1c7949e4c1293a059606958 Cr-Commit-Position: refs/heads/master@{#418863} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
