Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2596463002:
A simple, practically zero cost fallback crash handler for Crashpad handler process. (Closed)
|
Created:
4 years ago by Sigurður Ásgeirsson Modified:
3 years, 11 months ago Reviewers:
scottmg CC:
chromium-reviews, jam, darin-cc_chromium.org, sadrul, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionA simple, practically zero cost fallback crash handler for Crashpad handler process.
This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking.
BUG=678959
Review-Url: https://codereview.chromium.org/2596463002
Cr-Original-Original-Commit-Position: refs/heads/master@{#442596}
Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e853d8e8c29130
Review-Url: https://codereview.chromium.org/2596463002
Cr-Original-Commit-Position: refs/heads/master@{#442698}
Committed: https://chromium.googlesource.com/chromium/src/+/f87fdaa12b09ff286d873e7a3ea459507d7adb83
Review-Url: https://codereview.chromium.org/2596463002
Cr-Commit-Position: refs/heads/master@{#442974}
Committed: https://chromium.googlesource.com/chromium/src/+/21b194219385cd209294216842e192978cc2b424
Patch Set 1 #Patch Set 2 : Launcher works, test generates sane crash reports. #Patch Set 3 : Fix build deps, start on the crash handler itself. #Patch Set 4 : Minimal implementation and testing of the handler, still needs to write CrashpadInfo to the generat… #Patch Set 5 : Write dump with MDWD, add CrashpadInfo and test that Crashpad can parse it. #Patch Set 6 : Tweak comments for moar better confusion-less reading. #
Total comments: 38
Patch Set 7 : Pass (most) crash keys as args into GenerateCrashDump. #Patch Set 8 : Address Scott's comments. #Patch Set 9 : Winnow CL down per Scott's request. #Patch Set 10 : Move dependency to Win-only as per The Truth Of The Matter(tm). #Patch Set 11 : Fix Clank comparison nit. #
Total comments: 8
Patch Set 12 : Address Scott's comments. #Patch Set 13 : Fix signedness comparison goof. #Patch Set 14 : Fix test flakiness by spinning a process to dump with MDWD. #Patch Set 15 : On 64 bit systems, pointers are large - doofus. #
Dependent Patchsets: Messages
Total messages: 59 (33 generated)
siggi@chromium.org changed reviewers: + scottmg@chromium.org
Hey Scott, this is just an FYI, not ready for review yet. The idea is to string a FallbackCrashHandlerLauncher to the crash handler's UEF. This will (sometimes, hopefully often enough) launch a new process where a FallbackCrashHandler will be instantiated. The FallbackCrashHandler will then use MinidumpWriteDump to snarf the crash dump, and then hand it off to the CrashPad database. This gives us a reasonable amount of orthogonality to the CrashPad implementation, and should allow us to diagnose everything short of full-on, 100% upload failure. Siggi
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. BUG= ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. BUG=678959 ==========
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. BUG=678959 ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher and a fallback crash hander. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. On the handler side, it parses the command line, and creates a dump with MinidumpWriteDump. This is then retouched to add the CrashpadInfo metadata, and to massage the dump into where Crashpad will parse it. BUG=678959 ==========
Hey Scott, this is ready for initial review, please take a look. I still need to add plumbing to get the right channel/version etc, into the crash keys, so there's a DO NOT SUBMIT in there. Pay no heed. I'd like to later also relax Crashpad's minidump parsing to allow it to parse dumps from MinidumpWriteDump without the massaging I need to do. Siggi
I'm going to be That Guy and ask you to break this CL up. fallback_crash_handler_launcher can land separately/before fallback_crash_handler, right? (I've only looked at the former, not the latter at all yet.) https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:23: DWORD kAccessMask = const DWORD if using k..., otherwise access_mask. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:76: int len = wsprintf(buf, L"%d", GetCurrentThreadId()); How about doing a resize() in Initialize() and then wsprintf directly into &cmd_line_[N], followed by resize()ing down to the final size? (Just to avoid the 32 bytes on the stack really, I guess, but it makes me feel a little less likely that something's going to allocate.) https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:97: error = GetLastError(); return GetLastError() seems simpler here, and move the DWORD error declaration down to the WFSO line. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:106: if (error != WAIT_OBJECT_0) { Please add (or add a note to add) UMA that tracks if this happens. (And maybe some other things near here that aren't likely to cause additional chaos.) https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.h (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 :) https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:14: #include <windows.h> This goes before " includes. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:25: // Only one instance of this class can exist at a time. "Only one instance" should be confirmed somewhere if important, at least with a DCHECK. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:55: // Stores the pre-cooked command line, with an allottment of zeros at the back allotment https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:33: CHECK(cmd_line->HasSwitch("process")); I've never used MULTIPROCESS_TEST_MAIN... can these be ASSERT/EXPECT instead? https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:36: unsigned uint_process = 0; unsigned int, and below for thread_id. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:73: MINIDUMP_TYPE minidump_type = static_cast<MINIDUMP_TYPE>( const makes sense here. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:127: ASSERT_EQ(ERROR_SUCCESS, launcher.LaunchAndWaitForHandler(&exc_ptrs)); It's a bit weird that in the test we don't test the normal behaviour of the launcher process being terminated. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.h (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:13: #include <windows.h> <> before "" https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:18: // (MinidumpWriteDump) to generate the crash report, then adds the report to MiniDumpWriteDump https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
Thanks - comments addressed, and CL winnowed down to the launcher only. PTAL? https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 18:29:04, scottmg wrote: > 2017 Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:23: DWORD kAccessMask = On 2017/01/06 18:29:04, scottmg wrote: > const DWORD if using k..., otherwise access_mask. Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:76: int len = wsprintf(buf, L"%d", GetCurrentThreadId()); On 2017/01/06 18:29:04, scottmg wrote: > How about doing a resize() in Initialize() and then wsprintf directly into > &cmd_line_[N], followed by resize()ing down to the final size? > > (Just to avoid the 32 bytes on the stack really, I guess, but it makes me feel a > little less likely that something's going to allocate.) Good idea, done! https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:97: error = GetLastError(); On 2017/01/06 18:29:04, scottmg wrote: > return GetLastError() seems simpler here, and move the DWORD error declaration > down to the WFSO line. Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:106: if (error != WAIT_OBJECT_0) { On 2017/01/06 18:29:04, scottmg wrote: > Please add (or add a note to add) UMA that tracks if this happens. (And maybe > some other things near here that aren't likely to cause additional chaos.) Added a TODO - maybe it'll be saner to do this from the caller also. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.h (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 18:29:04, scottmg wrote: > 2017 :) Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:14: #include <windows.h> On 2017/01/06 18:29:04, scottmg wrote: > This goes before " includes. Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:25: // Only one instance of this class can exist at a time. On 2017/01/06 18:29:04, scottmg wrote: > "Only one instance" should be confirmed somewhere if important, at least with a > DCHECK. Ah, that's redundant. Initially I had wanted to make this class register the UEF, but that made it really hard to test. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:55: // Stores the pre-cooked command line, with an allottment of zeros at the back On 2017/01/06 18:29:04, scottmg wrote: > allotment Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 18:29:04, scottmg wrote: > 2017 Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:33: CHECK(cmd_line->HasSwitch("process")); On 2017/01/06 18:29:04, scottmg wrote: > I've never used MULTIPROCESS_TEST_MAIN... can these be ASSERT/EXPECT instead? Sadly no, there's no GTest on the other side. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:36: unsigned uint_process = 0; On 2017/01/06 18:29:04, scottmg wrote: > unsigned int, and below for thread_id. Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:73: MINIDUMP_TYPE minidump_type = static_cast<MINIDUMP_TYPE>( On 2017/01/06 18:29:04, scottmg wrote: > const makes sense here. Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win_unittest.cc:127: ASSERT_EQ(ERROR_SUCCESS, launcher.LaunchAndWaitForHandler(&exc_ptrs)); On 2017/01/06 18:29:04, scottmg wrote: > It's a bit weird that in the test we don't test the normal behaviour of the > launcher process being terminated. Yeah, I suppose, though this is a function of the handler... I'm thinking this class is mechanism, and the user is the policy. Hence logging and terminating and all of that should be done by the caller of these. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 18:29:04, scottmg wrote: > 2017 Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win.h (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 18:29:04, scottmg wrote: > 2017 Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:13: #include <windows.h> On 2017/01/06 18:29:04, scottmg wrote: > <> before "" Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win.h:18: // (MinidumpWriteDump) to generate the crash report, then adds the report to On 2017/01/06 18:29:04, scottmg wrote: > MiniDumpWriteDump Done. https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_win_unittest.cc (right): https://codereview.chromium.org/2596463002/diff/100001/components/crash/conte... components/crash/content/app/fallback_crash_handler_win_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 18:29:04, scottmg wrote: > 2017 Done.
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher and a fallback crash hander. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. On the handler side, it parses the command line, and creates a dump with MinidumpWriteDump. This is then retouched to add the CrashpadInfo metadata, and to massage the dump into where Crashpad will parse it. BUG=678959 ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 ==========
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: 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 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/...)
lgtm https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.cc (right): https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:18: } nit; \n before } (or remove the \n after the opening {. https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:79: wsprintf(&cmd_line_.back() - kCommandLineTailSize + 1, L"%d", DCHECK that the return value <= kCommandLineTailSize. Could also resize() the string to the correct size to avoid relying on the added bytes being \0. https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.h (right): https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:30: // Initializes everything that's needed in LaunchAndWaitforHandler. ...Waitfor... -> ...WaitFor... https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:44: // - The error encountered in retrieving the crash handler process' exit code. Note here that the return path is used by test code.
Thanks - committing... https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.cc (right): https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:18: } On 2017/01/09 21:10:55, scottmg wrote: > nit; \n before } (or remove the \n after the opening {. Done. https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.cc:79: wsprintf(&cmd_line_.back() - kCommandLineTailSize + 1, L"%d", On 2017/01/09 21:10:55, scottmg wrote: > DCHECK that the return value <= kCommandLineTailSize. Could also resize() the > string to the correct size to avoid relying on the added bytes being \0. Done. This is a vector here, which is already zero-tailed at pad-time. wsprintf is guaranteed to spit another zero at the tail, so I don't think a resize is called for. The less work the better, and I'm not sure I can convince myself that resize won't re-allocate, though IANA STDC++ L Note that there's also an assumption that this instance will be used precisely once, but for better or worse, it'll support reuse this way :/ https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... File components/crash/content/app/fallback_crash_handler_launcher_win.h (right): https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:30: // Initializes everything that's needed in LaunchAndWaitforHandler. On 2017/01/09 21:10:56, scottmg wrote: > ...Waitfor... -> ...WaitFor... Done. https://codereview.chromium.org/2596463002/diff/200001/components/crash/conte... components/crash/content/app/fallback_crash_handler_launcher_win.h:44: // - The error encountered in retrieving the crash handler process' exit code. On 2017/01/09 21:10:56, scottmg wrote: > Note here that the return path is used by test code. Done.
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/2596463002/#ps220001 (title: "Address Scott's comments.")
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/2596463002/#ps240001 (title: "Fix signedness comparison goof.")
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": 1484056387722040,
"parent_rev": "ad25bdddb113a0986c246b4a046e6f68a327ac98", "commit_rev":
"d4e43bb5146c7a59123ff5f3b6e853d8e8c29130"}
Message was sent while issue was closed.
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8...
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 442596 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/01/10 18:24:13, findit-for-me wrote: > FYI: Findit identified this CL at revision 442596 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds... [6868:6228:0110/095312.249:26903062:FATAL:fallback_crash_handler_launcher_win_unittest.cc(96)] Check failed: false. Unable to write dump, error 2147942699 :/
Message was sent while issue was closed.
On 2017/01/10 18:37:15, scottmg wrote: > On 2017/01/10 18:24:13, findit-for-me wrote: > > FYI: Findit identified this CL at revision 442596 as the culprit for > > failures in the build cycles as shown on: > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds... > > [6868:6228:0110/095312.249:26903062:FATAL:fallback_crash_handler_launcher_win_unittest.cc(96)] > Check failed: false. Unable to write dump, error 2147942699 > > > :/ "Only part of a ReadProcessMemory or WriteProcessMemory request was completed." seems specific and legit, though I don't immediately see why.
Message was sent while issue was closed.
It's pretty interesting that MDWD is failing in this way :/. I'm guessing this is because the process is wiggling underneath the reads, which I suppose is another failure mode for MDWD(self, ...), sigh. Guess I'll spin another main() for a target to dump. On Tue, Jan 10, 2017 at 1:39 PM <scottmg@chromium.org> wrote: > On 2017/01/10 18:37:15, scottmg wrote: > > On 2017/01/10 18:24:13, findit-for-me wrote: > > > FYI: Findit identified this CL at revision 442596 as the culprit for > > > failures in the build cycles as shown on: > > > > > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > > > > > https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds... > > > > > > [6868:6228:0110/095312.249:26903062:FATAL:fallback_crash_handler_launcher_win_unittest.cc(96)] > > Check failed: false. Unable to write dump, error 2147942699 > <(214)%20794-2699> > > > > > > :/ > > "Only part of a ReadProcessMemory or WriteProcessMemory request was > completed." > seems specific and legit, though I don't immediately see why. > > https://codereview.chromium.org/2596463002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2621113002/ by siggi@chromium.org. The reason for reverting is: Failing on Win10 Tests x64 bot on main waterfall..
Message was sent while issue was closed.
Hey Scott, here's how I propose to fix it. This way the process getting dumped should be running nothing but the main thread sleeping on the dumper's process handle. PTAL. Siggi
Message was sent while issue was closed.
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... ==========
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...
lgtm
On 2017/01/10 19:56:39, Sigurður Ásgeirsson wrote: > Hey Scott, > > here's how I propose to fix it. This way the process getting dumped should be > running nothing but the main thread sleeping on the dumper's process handle. > PTAL. > > Siggi Things like this make me glad that we aren’t trusting MDWD for mainline crash reports anymore.
I suspect that MDWD isn't suspending the threads in the target. This is of course undocumented, but it sort of makes sense that it wouldn't. In which case we've been holding it wrong <https://memegenerator.net/instance/64529635> forever :/. On Tue, Jan 10, 2017 at 3:01 PM <mark@chromium.org> wrote: On 2017/01/10 19:56:39, Sigurður Ásgeirsson wrote: > Hey Scott, > > here's how I propose to fix it. This way the process getting dumped should be > running nothing but the main thread sleeping on the dumper's process handle. > PTAL. > > Siggi Things like this make me glad that we aren’t trusting MDWD for mainline crash reports anymore. https://codereview.chromium.org/2596463002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by siggi@chromium.org
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...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1484078817695840,
"parent_rev": "ffe695ceccba7953f426334c81915cc9eb5484ee", "commit_rev":
"f87fdaa12b09ff286d873e7a3ea459507d7adb83"}
Message was sent while issue was closed.
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Original-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442698} Committed: https://chromium.googlesource.com/chromium/src/+/f87fdaa12b09ff286d873e7a3ea4... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/f87fdaa12b09ff286d873e7a3ea4...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2620213002/ by dschuyler@chromium.org. The reason for reverting is: Sorry, it looks like the new patch also has a build failure. See crbug 679920. (I'm not 100% sure, this is my best guess, apologies if it turns out incorrect)..
Message was sent while issue was closed.
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Original-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442698} Committed: https://chromium.googlesource.com/chromium/src/+/f87fdaa12b09ff286d873e7a3ea4... ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Original-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442698} Committed: https://chromium.googlesource.com/chromium/src/+/f87fdaa12b09ff286d873e7a3ea4... ==========
Hey Scott, turns out the Win10 failures are because on the Win10 bot, the heap is over the 4G break. This patch should allow 64 bit pointers to get unmolested through the command line :/. 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...
On 2017/01/11 17:47:07, Sigurður Ásgeirsson wrote: > Hey Scott, > > turns out the Win10 failures are because on the Win10 bot, the heap is over the > 4G break. This patch should allow 64 bit pointers to get unmolested through the > command line :/. > > Siggi Oof, I should have noticed that. :( lgtm.
The CQ bit was unchecked by siggi@chromium.org
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...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484157200191290,
"parent_rev": "7fb9535883e50ab0d7262ea8a0e8470934e57a9c", "commit_rev":
"21b194219385cd209294216842e192978cc2b424"}
Message was sent while issue was closed.
Description was changed from ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Original-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442698} Committed: https://chromium.googlesource.com/chromium/src/+/f87fdaa12b09ff286d873e7a3ea4... ========== to ========== A simple, practically zero cost fallback crash handler for Crashpad handler process. This implements a fallback crash handler launcher. The launcher will be instantiated in the Crashpad handler process, where it pre-computes a command line to be launched on crash. This will be triggered off the UEF, under some form of single-instance locking. BUG=678959 Review-Url: https://codereview.chromium.org/2596463002 Cr-Original-Original-Commit-Position: refs/heads/master@{#442596} Committed: https://chromium.googlesource.com/chromium/src/+/d4e43bb5146c7a59123ff5f3b6e8... Review-Url: https://codereview.chromium.org/2596463002 Cr-Original-Commit-Position: refs/heads/master@{#442698} Committed: https://chromium.googlesource.com/chromium/src/+/f87fdaa12b09ff286d873e7a3ea4... Review-Url: https://codereview.chromium.org/2596463002 Cr-Commit-Position: refs/heads/master@{#442974} Committed: https://chromium.googlesource.com/chromium/src/+/21b194219385cd209294216842e1... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/21b194219385cd209294216842e1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
