Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2611393002:
Part two of fallback crash handler for Crashpad handler process. (Closed)
|
Created:
3 years, 11 months ago by Sigurður Ásgeirsson Modified:
3 years, 11 months ago Reviewers:
scottmg CC:
chromium-reviews, jam, darin-cc_chromium.org, sadrul, kalyank, Mark Mentovai Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPart two of fallback crash handler for Crashpad handler process.
This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it.
After this, the dump is dropped into the Crashpad database for eventual upload.
BUG=678959
Review-Url: https://codereview.chromium.org/2611393002
Cr-Original-Commit-Position: refs/heads/master@{#443249}
Committed: https://chromium.googlesource.com/chromium/src/+/92651104e939dbe94378a607e7acd381ca394fd3
Review-Url: https://codereview.chromium.org/2611393002
Cr-Commit-Position: refs/heads/master@{#443406}
Committed: https://chromium.googlesource.com/chromium/src/+/b0cb5ce73c6171b11cee16e6f512a4e0ca6a07eb
Patch Set 1 #Patch Set 2 : Merge base CL changes. #Patch Set 3 : Merge base CL changes. #Patch Set 4 : Make CommandLine param to ParseCommandLine const. #
Total comments: 46
Patch Set 5 : Address Scott's comments. Also spin a test process to dump to avoid test flakes. #Patch Set 6 : Speling [sic] and fix outdated comment. #
Total comments: 8
Patch Set 7 : Address Scott's comments moar again. #Patch Set 8 : Back out of multi-process testing, see whether the simple approach will work first. #
Total comments: 2
Patch Set 9 : Address Scott's - the eagle-eyed - comment. #Patch Set 10 : Fix Clang-reported signedness goof. #Patch Set 11 : Add a missing override specifier. #Patch Set 12 : Now remove ALL unused directory entries, instead of ONE. Fix non-const casts to constness. #
Total comments: 1
Messages
Total messages: 57 (34 generated)
siggi@chromium.org changed reviewers: + scottmg@chromium.org
Hey Scott, here's the balance from https://codereview.chromium.org/2596463002/. This is the handler and the associated test. Siggi
The CQ bit was checked by siggi@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/...)
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 ========== to ========== Part two of fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 ==========
Hey Scott, gentle nudge - also changed the title of the CL, which was identical to the launcher CL before.
The CQ bit was checked by siggi@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 checked by siggi@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/...)
https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... File components/crash/content/app/fallback_crash_handler_win.cc (right): https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:31: class MinidumpUpdater { A short block comment here on what this class is for would be nice. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:79: // CrashPad has some fairly unreasonable checking on the minidump header and Crashpad https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:84: // TODO(siggi): Fix Crashpad to ignore unused streams. """UnusedStream: Reserved. Do not use this enumeration value.""" Bah. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:91: // TODO(siggi): Fix Crashpad's version checking. In what way? How about just doing that upstream rather than the todo? https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:132: break; This is a pretty weird break. I think the normal-ish structure would be easier to read: for (const auto& entry : directory_) { if (entry.StreamType == CrashpadInfo) { fill crashpad_info_pos break; } } https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:180: !WriteAndAdvance(&entries.at(0), entry_count * sizeof(entries[0]), Why the .at(0)'s here? Don't mix entries.at(0) with entries[0] anyway. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:228: // Writes a minidump file to for |process| to |dump_file| with embedded "file to for" https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:281: // Appends the full contents of |source| to |dest| at te. "te"? https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:286: // Rewind the source . at end. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:321: unsigned uint_process = 0; unsigned int. Not necessary to 0 initialize. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:381: uint32_t minidump_type = MiniDumpWithUnloadedModules | const https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:387: exc_info.ExceptionPointers = exc_ptrs_; And then you'd need a cast here, but only right at the point where it's interfacing with system definitions that require the * type. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... File components/crash/content/app/fallback_crash_handler_win.h (right): https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.h:51: EXCEPTION_POINTERS* exc_ptrs_; I would prefer this to be maintained as a uint64_t or a uintptr_t, rather than a fake pointer since it doesn't point to anything dereference-able. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.h:53: }; #include "base/macros.h" and DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:40: DWORD kAccessMask = const https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:43: ASSERT_EQ(ERROR_SUCCESS, GetLastError()); Does OpenProcess() necessarily set GLE on success? I think only checking self.IsValid() by itself is better (and add a log of GLE if you want to have the test output that on failure). https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:62: CONTEXT ctx_; context_ and exception_ instead of ctx_ and exc_ please. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:66: base::win::ScopedHandle self_; Since you have to leak this in all usages, perhaps a raw HANDLE is a better option here. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:67: base::ScopedTempDir database_dir_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:84: EXPECT_FALSE(handler.ParseCommandLine(cmd_line)); I guess this should be ASSERT as otherwise it's tried to take the handle from process arg. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:90: // Because how handle ownership is guarded, we have to "disown" it before "Because of how..." https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:104: // terminate it. MinidumpWriteDump is alleged to occasionally hang if used MiniDump https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:149: ASSERT_EQ(expected_client_id, client_id); All the ASSERT_ from here down should be EXPECT_ I think.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by siggi@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...
Thanks - another look? https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... File components/crash/content/app/fallback_crash_handler_win.cc (right): https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:31: class MinidumpUpdater { On 2017/01/10 18:26:27, scottmg wrote: > A short block comment here on what this class is for would be nice. Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:79: // CrashPad has some fairly unreasonable checking on the minidump header and On 2017/01/10 18:26:27, scottmg wrote: > Crashpad Ugh, bad habit. Dies hard. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:84: // TODO(siggi): Fix Crashpad to ignore unused streams. On 2017/01/10 18:26:27, scottmg wrote: > """UnusedStream: Reserved. Do not use this enumeration value.""" > > Bah. Yeah, MDWD leaves a couple of unused-type entries in the stream directory. Why? Dunno. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:91: // TODO(siggi): Fix Crashpad's version checking. On 2017/01/10 18:26:28, scottmg wrote: > In what way? How about just doing that upstream rather than the todo? I'll get there, but I'm not sure what the deal is, maybe no checking is best. Note that according to <https://msdn.microsoft.com/en-us/library/windows/desktop/ms680378(v=vs.85).aspx> Version: The version of the minidump format. The low-order word is MINIDUMP_VERSION. The high-order word is an internal value that is implementation specific. Also Version from MDWD is going to be related to the current system's code, not the headers you compile against. Not a biggie to massage into CP-friendly shape for now, in any case. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:132: break; On 2017/01/10 18:26:28, scottmg wrote: > This is a pretty weird break. > > I think the normal-ish structure would be easier to read: > > for (const auto& entry : directory_) { > if (entry.StreamType == CrashpadInfo) { > fill crashpad_info_pos > break; > } > } Yeah, that reads sane - thanks. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:180: !WriteAndAdvance(&entries.at(0), entry_count * sizeof(entries[0]), On 2017/01/10 18:26:28, scottmg wrote: > Why the .at(0)'s here? Don't mix entries.at(0) with entries[0] anyway. Yeah, I guess there's no point in using at() with no exceptions :/. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:228: // Writes a minidump file to for |process| to |dump_file| with embedded On 2017/01/10 18:26:28, scottmg wrote: > "file to for" Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:281: // Appends the full contents of |source| to |dest| at te. On 2017/01/10 18:26:27, scottmg wrote: > "te"? Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:286: // Rewind the source On 2017/01/10 18:26:27, scottmg wrote: > . at end. Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:321: unsigned uint_process = 0; On 2017/01/10 18:26:27, scottmg wrote: > unsigned int. Not necessary to 0 initialize. Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:381: uint32_t minidump_type = MiniDumpWithUnloadedModules | On 2017/01/10 18:26:27, scottmg wrote: > const Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.cc:387: exc_info.ExceptionPointers = exc_ptrs_; On 2017/01/10 18:26:27, scottmg wrote: > And then you'd need a cast here, but only right at the point where it's > interfacing with system definitions that require the * type. Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... File components/crash/content/app/fallback_crash_handler_win.h (right): https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.h:51: EXCEPTION_POINTERS* exc_ptrs_; On 2017/01/10 18:26:28, scottmg wrote: > I would prefer this to be maintained as a uint64_t or a uintptr_t, rather than a > fake pointer since it doesn't point to anything dereference-able. Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win.h:53: }; On 2017/01/10 18:26:28, scottmg wrote: > #include "base/macros.h" and DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:40: DWORD kAccessMask = On 2017/01/10 18:26:28, scottmg wrote: > const Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:43: ASSERT_EQ(ERROR_SUCCESS, GetLastError()); On 2017/01/10 18:26:28, scottmg wrote: > Does OpenProcess() necessarily set GLE on success? I think only checking > self.IsValid() by itself is better (and add a log of GLE if you want to have the > test output that on failure). Good point - so amended. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:62: CONTEXT ctx_; On 2017/01/10 18:26:28, scottmg wrote: > context_ and exception_ instead of ctx_ and exc_ please. Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:66: base::win::ScopedHandle self_; On 2017/01/10 18:26:28, scottmg wrote: > Since you have to leak this in all usages, perhaps a raw HANDLE is a better > option here. Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:67: base::ScopedTempDir database_dir_; On 2017/01/10 18:26:28, scottmg wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:84: EXPECT_FALSE(handler.ParseCommandLine(cmd_line)); On 2017/01/10 18:26:28, scottmg wrote: > I guess this should be ASSERT as otherwise it's tried to take the handle from > process arg. Done, though this process is pretty horked either way. At least there'll be a clean failure here. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:90: // Because how handle ownership is guarded, we have to "disown" it before On 2017/01/10 18:26:28, scottmg wrote: > "Because of how..." Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:104: // terminate it. MinidumpWriteDump is alleged to occasionally hang if used On 2017/01/10 18:26:28, scottmg wrote: > MiniDump Done. https://codereview.chromium.org/2611393002/diff/60001/components/crash/conten... components/crash/content/app/fallback_crash_handler_win_unittest.cc:149: ASSERT_EQ(expected_client_id, client_id); On 2017/01/10 18:26:28, scottmg wrote: > All the ASSERT_ from here down should be EXPECT_ I think. Done.
Cool, lgtm. I'm excited to find out what stupid bugs I wrote in Crashpad. https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.cc (right): https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.cc:31: // This class is a helper to edit minidump files written by MinidumpWriteDump. MiniDumpWriteDump https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.cc:439: status = database->FinishedWritingCrashReport(report, &report_id); These crashes are going to show up in the UMA, e.g. for Metrics::CrashReportPending() as called here. I think that's OK, but we'll just have to be aware that it's going to mess with the numbers a bit. https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.h (right): https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:52: uintptr_t exc_ptrs_; exception instead of exc https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win_unittest.cc:29: // MinidumpWriteDump on one's own process, as that can hang or flake out in same, and another below
Comments addressed, but this'll need some more work. Turns out that I can't dump the sleeper with PIDs and exception pointers from MY process. I'm going to go cry into my Martini - brace for impact in the morn'. https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.cc (right): https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.cc:31: // This class is a helper to edit minidump files written by MinidumpWriteDump. On 2017/01/10 21:33:39, scottmg wrote: > MiniDumpWriteDump Good lord - I though I'd global-replaced those. Thanks! https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.cc:439: status = database->FinishedWritingCrashReport(report, &report_id); On 2017/01/10 21:33:39, scottmg wrote: > These crashes are going to show up in the UMA, e.g. for > Metrics::CrashReportPending() as called here. I think that's OK, but we'll just > have to be aware that it's going to mess with the numbers a bit. Oh - really? Who takes care of setting up the persistent UMA thingy? https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.h (right): https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:52: uintptr_t exc_ptrs_; On 2017/01/10 21:33:40, scottmg wrote: > exception instead of exc Done. https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2611393002/diff/120001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win_unittest.cc:29: // MinidumpWriteDump on one's own process, as that can hang or flake out in On 2017/01/10 21:33:40, scottmg wrote: > same, and another below Done.
The CQ bit was checked by siggi@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...
Hey Scott, one quick, last look? I backed out of the broken multiprocess testing. Since the other CL was failing for knowable reasons, I figure I might as well try for the simple, stupid approach here first? Siggi
https://codereview.chromium.org/2611393002/diff/160001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2611393002/diff/160001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win_unittest.cc:57: return base::UintToString(reinterpret_cast<uintptr_t>(&exception_ptrs_)); Uint64?
Thanks - one truly last look? https://codereview.chromium.org/2611393002/diff/160001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2611393002/diff/160001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win_unittest.cc:57: return base::UintToString(reinterpret_cast<uintptr_t>(&exception_ptrs_)); On 2017/01/11 19:02:53, scottmg wrote: > Uint64? Ugh, too right - thanks!
lgtm
The CQ bit was checked by siggi@chromium.org
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by siggi@chromium.org
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by siggi@chromium.org
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 siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2611393002/#ps200001 (title: "Fix Clang-reported signedness goof.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2611393002/#ps220001 (title: "Add a missing override specifier.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484234123960790,
"parent_rev": "c9c93b5a51e998ea46dce116486b03b999b344d9", "commit_rev":
"92651104e939dbe94378a607e7acd381ca394fd3"}
Message was sent while issue was closed.
Description was changed from ========== Part two of fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 ========== to ========== Part two of fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 Review-Url: https://codereview.chromium.org/2611393002 Cr-Commit-Position: refs/heads/master@{#443249} Committed: https://chromium.googlesource.com/chromium/src/+/92651104e939dbe94378a607e7ac... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/92651104e939dbe94378a607e7ac...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:220001) has been created in https://codereview.chromium.org/2628863005/ by siggi@chromium.org. The reason for reverting is: Breaks components_unittests on Win10..
Message was sent while issue was closed.
Turns out I was only removing one of the unused dir entries. I'm going to submit this and then go cry into my Martini. https://codereview.chromium.org/2611393002/diff/240001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.cc (right): https://codereview.chromium.org/2611393002/diff/240001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.cc:94: directory_.end()); Ugh :(.
Message was sent while issue was closed.
Description was changed from ========== Part two of fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 Review-Url: https://codereview.chromium.org/2611393002 Cr-Commit-Position: refs/heads/master@{#443249} Committed: https://chromium.googlesource.com/chromium/src/+/92651104e939dbe94378a607e7ac... ========== to ========== Part two of fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 Review-Url: https://codereview.chromium.org/2611393002 Cr-Commit-Position: refs/heads/master@{#443249} Committed: https://chromium.googlesource.com/chromium/src/+/92651104e939dbe94378a607e7ac... ==========
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2611393002/#ps240001 (title: "Now remove ALL unused directory entries, instead of ONE. Fix non-const casts to constness.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484257017945780,
"parent_rev": "dea291aa30b95a0d81e2ac1d2661c1d93061c5a4", "commit_rev":
"b0cb5ce73c6171b11cee16e6f512a4e0ca6a07eb"}
Message was sent while issue was closed.
Description was changed from ========== Part two of fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 Review-Url: https://codereview.chromium.org/2611393002 Cr-Commit-Position: refs/heads/master@{#443249} Committed: https://chromium.googlesource.com/chromium/src/+/92651104e939dbe94378a607e7ac... ========== to ========== Part two of fallback crash handler for Crashpad handler process. This implements the crash handler companion to the launcher in https://codereview.chromium.org/2596463002/. The handler parses the command line as created by the launcher. After this, it can write a minidump with MinidumpWriteDump, then augment it with Crashpad metadata and massage such that Crashpad can parse it. After this, the dump is dropped into the Crashpad database for eventual upload. BUG=678959 Review-Url: https://codereview.chromium.org/2611393002 Cr-Original-Commit-Position: refs/heads/master@{#443249} Committed: https://chromium.googlesource.com/chromium/src/+/92651104e939dbe94378a607e7ac... Review-Url: https://codereview.chromium.org/2611393002 Cr-Commit-Position: refs/heads/master@{#443406} Committed: https://chromium.googlesource.com/chromium/src/+/b0cb5ce73c6171b11cee16e6f512... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/b0cb5ce73c6171b11cee16e6f512... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
