Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377693002:
TerminateProcess if DumpAndCrash called without registration (Closed)
DescriptionTerminateProcess if DumpAndCrash called without registration
Temporary workaround for linked bug. I think it probably makes more
sense for the function to crash anyway, because the caller is not
expecting the call to return (esp. if used in a "security"-type crash.)
R=mark@chromium.org
BUG=634418
Committed: https://crrev.com/90e45dfec4254092f8301c0f1d28c1768e90533f
Cr-Commit-Position: refs/heads/master@{#421274}
Patch Set 1 #Patch Set 2 : self-terminate instead #
Total comments: 1
Messages
Total messages: 20 (11 generated)
I haven't reproduced or confirmed if there's any side-effects of this, but it seems reasonable to me logic-wise.
The CQ bit was checked by scottmg@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 2016/09/27 17:02:56, scottmg wrote: > I haven't reproduced or confirmed if there's any side-effects of this, but it > seems reasonable to me logic-wise. Hmm, although I suppose there's the potential we end up calling ourselves. Perhaps it ought to be that LOG(ERROR) is followed by self-termination, rather than trying to FATAL.
Description was changed from ========== LOG(FATAL) if DumpAndCrash called without registration Temporary workaround for linked bug. I think it probably makes more sense for the function to crash anyway, because the caller is not expecting the call to return (esp. if used in a "security"-type crash.) R=mark@chromium.org BUG=634418 ========== to ========== TerminateProcess if DumpAndCrash called without registration Temporary workaround for linked bug. I think it probably makes more sense for the function to crash anyway, because the caller is not expecting the call to return (esp. if used in a "security"-type crash.) R=mark@chromium.org BUG=634418 ==========
On 2016/09/27 17:05:55, scottmg wrote: > On 2016/09/27 17:02:56, scottmg wrote: > > I haven't reproduced or confirmed if there's any side-effects of this, but it > > seems reasonable to me logic-wise. > > Hmm, although I suppose there's the potential we end up calling ourselves. > Perhaps it ought to be that LOG(ERROR) is followed by self-termination, rather > than trying to FATAL. Updated to still LOG(ERROR), but then TerminateProcess() rather than returning. (There's already a 0xffff7001 and a 0xffff7002 for unusual termination codes, which is why I added 0xffff7003 here. I'll tidy these 3 up when I land upstream.)
The CQ bit was checked by scottmg@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...
martiniss@chromium.org changed reviewers: + martiniss@chromium.org
lgtm rs-lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2377693002/diff/20001/third_party/crashpad/cr... File third_party/crashpad/crashpad/client/crashpad_client_win.cc (right): https://codereview.chromium.org/2377693002/diff/20001/third_party/crashpad/cr... third_party/crashpad/crashpad/client/crashpad_client_win.cc:491: const UINT kCrashUnregistered = 0xffff7003; Seems like maybe we should have a common home for these constants, since there are now two sprinkled in different parts of this file, and one more in Crashpad proper.
scottmg wrote: > (There's already a 0xffff7001 and a 0xffff7002 for unusual termination codes, > which is why I added 0xffff7003 here. I'll tidy these 3 up when I land > upstream.) Oh, I just saw this. Right. Carry on.
The CQ bit was checked by scottmg@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 ========== TerminateProcess if DumpAndCrash called without registration Temporary workaround for linked bug. I think it probably makes more sense for the function to crash anyway, because the caller is not expecting the call to return (esp. if used in a "security"-type crash.) R=mark@chromium.org BUG=634418 ========== to ========== TerminateProcess if DumpAndCrash called without registration Temporary workaround for linked bug. I think it probably makes more sense for the function to crash anyway, because the caller is not expecting the call to return (esp. if used in a "security"-type crash.) R=mark@chromium.org BUG=634418 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== TerminateProcess if DumpAndCrash called without registration Temporary workaround for linked bug. I think it probably makes more sense for the function to crash anyway, because the caller is not expecting the call to return (esp. if used in a "security"-type crash.) R=mark@chromium.org BUG=634418 ========== to ========== TerminateProcess if DumpAndCrash called without registration Temporary workaround for linked bug. I think it probably makes more sense for the function to crash anyway, because the caller is not expecting the call to return (esp. if used in a "security"-type crash.) R=mark@chromium.org BUG=634418 Committed: https://crrev.com/90e45dfec4254092f8301c0f1d28c1768e90533f Cr-Commit-Position: refs/heads/master@{#421274} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/90e45dfec4254092f8301c0f1d28c1768e90533f Cr-Commit-Position: refs/heads/master@{#421274} |
