Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1126783004:
Introduce RegistrationServer. (Closed)
|
Created:
5 years, 7 months ago by erikwright (departed) Modified:
5 years, 7 months ago CC:
crashpad-dev_chromium.org, cpu_(ooo_6.6-7.5), Sigurður Ásgeirsson, robertshield Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Target Ref:
refs/heads/master Project:
crashpad Visibility:
Public. |
DescriptionIntroduce RegistrationServer, which implements a Crashpad client registration protocol for Windows.
BUG=
R=cpu@chromium.org, scottmg@chromium.org
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/9ff3d9335fd0a8e3b9e84e88d565945acdb2fa94
Patch Set 1 #
Total comments: 51
Patch Set 2 : Add a test, move Thread. #Patch Set 3 : Extract thread move. #Patch Set 4 : Further refinements. #Patch Set 5 : More tweaks. #Patch Set 6 : Working, with tests. #Patch Set 7 : Tweaks. #Patch Set 8 : Implement pipe client PID detection. #
Total comments: 70
Patch Set 9 : Review comments. #Patch Set 10 : Review comments. #Patch Set 11 : Review comments. #
Total comments: 17
Patch Set 12 : Review feedback. #
Total comments: 6
Patch Set 13 : Review feedback. #Patch Set 14 : Fix .gyp for POSIX. #
Total comments: 12
Patch Set 15 : Tidying up. #Patch Set 16 : Refactoring, additional coverage, bug fix. #Patch Set 17 : Add comments, tidy. #
Total comments: 13
Patch Set 18 : Review feedback. #
Total comments: 1
Patch Set 19 : One extra comment. #
Messages
Total messages: 28 (3 generated)
erikwright@chromium.org changed reviewers: + mark@chromium.org
Mark: PTAL. I will add tests, but first I want to get some feedback on the general structure of the code, where it's located, etc., to feel out the Crashpad coding conventions.
mark@chromium.org changed reviewers: + scottmg@chromium.org
+scott too. He’s very familiar with Crashpad code and style. https://codereview.chromium.org/1126783004/diff/1/client/registration_request... File client/registration_request_win.h (right): https://codereview.chromium.org/1126783004/diff/1/client/registration_request... client/registration_request_win.h:1: // Copyright 2015 The Crashpad Authors. All rights reserved. This would be easier to review with --no-find-copies. https://codereview.chromium.org/1126783004/diff/1/client/registration_request... client/registration_request_win.h:1: // Copyright 2015 The Crashpad Authors. All rights reserved. Headers should be listed in the .gyp file too. https://codereview.chromium.org/1126783004/diff/1/client/registration_request... client/registration_request_win.h:22: struct RegistrationRequest { Ideally, everything in namespace crashpad has Doxygen comments. https://codereview.chromium.org/1126783004/diff/1/client/registration_request... client/registration_request_win.h:24: size_t crashpad_info_address; This may pose a problem for future cross-bitted operation. (And if you did use size_t, you’d need to #include <sys/types.h>.) https://codereview.chromium.org/1126783004/diff/1/client/registration_respons... File client/registration_response_win.h (right): https://codereview.chromium.org/1126783004/diff/1/client/registration_respons... client/registration_response_win.h:22: struct RegistrationResponse { Is there a compelling reason to separate the request and response into their own headers? It seems that they’ll always be used in concert. https://codereview.chromium.org/1126783004/diff/1/handler/handler.gyp File handler/handler.gyp (right): https://codereview.chromium.org/1126783004/diff/1/handler/handler.gyp#newcode78 handler/handler.gyp:78: '../third_party/mini_chromium/mini_chromium.gyp:base', This depends on at least client and util too. https://codereview.chromium.org/1126783004/diff/1/handler/handler_test.gyp File handler/handler_test.gyp (right): https://codereview.chromium.org/1126783004/diff/1/handler/handler_test.gyp#ne... handler/handler_test.gyp:21: 'target_name': 'crashpad_handler_test', This whole target should be win-only. An executable without sources will be a build-time error on other platforms. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:18: #include "client/registration_request_win.h" Blank line before this. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:24: const int kNumThreads = 3; This is only used in the constructor, it doesn’t need to be out here. You can have a local const in the constructor. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:29: : delegate_(delegate.Pass()) { Explicitly initialize everything even if it’s the same as default initialization. listener_threads_() is missing. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:35: ::CreateNamedPipe(pipe_name.c_str(), open_mode, No :: in crashpad except where it’s needed to disambiguate. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:37: kNumThreads, 512, 512, 20, NULL)); In Crashpad, use nullptr, not NULL. Generally we don’t bin-pack like this. We have a .clang-format that handles things fairly well. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:37: kNumThreads, 512, 512, 20, NULL)); What is the significance of 20? https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:52: delete thread; Use a PointerVector<>. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:57: ScopedKernelHANDLE pipe, This would fit on the preceding line. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:67: break; PLOG(ERROR) https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:81: } …otherwise? Is this something that should be logged? Should this break or continue or return? The same applies to all of the other unexpected cases in this method. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:93: ::WriteFile( Check everything. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:107: Get rid of the blank line here. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... File handler/win/registration_server.h (right): https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:23: #include "test/thread.h" Can’t depend on test code from non-test code. Thread will need to be hoisted out of test. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:29: //! protocol. Since this is Windows-specific, the comment should say so. The generated documentation won’t make it very obvious that this came from handler/win. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:43: //! \param[out] request_dump_event A HANDLE, valid in the client process, to Surround system types in backticks so that they’re set off in the documentation: `HANDLE`. This isn’t necessary for our own types because they’re automatically linked. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:47: //! successfully captured.. Double period. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:49: size_t crashpad_info_address, #include <sys/types.h> https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:50: HANDLE* request_dump_event, #include <windows.h> https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:70: // test::Thread implementation. // test::Thread:
https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... File handler/win/registration_server.h (right): https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:32: //! \brief Handles registration requests. Please add something high level here about to whom this is intended to delegate, and why it shouldn't just have a concrete implementation in Crashpad. (I generally find Chromium's maze of delegate/host/impls unpleasant.) https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:49: size_t crashpad_info_address, WinVMAddress in util/win/address_types.h would make sense instead of size_t here.
Scott: PTAL. https://codereview.chromium.org/1126783004/diff/1/client/registration_respons... File client/registration_response_win.h (right): https://codereview.chromium.org/1126783004/diff/1/client/registration_respons... client/registration_response_win.h:22: struct RegistrationResponse { On 2015/05/05 20:20:33, Mark Mentovai wrote: > Is there a compelling reason to separate the request and response into their own > headers? It seems that they’ll always be used in concert. No problem. Do you have an opinion on the name of the file that contains both? client_registration_messages.h? https://codereview.chromium.org/1126783004/diff/1/handler/handler.gyp File handler/handler.gyp (right): https://codereview.chromium.org/1126783004/diff/1/handler/handler.gyp#newcode78 handler/handler.gyp:78: '../third_party/mini_chromium/mini_chromium.gyp:base', On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > This depends on at least client and util too. Done. https://codereview.chromium.org/1126783004/diff/1/handler/handler_test.gyp File handler/handler_test.gyp (right): https://codereview.chromium.org/1126783004/diff/1/handler/handler_test.gyp#ne... handler/handler_test.gyp:21: 'target_name': 'crashpad_handler_test', On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > This whole target should be win-only. An executable without sources will be a > build-time error on other platforms. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:18: #include "client/registration_request_win.h" On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > Blank line before this. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:24: const int kNumThreads = 3; On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > This is only used in the constructor, it doesn’t need to be out here. You can > have a local const in the constructor. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:29: : delegate_(delegate.Pass()) { On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > Explicitly initialize everything even if it’s the same as default > initialization. listener_threads_() is missing. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:35: ::CreateNamedPipe(pipe_name.c_str(), open_mode, On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > No :: in crashpad except where it’s needed to disambiguate. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:37: kNumThreads, 512, 512, 20, NULL)); On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > What is the significance of 20? Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:37: kNumThreads, 512, 512, 20, NULL)); On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > In Crashpad, use nullptr, not NULL. Generally we don’t bin-pack like this. We > have a .clang-format that handles things fairly well. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:52: delete thread; On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > Use a PointerVector<>. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:57: ScopedKernelHANDLE pipe, On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > This would fit on the preceding line. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:67: break; On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > PLOG(ERROR) Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:81: } On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > …otherwise? Is this something that should be logged? Should this break or > continue or return? The same applies to all of the other unexpected cases in > this method. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:93: ::WriteFile( On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > Check everything. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.cc:107: On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > Get rid of the blank line here. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... File handler/win/registration_server.h (right): https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:23: #include "test/thread.h" On 2015/05/05 20:20:34, Mark Mentovai - out til August wrote: > Can’t depend on test code from non-test code. Thread will need to be hoisted out > of test. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:29: //! protocol. On 2015/05/05 20:20:33, Mark Mentovai - out til August wrote: > Since this is Windows-specific, the comment should say so. The generated > documentation won’t make it very obvious that this came from handler/win. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:32: //! \brief Handles registration requests. On 2015/05/05 21:12:27, scottmg wrote: > Please add something high level here about to whom this is intended to delegate, > and why it shouldn't just have a concrete implementation in Crashpad. (I > generally find Chromium's maze of delegate/host/impls unpleasant.) This allows me to test the registrar without going through pipes and to verify the pipe-based protocol without having to also test the other aspects of the registrar. For example, I can very trivially verify that an IPC invocation is turned into a valid process handle with the right permissions. But if this class also (internally or via direct invocation of a concrete class) acted as the registrar I would not be able to intercept that process handle. To be honest, I'm not sure what the comment would say.. "Decoupled for testability"? https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:43: //! \param[out] request_dump_event A HANDLE, valid in the client process, to On 2015/05/05 20:20:34, Mark Mentovai - out til August wrote: > Surround system types in backticks so that they’re set off in the documentation: > `HANDLE`. This isn’t necessary for our own types because they’re automatically > linked. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:47: //! successfully captured.. On 2015/05/05 20:20:34, Mark Mentovai - out til August wrote: > Double period. Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:49: size_t crashpad_info_address, On 2015/05/05 20:20:34, Mark Mentovai - out til August wrote: > #include <sys/types.h> Acknowledged. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:50: HANDLE* request_dump_event, On 2015/05/05 20:20:34, Mark Mentovai - out til August wrote: > #include <windows.h> Done. https://codereview.chromium.org/1126783004/diff/1/handler/win/registration_se... handler/win/registration_server.h:70: // test::Thread implementation. On 2015/05/05 20:20:34, Mark Mentovai - out til August wrote: > // test::Thread: Done.
scottmg@chromium.org changed reviewers: + cpu@chromium.org
Good stuff! +cpu to look at details of pipe stuff in second half of handler/win/registration_server.cc (I'm not sure about ImpersonateNamedPipeClient, etc.) https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:19: #include "util/win/address_types.h" Blank line before. https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:29: crashpad::WinVMAddress crashpad_info_address; We should either pragma pack 1 this header, or move the client_process_id below the crashpad_info_address, otherwise it's sending undefined padding bytes across the pipe. https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:36: HANDLE request_report_event; Will these HANDLEs work correctly when the crash reporter is a 64 bit process and the target process is x86? https://codereview.chromium.org/1126783004/diff/140001/handler/handler.gyp File handler/handler.gyp (right): https://codereview.chromium.org/1126783004/diff/140001/handler/handler.gyp#ne... handler/handler.gyp:79: '../util/util.gyp:crashpad_util', sort https://codereview.chromium.org/1126783004/diff/140001/handler/handler_test.gyp File handler/handler_test.gyp (right): https://codereview.chromium.org/1126783004/diff/140001/handler/handler_test.g... handler/handler_test.gyp:1: # Copyright 2014 The Crashpad Authors. All rights reserved. 2015 https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:17: #include <cstring> string.h https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:36: HMODULE kernel_dll = LoadLibrary(TEXT("Kernel32.dll")); We only build with _UNICODE, just L" instead, and lower case 'k' on kernel32 for consistency with other code. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:38: typedef BOOL(WINAPI * GetNamedPipeClientProcessIdProc)(HANDLE, PULONG); We've been using decltype instead (since the prototype is available). See e.g. NtQueryInformationProcess in util/win/process_info.cc https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:46: result = (proc)(pipe, process_id); Remove extra () around `proc`. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:86: // Invokes ReadFile. These function comments don't seem very useful. (Just removing them is fine if there's nothing to say.) https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:119: event_.reset(CreateEvent(nullptr, TRUE, FALSE, nullptr)); true and false (even though they're BOOL, not bool) and elsewhere. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:121: PLOG(ERROR); << "failed to create event" or just << "CreateEvent", and elsewhere https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:139: SetEvent(event_.get()); I think this path would execute if the event failed to be created in the constructor. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:207: if (ReadFile(pipe_.get(), &request_, sizeof(request_), &bytes_read, &overlapped_)) { 80 col https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:219: if (WriteFile(pipe_.get(), &response_, sizeof(response_), &bytes_written, &overlapped_)) { 80 col https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:231: if (TryGetNamedPipeClientProcessId(pipe_.get(), &real_client_process_id)) { What's the upside of doing this when it doesn't work on XP? (in a comment if it makes sense) https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:265: ResetConnection(); LOG(ERROR) here https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:284: RegistrationServer::RegistrationServer() : stop_event_()/*, stopped_event_()*/ { Delete old (?) comment. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:316: delegate->OnStarted(); Moving this down below the loop seems tidier than having an extra condition in the loop. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:320: PLOG(ERROR); If we get to this case, then the next time around the loop isn't going to have FILE_FLAG_FIRST_PIPE_INSTANCE set so isn't going to work properly. So I think it might as well exit after logging an error. Alternatively if it's worth retrying, then i == 0 above needs to be pipes.size() == 0. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:359: static_cast<DWORD>(handles.size()), handles.data(), TRUE, INFINITE); If we exited the loop above with WAIT_FAILED, is this going to succeed? Is the PCHECK going to crash? https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server_test.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:19: #include <windows.h> C headers before C++ headers. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:20: #include "base/basictypes.h" Blank line before. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:21: #include "base/memory/scoped_ptr.h" unused? https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:23: #include "base/strings/stringprintf.h" unused? https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:28: #include "util/stdlib/pointer_container.h" unused? https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:29: #include "util/thread/thread.h" Looks like it could have stayed in test/ after all? Probably will be useful in the future anyway. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:143: // Thread : no space before : https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:171: for (int retries = 0; !pipe.is_valid() && retries < kMaxRetries; ++retries) { 80 col https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:172: if (!::WaitNamedPipe(pipe_name_.c_str(), NMPWAIT_WAIT_FOREVER)) No leading :: unless there's a name conflict (and below). https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:408: VerifyRegistration(*delegate().registered_processes()[0], request, response_2); 80 col
scott, cpu: PTAL. https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:19: #include "util/win/address_types.h" On 2015/05/20 18:53:09, scottmg wrote: > Blank line before. Done. https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:29: crashpad::WinVMAddress crashpad_info_address; On 2015/05/20 18:53:09, scottmg wrote: > We should either pragma pack 1 this header, or move the client_process_id below > the crashpad_info_address, otherwise it's sending undefined padding bytes across > the pipe. Done. I did it separately for each structure to avoid the risk of code being moved around or unrelated code being inserted between them. Even though the second one doesn't require it, I think it's best to have it there now (in case members are added later). https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:36: HANDLE request_report_event; On 2015/05/20 18:53:09, scottmg wrote: > Will these HANDLEs work correctly when the crash reporter is a 64 bit process > and the target process is x86? From MSDN (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251%28v=vs.85%2...): DuplicateHandle can be used to duplicate a handle between a 32-bit process and a 64-bit process. The resulting handle is appropriately sized to work in the target process. https://codereview.chromium.org/1126783004/diff/140001/handler/handler.gyp File handler/handler.gyp (right): https://codereview.chromium.org/1126783004/diff/140001/handler/handler.gyp#ne... handler/handler.gyp:79: '../util/util.gyp:crashpad_util', On 2015/05/20 18:53:09, scottmg wrote: > sort Done. https://codereview.chromium.org/1126783004/diff/140001/handler/handler_test.gyp File handler/handler_test.gyp (right): https://codereview.chromium.org/1126783004/diff/140001/handler/handler_test.g... handler/handler_test.gyp:1: # Copyright 2014 The Crashpad Authors. All rights reserved. On 2015/05/20 18:53:09, scottmg wrote: > 2015 Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:17: #include <cstring> On 2015/05/20 18:53:09, scottmg wrote: > string.h Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:36: HMODULE kernel_dll = LoadLibrary(TEXT("Kernel32.dll")); On 2015/05/20 18:53:09, scottmg wrote: > We only build with _UNICODE, just L" instead, and lower case 'k' on kernel32 for > consistency with other code. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:38: typedef BOOL(WINAPI * GetNamedPipeClientProcessIdProc)(HANDLE, PULONG); On 2015/05/20 18:53:09, scottmg wrote: > We've been using decltype instead (since the prototype is available). See e.g. > NtQueryInformationProcess in util/win/process_info.cc Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:46: result = (proc)(pipe, process_id); On 2015/05/20 18:53:09, scottmg wrote: > Remove extra () around `proc`. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:86: // Invokes ReadFile. On 2015/05/20 18:53:09, scottmg wrote: > These function comments don't seem very useful. (Just removing them is fine if > there's nothing to say.) Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:119: event_.reset(CreateEvent(nullptr, TRUE, FALSE, nullptr)); On 2015/05/20 18:53:09, scottmg wrote: > true and false (even though they're BOOL, not bool) and elsewhere. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:121: PLOG(ERROR); On 2015/05/20 18:53:09, scottmg wrote: > << "failed to create event" or just << "CreateEvent", and elsewhere I received some guidance in Chromium to not include verbose non-Debug log messages. Reasoning: Whatever string I put here will be baked into the code. On the other hand, with no string a line number and filename will be output, and we have the source. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:139: SetEvent(event_.get()); On 2015/05/20 18:53:09, scottmg wrote: > I think this path would execute if the event failed to be created in the > constructor. I fixed things so the client now knows when it enters an invalid state. So Stop should never be called in that condition https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:207: if (ReadFile(pipe_.get(), &request_, sizeof(request_), &bytes_read, &overlapped_)) { On 2015/05/20 18:53:09, scottmg wrote: > 80 col Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:219: if (WriteFile(pipe_.get(), &response_, sizeof(response_), &bytes_written, &overlapped_)) { On 2015/05/20 18:53:09, scottmg wrote: > 80 col Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:231: if (TryGetNamedPipeClientProcessId(pipe_.get(), &real_client_process_id)) { On 2015/05/20 18:53:09, scottmg wrote: > What's the upside of doing this when it doesn't work on XP? (in a comment if it > makes sense) Added a comment. Carlos implied there was a minor hypothetical attack vector here. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:265: ResetConnection(); On 2015/05/20 18:53:09, scottmg wrote: > LOG(ERROR) here When calling into other crashpad code, I assume that that code will log whatever error might occur. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:284: RegistrationServer::RegistrationServer() : stop_event_()/*, stopped_event_()*/ { On 2015/05/20 18:53:09, scottmg wrote: > Delete old (?) comment. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:316: delegate->OnStarted(); On 2015/05/20 18:53:09, scottmg wrote: > Moving this down below the loop seems tidier than having an extra condition in > the loop. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:320: PLOG(ERROR); On 2015/05/20 18:53:09, scottmg wrote: > If we get to this case, then the next time around the loop isn't going to have > FILE_FLAG_FIRST_PIPE_INSTANCE set so isn't going to work properly. So I think it > might as well exit after logging an error. Alternatively if it's worth retrying, > then i == 0 above needs to be pipes.size() == 0. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:359: static_cast<DWORD>(handles.size()), handles.data(), TRUE, INFINITE); On 2015/05/20 18:53:09, scottmg wrote: > If we exited the loop above with WAIT_FAILED, is this going to succeed? Is the > PCHECK going to crash? It's hard to rationalize about what would happen if WAIT_FAILED occurs in the first WaitForMultipleObjects. Quite possibly the root cause of that failure might cause the second WFMO to fail too. The important thing is that we don't destroy the PipeState instances if we don't know that the CancelIo completed. The PCHECK on line 360 ensures this. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server_test.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:19: #include <windows.h> On 2015/05/20 18:53:10, scottmg wrote: > C headers before C++ headers. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:20: #include "base/basictypes.h" On 2015/05/20 18:53:10, scottmg wrote: > Blank line before. Acknowledged. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:21: #include "base/memory/scoped_ptr.h" On 2015/05/20 18:53:10, scottmg wrote: > unused? Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:23: #include "base/strings/stringprintf.h" On 2015/05/20 18:53:10, scottmg wrote: > unused? line 159 https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:28: #include "util/stdlib/pointer_container.h" On 2015/05/20 18:53:10, scottmg wrote: > unused? used for PointerVector https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:29: #include "util/thread/thread.h" On 2015/05/20 18:53:10, scottmg wrote: > Looks like it could have stayed in test/ after all? Probably will be useful in > the future anyway. Yeah, my initial implementation used this in RegistrationServer, but during testing I learned that this implementation was not viable due to the lack of any means to stop synchronous IO calls. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:143: // Thread : On 2015/05/20 18:53:10, scottmg wrote: > no space before : Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:171: for (int retries = 0; !pipe.is_valid() && retries < kMaxRetries; ++retries) { On 2015/05/20 18:53:10, scottmg wrote: > 80 col Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:172: if (!::WaitNamedPipe(pipe_name_.c_str(), NMPWAIT_WAIT_FOREVER)) On 2015/05/20 18:53:10, scottmg wrote: > No leading :: unless there's a name conflict (and below). Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:408: VerifyRegistration(*delegate().registered_processes()[0], request, response_2); On 2015/05/20 18:53:10, scottmg wrote: > 80 col Done.
https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:36: HANDLE request_report_event; On 2015/05/20 20:54:13, erikwright wrote: > On 2015/05/20 18:53:09, scottmg wrote: > > Will these HANDLEs work correctly when the crash reporter is a 64 bit process > > and the target process is x86? > > From MSDN > (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251%28v=vs.85%2...): > > DuplicateHandle can be used to duplicate a handle between a 32-bit process and a > 64-bit process. The resulting handle is appropriately sized to work in the > target process. HANDLEs have only 32 significant bits for both x64 and x86, but in sending a RegistrationResponse across the pipe, won't there be a mismatch in message size because HANDLE is just a void* of differing size on either side? https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:86: // Invokes ReadFile. On 2015/05/20 20:54:14, erikwright wrote: > On 2015/05/20 18:53:09, scottmg wrote: > > These function comments don't seem very useful. (Just removing them is fine if > > there's nothing to say.) > > Done. Sorry, I meant just IssueRead and IssueWrite. I think the HandleRequest and ResetConnection ones were valuable. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:121: PLOG(ERROR); On 2015/05/20 20:54:14, erikwright wrote: > On 2015/05/20 18:53:09, scottmg wrote: > > << "failed to create event" or just << "CreateEvent", and elsewhere > > I received some guidance in Chromium to not include verbose non-Debug log > messages. Reasoning: > > Whatever string I put here will be baked into the code. On the other hand, with > no string a line number and filename will be output, and we have the source. Yeah, it's a divergence from Chromium. :( Mark has generally requested that these logs be somewhat more informative with a view to capturing and including them in dumps. Tracking them back to the line number is plausible, but could be somewhat tedious as there's likely to be code shifts amongst versions and it could get quite complicated to correlate in that case. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server_test.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:28: #include "util/stdlib/pointer_container.h" On 2015/05/20 20:54:15, erikwright wrote: > On 2015/05/20 18:53:10, scottmg wrote: > > unused? > > used for PointerVector I don't see a PointerVector in this file? https://codereview.chromium.org/1126783004/diff/200001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/200001/client/registration_pr... client/registration_protocol_win.h:25: #pragma pack(push,1) Space after comma, and below. https://codereview.chromium.org/1126783004/diff/200001/client/registration_pr... client/registration_protocol_win.h:25: #pragma pack(push,1) I think better to move these above the \brief line, I'm not sure whether Doxygen will get confused. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.cc:67: bool Start(); Consider if you prefer Initialize() for this function instead of Start() -- Initialize is commonly used in Crashpad, but I understand the desire for symmetry with Stop() too. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.cc:162: false); // do not wait "Do not wait." https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.cc:294: // pointers to them to RegisterClient). Good thought. I'm not sure it's necessary in a win/ file though. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... File handler/win/registration_server.h (right): https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.h:21: #include "base/memory/scoped_ptr.h" unused https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... File handler/win/registration_server_test.cc (right): https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server_test.cc:179: GENERIC_READ | GENERIC_WRITE, Wrong indent (fyi, there's a .clang-format in the project root) https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server_test.cc:199: const_cast<void*>(request_buffer), wrong indent
Scott: PTAL. https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/140001/client/registration_pr... client/registration_protocol_win.h:36: HANDLE request_report_event; On 2015/05/21 02:32:36, scottmg wrote: > On 2015/05/20 20:54:13, erikwright wrote: > > On 2015/05/20 18:53:09, scottmg wrote: > > > Will these HANDLEs work correctly when the crash reporter is a 64 bit > process > > > and the target process is x86? > > > > From MSDN > > > (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251%28v=vs.85%2...): > > > > DuplicateHandle can be used to duplicate a handle between a 32-bit process and > a > > 64-bit process. The resulting handle is appropriately sized to work in the > > target process. > > HANDLEs have only 32 significant bits for both x64 and x86, but in sending a > RegistrationResponse across the pipe, won't there be a mismatch in message size > because HANDLE is just a void* of differing size on either side? Yes you are correct. I thought about declaring something in address_types.h but it seemed different in usage from WinVMAddress. Let me know if you feel differently. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:86: // Invokes ReadFile. On 2015/05/21 02:32:36, scottmg wrote: > On 2015/05/20 20:54:14, erikwright wrote: > > On 2015/05/20 18:53:09, scottmg wrote: > > > These function comments don't seem very useful. (Just removing them is fine > if > > > there's nothing to say.) > > > > Done. > > Sorry, I meant just IssueRead and IssueWrite. I think the HandleRequest and > ResetConnection ones were valuable. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server.cc:121: PLOG(ERROR); On 2015/05/21 02:32:36, scottmg wrote: > On 2015/05/20 20:54:14, erikwright wrote: > > On 2015/05/20 18:53:09, scottmg wrote: > > > << "failed to create event" or just << "CreateEvent", and elsewhere > > > > I received some guidance in Chromium to not include verbose non-Debug log > > messages. Reasoning: > > > > Whatever string I put here will be baked into the code. On the other hand, > with > > no string a line number and filename will be output, and we have the source. > > Yeah, it's a divergence from Chromium. :( > > Mark has generally requested that these logs be somewhat more informative with a > view to capturing and including them in dumps. Tracking them back to the line > number is plausible, but could be somewhat tedious as there's likely to be code > shifts amongst versions and it could get quite complicated to correlate in that > case. Done. https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... File handler/win/registration_server_test.cc (right): https://codereview.chromium.org/1126783004/diff/140001/handler/win/registrati... handler/win/registration_server_test.cc:28: #include "util/stdlib/pointer_container.h" On 2015/05/21 02:32:36, scottmg wrote: > On 2015/05/20 20:54:15, erikwright wrote: > > On 2015/05/20 18:53:10, scottmg wrote: > > > unused? > > > > used for PointerVector > > I don't see a PointerVector in this file? MockDelegate::registered_processes_ https://codereview.chromium.org/1126783004/diff/200001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/200001/client/registration_pr... client/registration_protocol_win.h:25: #pragma pack(push,1) On 2015/05/21 02:32:36, scottmg wrote: > Space after comma, and below. Done. https://codereview.chromium.org/1126783004/diff/200001/client/registration_pr... client/registration_protocol_win.h:25: #pragma pack(push,1) On 2015/05/21 02:32:36, scottmg wrote: > I think better to move these above the \brief line, I'm not sure whether Doxygen > will get confused. Done. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.cc:67: bool Start(); On 2015/05/21 02:32:36, scottmg wrote: > Consider if you prefer Initialize() for this function instead of Start() -- > Initialize is commonly used in Crashpad, but I understand the desire for > symmetry with Stop() too. Done. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.cc:162: false); // do not wait On 2015/05/21 02:32:36, scottmg wrote: > "Do not wait." Done. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.cc:294: // pointers to them to RegisterClient). On 2015/05/21 02:32:36, scottmg wrote: > Good thought. I'm not sure it's necessary in a win/ file though. I get compilation warnings if I pass the unaligned values, which would require the use of __unaligned (https://msdn.microsoft.com/en-us/library/ms177389.aspx) Potentially it makes sense to disable the warnings? https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... File handler/win/registration_server.h (right): https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.h:21: #include "base/memory/scoped_ptr.h" On 2015/05/21 02:32:36, scottmg wrote: > unused Done. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... File handler/win/registration_server_test.cc (right): https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server_test.cc:179: GENERIC_READ | GENERIC_WRITE, On 2015/05/21 02:32:36, scottmg wrote: > Wrong indent (fyi, there's a .clang-format in the project root) Sorry, oversight from removing leading '::'. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server_test.cc:199: const_cast<void*>(request_buffer), On 2015/05/21 02:32:37, scottmg wrote: > wrong indent Done.
LGTM Please let cpu have a quick look at the pipe registration stuff before landing. https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/200001/handler/win/registrati... handler/win/registration_server.cc:294: // pointers to them to RegisterClient). On 2015/05/21 15:12:37, erikwright wrote: > On 2015/05/21 02:32:36, scottmg wrote: > > Good thought. I'm not sure it's necessary in a win/ file though. > > I get compilation warnings if I pass the unaligned values, which would require > the use of __unaligned (https://msdn.microsoft.com/en-us/library/ms177389.aspx) > > Potentially it makes sense to disable the warnings? Interesting, I didn't know that! What you have seems good then. https://codereview.chromium.org/1126783004/diff/220001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/220001/client/registration_pr... client/registration_protocol_win.h:39: //! \brief An event handle, valid in the client process, that should be Change handle to `HANDLE` since it's now less clear what's supposed to be in these fields, and note why they can't be HANDLE. https://codereview.chromium.org/1126783004/diff/220001/client/registration_pr... client/registration_protocol_win.h:41: uint32_t request_report_event; I think either WinVMAddress (because it's a void* in the target) or uint32_t (because we know that only 32 bits are important) is reasonable for this. If it comes up again we can add a WinCrossProcessHANDLE (or some better name). https://codereview.chromium.org/1126783004/diff/220001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/220001/handler/win/registrati... handler/win/registration_server.cc:313: response_.request_report_event = reinterpret_cast<uint32_t>(request_report_event); 80 col https://codereview.chromium.org/1126783004/diff/220001/handler/win/registrati... handler/win/registration_server.cc:313: response_.request_report_event = reinterpret_cast<uint32_t>(request_report_event); Add a short comment here about the fact that it's safe to truncate these HANDLE values.
Carlos: PTAL.
I was hoping to avoid the state machine by being synchronous. From what I follow it seems correct but the async mode introduces complexity that I don't see much gain from. Are you sure you want async? (I'll keep looking at the code regardless) https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:37: HMODULE kernel_dll = LoadLibrary(L"kernel32.dll"); loadlibrary is far slower than GetModuleHandle, this is sadness that appeared when windows invented SxS. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:41: GetProcAddress(kernel_dll, "GetNamedPipeClientProcessId")); the lines 39-41 are classy. I've been meaning to do this all over chromium (with a wrapper). You are welcome if you what to tackle that. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:87: as a side note c+11 alias makes 86 look nicer, but I don't care either way. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:234: } this is tricky, ansync is how we did breakpad but we have always had weird crashes, for example it is not clear at all when it returns anything other than 0? "If the operation is asynchronous, ConnectNamedPipe returns immediately. If the operation is still pending, the return value is zero and GetLastError returns ERROR_IO_PENDING. (You can use the HasOverlappedIoCompleted macro to determine when the operation has finished.) If the function fails, the return value is zero and GetLastError returns a value other than ERROR_IO_PENDING or ERROR_PIPE_CONNECTED. If a client connects before the function is called, the function returns zero and GetLastError returns ERROR_PIPE_CONNECTED. This can happen if a client connects in the interval between the call to CreateNamedPipe and the call to ConnectNamedPipe. In this situation, there is a good connection between client and server, even though the function returns zero."
https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:398: while (true) { we also lost the ability to service two clients at the same time? because we are single threaded? I guess you are trading off that over the complexity of having 3 threads (one master two slave)? 'cause we have 3 pipes but if we have 3 processes connecting we still have more latency. Maybe that is the tradeoff you want to make. Let me know. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:432: How does this work? I mean why do we believe we get handle signaling on 434? and if so why just one per event? what if somebody connected at 424? If I am not mistaken in chrome we had issues also keeping alive the overlapped structure long enough because the completion can arrive quite late. The current scheme in chrome involves passing the ownership of the overlapped to some global entity and possibly leaking the overlapped structures.
With regards to the concerns about stability of the async approach in Breakpad: (1) The Breakpad implementation is 931 lines. This implementation is 436. If I include headers it's 1230 vs. 520. https://code.google.com/p/google-breakpad/source/browse/trunk/src/client/wind... (2) The Breakpad implementation only has unit test coverage to test that it doesn't crash when faced with misbehaving clients (306 lines of test). This implementation has much more test coverage, including coverage of proper behaviour with multiple simultaneous clients, etc. (422 lines of test). https://code.google.com/p/google-breakpad/source/browse/trunk/src/client/wind... (3) The Breakpad implementation combines pipe handling code with client registration and dump generation. This implementation decouples those responsibilities, which makes it easier to write, test, and debug. Fun fact: I wrote all of the unit tests for the Breakpad implementation when I fixed some bugs in it in 2010. I'm not trying to slag on the prior code, but I am hoping to illustrate that there are lots of reasons why the former implementation might have been unreliable, at least some of which don't apply here. On the other hand, new code should always be assumed to contain bugs, and that's why I think it is critical that "reporting on the reporter" be considered an essential feature of the new crash reporting facilities. This should be the first line of defense against failure - not side-stepping an API because, anecdotally, it is tricky to use correctly. https://codereview.chromium.org/1126783004/diff/220001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/220001/client/registration_pr... client/registration_protocol_win.h:39: //! \brief An event handle, valid in the client process, that should be On 2015/05/21 16:17:35, scottmg wrote: > Change handle to `HANDLE` since it's now less clear what's supposed to be in > these fields, and note why they can't be HANDLE. Done. https://codereview.chromium.org/1126783004/diff/220001/client/registration_pr... client/registration_protocol_win.h:41: uint32_t request_report_event; On 2015/05/21 16:17:35, scottmg wrote: > I think either WinVMAddress (because it's a void* in the target) or uint32_t > (because we know that only 32 bits are important) is reasonable for this. > > If it comes up again we can add a WinCrossProcessHANDLE (or some better name). Done. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... File handler/win/registration_server.cc (right): https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:37: HMODULE kernel_dll = LoadLibrary(L"kernel32.dll"); On 2015/05/21 22:16:28, cpu wrote: > loadlibrary is far slower than GetModuleHandle, this is sadness that appeared > when windows invented SxS. Done. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:41: GetProcAddress(kernel_dll, "GetNamedPipeClientProcessId")); On 2015/05/21 22:16:28, cpu wrote: > the lines 39-41 are classy. I've been meaning to do this all over chromium (with > a wrapper). You are welcome if you what to tackle that. Credit goes to Scott, who proposed this approach. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:87: On 2015/05/21 22:16:28, cpu wrote: > as a side note c+11 alias makes 86 look nicer, but I don't care either way. Done. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:234: } On 2015/05/21 22:16:28, cpu wrote: > this is tricky, ansync is how we did breakpad but we have always had weird > crashes, for example it is not clear at all when it returns anything other than > 0? > > > "If the operation is asynchronous, ConnectNamedPipe returns immediately. If the > operation is still pending, the return value is zero and GetLastError returns > ERROR_IO_PENDING. (You can use the HasOverlappedIoCompleted macro to determine > when the operation has finished.) If the function fails, the return value is > zero and GetLastError returns a value other than ERROR_IO_PENDING or > ERROR_PIPE_CONNECTED. > If a client connects before the function is called, the function returns zero > and GetLastError returns ERROR_PIPE_CONNECTED. This can happen if a client > connects in the interval between the call to CreateNamedPipe and the call to > ConnectNamedPipe. In this situation, there is a good connection between client > and server, even though the function returns zero." I agree that these comments seem to suggest that this method will always return 0 in async mode. However they also make it very clear what to do if the method does not return 0 - in that case the operation is "[not] still pending" and we can therefore consider it to be connected. So I believe that, according to the documentation, the implementation of IssueConnect is unambiguously good (even if line 222 may never execute). https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:398: while (true) { On 2015/05/21 22:55:58, cpu wrote: > we also lost the ability to service two clients at the same time? because we are > single threaded? > > I guess you are trading off that over the complexity of having 3 threads (one > master two slave)? > > 'cause we have 3 pipes but if we have 3 processes connecting we still have more > latency. Maybe that is the tradeoff you want to make. Let me know. Single-threaded and async IO are not mutually exclusive. But in my opinion multi-threaded is no longer required. Three separate clients may still connect simultaneously. Request handling will be interleaved, meaning that a delay in one client will not affect the others. The server only operates on a request after the data is already available and is not blocked while the client is processing a response. It would be trivial to have the separate pipes operate on separate threads, but I see no reason to think it will be meaningfully faster. I would maintain this simple implementation until benchmarking demonstrated differently. https://codereview.chromium.org/1126783004/diff/260001/handler/win/registrati... handler/win/registration_server.cc:432: On 2015/05/21 22:55:58, cpu wrote: > How does this work? I mean why do we believe we get handle signaling on 434? and > if so why just one per event? > > what if somebody connected at 424? > > If I am not mistaken in chrome we had issues also keeping alive the overlapped > structure long enough because the completion can arrive quite late. The current > scheme in chrome involves passing the ownership of the overlapped to some global > entity and possibly leaking the overlapped structures. In PipeState::Stop we call CancelIo for the pipe. This will cancel the operation that is in progress (causing the operation to complete). When the operation completes, either due to cancellation or because it actually completed (i.e. someone connected or data became available) the event will be signaled. There is only ever one operation going on per pipe at a time. The event is signaled when that operation completes. CancelIo triggers the associated event indirectly, by causing the operation to complete (if and only if it was not already complete).
I still feel a bit uneasy on the overlapped lifetime but I don't have a good argument against it. lets go as is. I know there are bugs in CancelIO() but that was long time ago, for all I know they could have been fixed in SP2 and possibly they only apply to disk filesystems. lgtm
cpu: Please at least OK the following concepts: I found that WriteFile completes immediately, before the client has read the data. A subsequent call to DisconnectNamedPipe will close the pipe and discard the unread data. This was not caught by my original tests, but is caught by the new tests I added (RegistrationPipeStateTest). I solve this problem by issuing a second "ReadFile" call that is not intended to receive any data, but instead to be terminated when the client closes the pipe (after having read the response). The new test suite also thoroughly covers all the possible states in which RegistrationPipeState::Stop might be called.
scottmg: Please review at least for code/comment style in the changes since patchset 15.
I'm concerned by the complexity in maintenance of all of the state involved here, but your explanations and tests seem good to me, so lgtm. https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... client/registration_protocol_win.h:18: #include <windows.h> sort https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... client/registration_protocol_win.h:38: //! https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... silly, but you could use https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203.aspx to avoid 80 col. https://codereview.chromium.org/1126783004/diff/230010/handler/handler_test.gyp File handler/handler_test.gyp (right): https://codereview.chromium.org/1126783004/diff/230010/handler/handler_test.g... handler/handler_test.gyp:41: 'win/registration_test_base.h', sort https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... File handler/win/registration_pipe_state.cc (right): https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... handler/win/registration_pipe_state.cc:18: #include <vector> split C and C++ headers by a newline https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... File handler/win/registration_pipe_state.h (right): https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... handler/win/registration_pipe_state.h:61: //! signaled. 4 space indent https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... File handler/win/registration_pipe_state_test.cc (right): https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... handler/win/registration_pipe_state_test.cc:79: ASSERT_EQ(WAIT_OBJECT_0, It seems a bit strange to have all these be ASSERT_EQ, rather than EXPECT_EQ. Is that intentional? (It doesn't matter too much in the case where there all passing of course.)
https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... File handler/win/registration_pipe_state_test.cc (right): https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... handler/win/registration_pipe_state_test.cc:79: ASSERT_EQ(WAIT_OBJECT_0, On 2015/05/25 18:41:54, scottmg wrote: > It seems a bit strange to have all these be ASSERT_EQ, rather than EXPECT_EQ. Is > that intentional? (It doesn't matter too much in the case where there all > passing of course.) Especially where we are doing blocking operations here, one of the possible outcomes of persisting in the event of an error would be to hang. So it's best to stop as soon as we detect a failure. It's true that for the final statement of each test, it's not required, but I find that to be more error prone (i.e., in the event of additional statements eventually being added) and cognitively demanding than using an ASSERT where an EXPECT would technically suffice.
On 2015/05/25 18:54:23, erikwright wrote: > https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... > File handler/win/registration_pipe_state_test.cc (right): > > https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... > handler/win/registration_pipe_state_test.cc:79: ASSERT_EQ(WAIT_OBJECT_0, > On 2015/05/25 18:41:54, scottmg wrote: > > It seems a bit strange to have all these be ASSERT_EQ, rather than EXPECT_EQ. > Is > > that intentional? (It doesn't matter too much in the case where there all > > passing of course.) > > Especially where we are doing blocking operations here, one of the possible > outcomes of persisting in the event of an error would be to hang. So it's best > to stop as soon as we detect a failure. > > It's true that for the final statement of each test, it's not required, but I > find that to be more error prone (i.e., in the event of additional statements > eventually being added) and cognitively demanding than using an ASSERT where an > EXPECT would technically suffice. PTAL. I find the implementation to be as simple as possible and no simpler. Or as complex as necessary and no more complex, depending on your perspective. I would graciously accept any suggestions as to how it might be simplified further. The RegistrationPipeState test is very thorough and a bit dense. I can think of some ways to maybe improve the readability of that but I'm not sure it's warranted for now. If that's your main cause for concern I can look into it in a follow-up.
On 2015/05/25 19:15:21, erikwright wrote: > On 2015/05/25 18:54:23, erikwright wrote: > > > https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... > > File handler/win/registration_pipe_state_test.cc (right): > > > > > https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... > > handler/win/registration_pipe_state_test.cc:79: ASSERT_EQ(WAIT_OBJECT_0, > > On 2015/05/25 18:41:54, scottmg wrote: > > > It seems a bit strange to have all these be ASSERT_EQ, rather than > EXPECT_EQ. > > Is > > > that intentional? (It doesn't matter too much in the case where there all > > > passing of course.) > > > > Especially where we are doing blocking operations here, one of the possible > > outcomes of persisting in the event of an error would be to hang. So it's best > > to stop as soon as we detect a failure. > > > > It's true that for the final statement of each test, it's not required, but I > > find that to be more error prone (i.e., in the event of additional statements > > eventually being added) and cognitively demanding than using an ASSERT where > an > > EXPECT would technically suffice. > > PTAL. > > I find the implementation to be as simple as possible and no simpler. Or as > complex as necessary and no more complex, depending on your perspective. I would > graciously accept any suggestions as to how it might be simplified further. Sorry, I didn't mean to imply it was more complicated than necessary, just that it was complicated because of all the states the pipe connections can be in. At a minimum, it's a solid start and now we have the interface to work against. If necessary, we can change the implementation behind that in the future if we have a reason to. > > The RegistrationPipeState test is very thorough and a bit dense. I can think of > some ways to maybe improve the readability of that but I'm not sure it's > warranted for now. If that's your main cause for concern I can look into it in a > follow-up.
https://codereview.chromium.org/1126783004/diff/330001/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/330001/client/registration_pr... client/registration_protocol_win.h:18: #include <windows.h> This still needs sorting.
Please expand the CL description a little too, at least to mention that it's Windows-specific.
Sorry, somehow I sent a reply rather than mailing my draft comments. Please see my reply to the sorting comment. https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... client/registration_protocol_win.h:18: #include <windows.h> On 2015/05/25 18:41:54, scottmg wrote: > sort I'm under the impression that it is a convention to place windows.h as the first header (except for XX.h when included from XX.cc). I actually performed the following grep to verify that this is the case in Crashpad: git grep -n --all-match -e windows.h -e stdint.h https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... client/registration_protocol_win.h:38: //! https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... On 2015/05/25 18:41:54, scottmg wrote: > silly, but you could use > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203.aspx > > to avoid 80 col. Done. https://codereview.chromium.org/1126783004/diff/230010/handler/handler_test.gyp File handler/handler_test.gyp (right): https://codereview.chromium.org/1126783004/diff/230010/handler/handler_test.g... handler/handler_test.gyp:41: 'win/registration_test_base.h', On 2015/05/25 18:41:54, scottmg wrote: > sort Done. https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... File handler/win/registration_pipe_state.cc (right): https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... handler/win/registration_pipe_state.cc:18: #include <vector> On 2015/05/25 18:41:54, scottmg wrote: > split C and C++ headers by a newline Done. https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... File handler/win/registration_pipe_state.h (right): https://codereview.chromium.org/1126783004/diff/230010/handler/win/registrati... handler/win/registration_pipe_state.h:61: //! signaled. On 2015/05/25 18:41:54, scottmg wrote: > 4 space indent Done.
https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... File client/registration_protocol_win.h (right): https://codereview.chromium.org/1126783004/diff/230010/client/registration_pr... client/registration_protocol_win.h:18: #include <windows.h> On 2015/05/25 19:42:12, erikwright wrote: > On 2015/05/25 18:41:54, scottmg wrote: > > sort > > I'm under the impression that it is a convention to place windows.h as the first > header (except for XX.h when included from XX.cc). > > I actually performed the following grep to verify that this is the case in > Crashpad: > > git grep -n --all-match -e windows.h -e stdint.h Huh! Fair enough. There are occasional broken headers that require windows before them so I guess that makes sense. This is good to go as far as I'm concerned, once you get whatever feedback you'd like from Carlos.
Message was sent while issue was closed.
Committed patchset #19 (id:350001) manually as 9ff3d9335fd0a8e3b9e84e88d565945acdb2fa94 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
