Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1095273003:
crashpad client for windows (Closed)
|
Created:
5 years, 8 months ago by cpu_(ooo_6.6-7.5) Modified:
5 years, 7 months ago CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Target Ref:
refs/heads/master Project:
crashpad Visibility:
Public. |
Descriptioncrashpad client for windows
Introduces CrashpadClient::SetHandler()
The code in the cc plays it fast and loose but helps ground
the intention.
BUG=crashpad:1
R=mark@chromium.org, scottmg@chromium.org
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/dd3c20667dafa7dcb9a1d512de0066a79887bab5
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 22
Patch Set 3 : review changes #
Total comments: 5
Patch Set 4 : nits #
Total comments: 1
Patch Set 5 : typo #
Messages
Total messages: 17 (5 generated)
cpu@chromium.org changed reviewers: + mark@chromium.org, scottmg@chromium.org
skeleton of client registration. ptal.
https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:81: //! The IPC port name (somehow) encodes enough information so that Maybe make the "enough information" explicit parameters here and then the body of SetHandler can turn them into an pipe name + args? ... but then it's almost the same as StartHandler's paramaters. I guess the semantics are slightly different since it's not going to ensure the handler is running? https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:85: //! \param[in] the full name of the crash handler IPC port. I think this should be \param[in] ipc_port the full name of the crash handler IPC port. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:87: //! \return 'true' on success `false` on failure. `true` instead of 'true' https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:88: bool SetHandler(const std::string& ipc_port); Probably just make this #if WIN and remove from the .mm file. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... client/crashpad_client_win.cc:1: // Copyright 2014 The Crashpad Authors. All rights reserved. 2015 https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... client/crashpad_client_win.cc:17: #include <Windows.h> lower 'W'
erikwright@chromium.org changed reviewers: + erikwright@chromium.org
I was actually starting to put together some registration code on Windows. Let's talk today about which aspects should go into Crashpad vs. into CAPS, as some aspects will definitely be Chrome specific.
https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:81: //! The IPC port name (somehow) encodes enough information so that On 2015/04/24 04:55:06, scottmg wrote: > Maybe make the "enough information" explicit parameters here and then the body > of SetHandler can turn them into an pipe name + args? > > ... but then it's almost the same as StartHandler's paramaters. I guess the > semantics are slightly different since it's not going to ensure the handler is > running? The "encodes" is misleading, I think the pipe name could be simple like it is today, (just encodes the user SID) or it can have other things like chrome channel. I am not sure if this encoding can be done at this layer. I guess if crashpad was the only one launching caps it could be just StartHandler() but if caps can be started by the shell (as startup items) then it seems weird here.
https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:75: //! \brief Sets the IPC port of a presumably running Crashpad handler process presumably-running https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:78: //! handler. However, just like StartHandler() crashes are not serviced comma after StartHandler() https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:85: //! \param[in] the full name of the crash handler IPC port. scottmg wrote: > I think this should be > > \param[in] ipc_port the full name of the crash handler IPC port. Yeah, and with a capital 't' on “The”. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:87: //! \return 'true' on success `false` on failure. On 2015/04/24 04:55:06, scottmg wrote: > `true` instead of 'true' and a comma or an “and” in between the true and false. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:88: bool SetHandler(const std::string& ipc_port); scottmg wrote: > Probably just make this #if WIN and remove from the .mm file. I agree with this for now, at least. There will be a SetHandler() for Mac, but it’ll accept a mach_port_t, not a const std::string&. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:93: //! or contacted by SetHandler(). SetHandler() may not need to contact the handler, so this can get away with being less specific and saying “configured” instead of “contacted”. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... client/crashpad_client_win.cc:29: SignalObjectAndWait(g_signal_exception, I would prefer if the return value were checked, so that causes of missing dumps attributable to errors in this file can be diagnosed. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... client/crashpad_client_win.cc:34: TerminateProcess(GetCurrentProcess(), 0); 0’s probably not the best exit code to use. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... client/crashpad_client_win.cc:55: // cases SetHandler() is used exclusively. This method can still be used to provide the reference implementation. As we discussed over e-mail or IM last week, the browser might want to StartHandler() if it finds that SetHandler() (or SetHandler()+UseHandler()) doesn’t work. And whatever’s responsible for starting the handler at login might wind up calling into this code too. I don’t meant that you should implement this right now, just that the comment doesn’t agree with our discussion, so I’m wondering if the comment predates it or if things changed after we talked.
all comments take care of. please take a look. Do note that SetHandler still takes the pipe name and I've agreed to provide some implementation for StartHandler for the purpose of testing or whatnot. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:85: //! \param[in] the full name of the crash handler IPC port. On 2015/04/24 04:55:06, scottmg wrote: > I think this should be > > \param[in] ipc_port the full name of the crash handler IPC port. Acknowledged. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:87: //! \return 'true' on success `false` on failure. On 2015/04/24 04:55:06, scottmg wrote: > `true` instead of 'true' Acknowledged. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:88: bool SetHandler(const std::string& ipc_port); On 2015/04/24 04:55:06, scottmg wrote: > Probably just make this #if WIN and remove from the .mm file. Acknowledged. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client.... client/crashpad_client.h:93: //! or contacted by SetHandler(). On 2015/04/28 17:53:05, Mark Mentovai wrote: > SetHandler() may not need to contact the handler, so this can get away with > being less specific and saying “configured” instead of “contacted”. Acknowledged. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... client/crashpad_client_win.cc:17: #include <Windows.h> On 2015/04/24 04:55:06, scottmg wrote: > lower 'W' Acknowledged. https://codereview.chromium.org/1095273003/diff/20001/client/crashpad_client_... client/crashpad_client_win.cc:55: // cases SetHandler() is used exclusively. On 2015/04/28 17:53:05, Mark Mentovai wrote: > This method can still be used to provide the reference implementation. As we > discussed over e-mail or IM last week, the browser might want to StartHandler() > if it finds that SetHandler() (or SetHandler()+UseHandler()) doesn’t work. And > whatever’s responsible for starting the handler at login might wind up calling > into this code too. > > I don’t meant that you should implement this right now, just that the comment > doesn’t agree with our discussion, so I’m wondering if the comment predates it > or if things changed after we talked. Probably my misunderstanding. I'll update the comment.
missed the copyright year. I'll take care of that.
missed the copyright year. I'll take care of that.
LGTM with these changes and the copyright year fixed. https://codereview.chromium.org/1095273003/diff/40001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1095273003/diff/40001/client/crashpad_client.... client/crashpad_client.h:88: #if defined(OS_WIN) Move this above the //! brief that applies to it. You can also make it #if defined(OS_WIN) || DOXYGEN to make sure it shows up in the generated docs. https://codereview.chromium.org/1095273003/diff/40001/client/crashpad_client.... client/crashpad_client.h:90: #endif and put a blank line after the #endif. https://codereview.chromium.org/1095273003/diff/40001/client/crashpad_client.... client/crashpad_client.h:109: //! function that when reached it will "signal and wait" for the crash Extraneous “it”. https://codereview.chromium.org/1095273003/diff/40001/client/crashpad_client_... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1095273003/diff/40001/client/crashpad_client_... client/crashpad_client_win.cc:44: LOG(INFO) << "SignalObjectAndWait timed out"; These two should probably be more severe than INFO, maybe WARNING? INFO is pretty rare in Crashpad. https://codereview.chromium.org/1095273003/diff/40001/client/crashpad_client_... client/crashpad_client_win.cc:46: LOG(INFO) << "SignalObjectAndWait returned " << rv You can use PLOG which will stream and explain GetLastError() for you. We use that pretty extensively in Crashpad.
lgtm https://codereview.chromium.org/1095273003/diff/60001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1095273003/diff/60001/client/crashpad_client.... client/crashpad_client.h:83: //! registration is done with a crash handler using the appropiate database "appropriate"
The CQ bit was checked by cpu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1095273003/#ps80001 (title: "typo")
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as dd3c20667dafa7dcb9a1d512de0066a79887bab5 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
