Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831271:
[Chromoting] Adding uiAccess='true' to the remoting_me2me_host.exe's manifest as it is required to … (Closed)
|
Created:
8 years, 4 months ago by alexeypa (please no reviews) Modified:
8 years, 4 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, sadrul, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, simonmorris+watch_chromium.org, brettw-cc_chromium.org, alexeypa+watch_chromium.org, erikwright+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Chromoting] Launch the host process elevated via ShellExecuteEx().
The host process is launched in in two steps now. An instance of remote_service.exe is launched in a user session (CreateProcessAsUser) and then it launches the host requesting elevation (ShellExecuteEx). This is needed because Windows 8 refuses to inject Alt+Tab unless uiAccess='true' is specified in the manifest, which in its turn requires ShellExecuteEx() to be used.
Lifetime of launched processes is controlled by assigning them to a job object.
Message loop changes are required to be able to process job object notifications on the I/O message loop.
BUG=135217
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151973
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR feedback. #
Total comments: 18
Patch Set 3 : More CR feedback. #
Total comments: 46
Patch Set 4 : CR feedback. #
Total comments: 26
Patch Set 5 : - #
Total comments: 2
Patch Set 6 : - #Patch Set 7 : rebased + a couple of merge fixes. #
Messages
Total messages: 20 (0 generated)
PTAL. jar@ -> base/* wez@ -> remoting/*
I'm going to ask Ricardo to review the IO completion port stuff in bas, and then I'll rubber stamp (base at least). https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_loop.h File base/message_loop.h (right): https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_loop.h#ne... base/message_loop.h:635: bool RegisterJobObject(HANDLE job, IOHandler* handler); Please add comment explaining that the return boolean value is meant to be. Comment for line 636 would also be appreciated. https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_pump_win.h File base/message_pump_win.h (right): https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_pump_win.... base/message_pump_win.h:326: bool has_context; With a pointer, it is confusing to have a bool, and a pointer. Perhaps a better name, such as: has_valid_io_context would help to make this more readable. https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_pump_win.... base/message_pump_win.h:338: // specifies whether complation packets will have valid OVERLAPPED pointers. typo: complation
https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_loop.h File base/message_loop.h (right): https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_loop.h#ne... base/message_loop.h:635: bool RegisterJobObject(HANDLE job, IOHandler* handler); On 2012/08/11 02:30:01, jar wrote: > Please add comment explaining that the return boolean value is meant to be. > > Comment for line 636 would also be appreciated. Done. https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_pump_win.h File base/message_pump_win.h (right): https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_pump_win.... base/message_pump_win.h:326: bool has_context; On 2012/08/11 02:30:01, jar wrote: > With a pointer, it is confusing to have a bool, and a pointer. Perhaps a better > name, such as: > > has_valid_io_context > > would help to make this more readable. Done. https://chromiumcodereview.appspot.com/10831271/diff/1/base/message_pump_win.... base/message_pump_win.h:338: // specifies whether complation packets will have valid OVERLAPPED pointers. On 2012/08/11 02:30:01, jar wrote: > typo: complation Done.
http://codereview.chromium.org/10831271/diff/5001/base/message_loop.h File base/message_loop.h (right): http://codereview.chromium.org/10831271/diff/5001/base/message_loop.h#newcode640 base/message_loop.h:640: // until the job object is destoyed. Returns true if the registration typo: destroyed http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:564: if (!item.has_valid_io_context || item.context->handler) { This deserves a comment given that the first condition is redundant and we are "just" avoiding touching invalid memory with the second condition. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:652: key = key | 1; ouch http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:653: } nit: single line if without {} http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:660: bool* has_valid_io_context_out) { nit: naming an out argument _out is highly atypical. Avoid it. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.h#new... base/message_pump_win.h:299: // Register the handler to be used to process job events. The registration We should not have copy-pasted documentation. Either Message_loop.h points the reader to message_pump_win.h or the other way around. http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/host_serv... File remoting/host/win/host_service.cc (right): http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/host_serv... remoting/host/win/host_service.cc:446: int CALLBACK WinMain(HINSTANCE /* instance */, nit: why not just leave the variable name without comment it out? http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.h (right): http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.h:79: void DrainSandboxNotifications(); Avoid naming a job object a "sandbox". Everywhere.
http://codereview.chromium.org/10831271/diff/5001/base/message_loop.h File base/message_loop.h (right): http://codereview.chromium.org/10831271/diff/5001/base/message_loop.h#newcode640 base/message_loop.h:640: // until the job object is destoyed. Returns true if the registration On 2012/08/13 18:54:12, rvargas wrote: > typo: destroyed Done. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:564: if (!item.has_valid_io_context || item.context->handler) { On 2012/08/13 18:54:12, rvargas wrote: > This deserves a comment given that the first condition is redundant and we are > "just" avoiding touching invalid memory with the second condition. Done, though both conditions are not redundant. Or I'm interpreting your comment incorrectly. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:652: key = key | 1; On 2012/08/13 18:54:12, rvargas wrote: > ouch I think introducing a constant instead on the flag will make the code less compact and less readable. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:653: } On 2012/08/13 18:54:12, rvargas wrote: > nit: single line if without {} Done. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:660: bool* has_valid_io_context_out) { On 2012/08/13 18:54:12, rvargas wrote: > nit: naming an out argument _out is highly atypical. Avoid it. Done. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.h#new... base/message_pump_win.h:299: // Register the handler to be used to process job events. The registration On 2012/08/13 18:54:12, rvargas wrote: > We should not have copy-pasted documentation. Either Message_loop.h points the > reader to message_pump_win.h or the other way around. Done. http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/host_serv... File remoting/host/win/host_service.cc (right): http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/host_serv... remoting/host/win/host_service.cc:446: int CALLBACK WinMain(HINSTANCE /* instance */, On 2012/08/13 18:54:12, rvargas wrote: > nit: why not just leave the variable name without comment it out? Done. http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.h (right): http://codereview.chromium.org/10831271/diff/5001/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.h:79: void DrainSandboxNotifications(); On 2012/08/13 18:54:12, rvargas wrote: > Avoid naming a job object a "sandbox". Everywhere. Done.
base LGTM http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:564: if (!item.has_valid_io_context || item.context->handler) { On 2012/08/13 20:02:23, alexeypa wrote: > On 2012/08/13 18:54:12, rvargas wrote: > > This deserves a comment given that the first condition is redundant and we are > > "just" avoiding touching invalid memory with the second condition. > > Done, though both conditions are not redundant. Or I'm interpreting your comment > incorrectly. > Thanks. What I meant is that (ignoring dereferencing an invalid pointer), handler cannot be NULL when has_valid_io_context is false (as in, the caller cannot clear up the handler after it was registered, for job related operations), so the job case would always flow into the if clause 'cause handler would be true. Of course, then we have the whole issue about not being able to see the handler at all (because there's no storage for it in the first place), but then the code doesn't follow the common pattern of "if (a.valid && a.use)"... so the comment helps. Another way to look at it: The if is basically saying "was this item cancelled or do I have to process it". The condition of job || handler is correct. I just wanted a comment pointing out why |handler| alone was not enough/possible. http://codereview.chromium.org/10831271/diff/5001/base/message_pump_win.cc#ne... base/message_pump_win.cc:652: key = key | 1; On 2012/08/13 20:02:23, alexeypa wrote: > On 2012/08/13 18:54:12, rvargas wrote: > > ouch > > I think introducing a constant instead on the flag will make the code less > compact and less readable. yeah... I was just showing pain, not suggesting that it should be changed.
On 2012/08/10 19:10:48, alexeypa wrote: > PTAL. > > jar@ -> base/* > wez@ -> remoting/* Can you update the description so the first line describes more accurately what this CL does? It does rather more than add uiAccess=true to the manifest! ;)
http://codereview.chromium.org/10831271/diff/1014/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/1014/base/message_pump_win.cc#ne... base/message_pump_win.cc:652: DCHECK((key & 1) == 0); nit: This check is for even (16-bit word) alignment, whereas the comment implies that it's a pointer (i.e. 32-bit or 64-bit) alignment check. Suggest rewording e.g. "IOHandler is at least pointer-size aligned, and hence even." or similar. http://codereview.chromium.org/10831271/diff/1014/base/message_pump_win.cc#ne... base/message_pump_win.cc:664: *has_valid_io_context = !(key & 1); nit: (key & 1) == 0 http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... File remoting/host/win/host_service.cc (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:63: // promt if required. nit: ... to be launched elevated, presenting a UAC prompt if necessary. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:75: L" --console - Run the service interactively for debugging purposes.\n" Will the --console option still work now that we're running with WinMain()? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:174: // Perform elevation is requested. nit: Launch with elevation was requested. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:252: info.lpParameters = parameters.c_str(); Does ShellExecute("RunAs") definitely not expect the binary name to be the first item in the parameters string? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... File remoting/host/win/host_service.h (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.h:64: // Runs the specified binary elevated. nit: There is no parameter; where is the binary specified? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.cc (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:87: // Job object initalization has to be performed on the I/O thread. nit: Why? :) e.g. because an I/O thread is needed to handle job notifications via a completion port? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:97: DCHECK(!timer_.IsRunning()); nit: The destructor DCHECKs should probably be CHECKs, since obscure shutdown race-conditions may occur in the wild that we won't see in debug builds. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:135: Stop(); I'm confused; isn't Stop() what the caller called to get us here? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:170: DWORD bytes_transfered, typo: bytes_transferred http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:175: // the message ID. nit: Suggest "... used in job notifications to supply the message ID." http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:191: // Wait until the sandbox is ready. nit: You mean that we can't launch until the sandbox is ready? Will we re-try the operation once it's ready? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:216: CHECK(result); nit: Do you need to create a local variable for this rather than CHECK(ResetEvent)? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:233: TerminateProcess(worker_process, CONTROL_C_EXIT); nit: ::TerminateProcess()? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:275: // Ignore the error. If the processes cannot be open the error code is generic typo: processes http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:351: if (job_state_ > kJobUninitialized) { Ick. Do you really need to use more-than here? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:359: // Wait for |launcher_| to be completely stopped. nit: You're not waiting for anything here; you mean don't complete shutdown if the |launcher_| isn't stopped yet? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:364: // Wait for the completion queue to be drained. nit: Similarly, you mean don't complete shutdown until the completion queue is drained? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:372: void WtsSessionProcessLauncher::DrainJobNotifications() { nit: This method just posts a notification back to the calling thread; it doesn't actually drain anything. Tricky to think of a suitably descriptive name. Perhaps OnJobShutdownCompleteOnIpcThread? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:377: // already. Let the main thread know that the queue has been drained. This assumes that the notifications are queued to the IPC thread before the PostTask()? Is that guaranteed by Windows, or would it be legal for it to defer those notifications in some situations? http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.h (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.h:54: virtual ~WtsSessionProcessLauncher(); You're using base::Unretained in PostTask() on this object, so you need to be very clear about when the caller can safely tear down this object (even if that just means a reminder to check the requirements imposed by Stoppable); if they tear-down after DoStop() has run but before the job drain dance has completed then a task will be posted to a dead object. Another way to better document why base::Unretained is safe would be to DCHECK() the launcher object's state immediately before the relevant PostTasks. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.h:77: // Drains the completion port queue to make sure that all job object nit: Clarify that you're referring to the Windows completion port for the job, rather than some primitive in Chrome.
http://codereview.chromium.org/10831271/diff/1014/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/1014/base/message_pump_win.cc#ne... base/message_pump_win.cc:652: DCHECK((key & 1) == 0); On 2012/08/14 01:08:59, Wez wrote: > nit: This check is for even (16-bit word) alignment, whereas the comment implies > that it's a pointer (i.e. 32-bit or 64-bit) alignment check. Suggest rewording > e.g. "IOHandler is at least pointer-size aligned, and hence even." or similar. Done. http://codereview.chromium.org/10831271/diff/1014/base/message_pump_win.cc#ne... base/message_pump_win.cc:664: *has_valid_io_context = !(key & 1); On 2012/08/14 01:08:59, Wez wrote: > nit: (key & 1) == 0 Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... File remoting/host/win/host_service.cc (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:63: // promt if required. On 2012/08/14 01:08:59, Wez wrote: > nit: ... to be launched elevated, presenting a UAC prompt if necessary. Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:75: L" --console - Run the service interactively for debugging purposes.\n" On 2012/08/14 01:08:59, Wez wrote: > Will the --console option still work now that we're running with WinMain()? Yes, Ctrl+C will not work, but for debugging --console is still super convenient. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:174: // Perform elevation is requested. On 2012/08/14 01:08:59, Wez wrote: > nit: Launch with elevation was requested. Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.cc:252: info.lpParameters = parameters.c_str(); On 2012/08/14 01:08:59, Wez wrote: > Does ShellExecute("RunAs") definitely not expect the binary name to be the first > item in the parameters string? No, it does not. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... File remoting/host/win/host_service.h (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/host_serv... remoting/host/win/host_service.h:64: // Runs the specified binary elevated. On 2012/08/14 01:08:59, Wez wrote: > nit: There is no parameter; where is the binary specified? Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.cc (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:87: // Job object initalization has to be performed on the I/O thread. On 2012/08/14 01:08:59, Wez wrote: > nit: Why? :) e.g. because an I/O thread is needed to handle job notifications > via a completion port? Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:97: DCHECK(!timer_.IsRunning()); On 2012/08/14 01:08:59, Wez wrote: > nit: The destructor DCHECKs should probably be CHECKs, since obscure shutdown > race-conditions may occur in the wild that we won't see in debug builds. Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:135: Stop(); On 2012/08/14 01:08:59, Wez wrote: > I'm confused; isn't Stop() what the caller called to get us here? Yes, but it does not mean that Stop() is still on the stack. Stop() could post tasks that eventually resulted in OnLauncherStopped() to be called. At this point we need to call Stop() again to resume shutting down from where we left it. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:170: DWORD bytes_transfered, On 2012/08/14 01:08:59, Wez wrote: > typo: bytes_transferred Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:175: // the message ID. On 2012/08/14 01:08:59, Wez wrote: > nit: Suggest "... used in job notifications to supply the message ID." Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:191: // Wait until the sandbox is ready. On 2012/08/14 01:08:59, Wez wrote: > Will we re-try > the operation once it's ready? Yes. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:216: CHECK(result); On 2012/08/14 01:08:59, Wez wrote: > nit: Do you need to create a local variable for this rather than > CHECK(ResetEvent)? Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:233: TerminateProcess(worker_process, CONTROL_C_EXIT); On 2012/08/14 01:08:59, Wez wrote: > nit: ::TerminateProcess()? '::' is not necessary, less common in Chromium code and frowned at by many reviewer. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:275: // Ignore the error. If the processes cannot be open the error code is generic On 2012/08/14 01:08:59, Wez wrote: > typo: processes Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:351: if (job_state_ > kJobUninitialized) { On 2012/08/14 01:08:59, Wez wrote: > Ick. Do you really need to use more-than here? No, that is wrong. It should list specific states. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:359: // Wait for |launcher_| to be completely stopped. On 2012/08/14 01:08:59, Wez wrote: > nit: You're not waiting for anything here; you mean don't complete shutdown if > the |launcher_| isn't stopped yet? Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:364: // Wait for the completion queue to be drained. On 2012/08/14 01:08:59, Wez wrote: > nit: Similarly, you mean don't complete shutdown until the completion queue is > drained? Done. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:372: void WtsSessionProcessLauncher::DrainJobNotifications() { On 2012/08/14 01:08:59, Wez wrote: > OnJobShutdownCompleteOnIpcThread? |DrainJobNotifications| describes what happens, but does not document how. |OnJobShutdownCompleteOnIpcThread| documents when something happens, but does not say why and what. I think the former is a better name. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:377: // already. Let the main thread know that the queue has been drained. On 2012/08/14 01:08:59, Wez wrote: > Is that guaranteed by Windows Yes, no notifications can be posted after the job object has been destroyed. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.h (right): http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.h:54: virtual ~WtsSessionProcessLauncher(); On 2012/08/14 01:08:59, Wez wrote: > You're using base::Unretained in PostTask() on this object, so you need to be > very clear about when the caller can safely tear down this object (even if that > just means a reminder to check the requirements imposed by Stoppable); if they > tear-down after DoStop() has run but before the job drain dance has completed > then a task will be posted to a dead object. > Another way to better document why base::Unretained is safe would be to DCHECK() > the launcher object's state immediately before the relevant PostTasks. Added CHECKs to ~Stoppable and ~WtsSessionProcessLauncher. http://codereview.chromium.org/10831271/diff/1014/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.h:77: // Drains the completion port queue to make sure that all job object On 2012/08/14 01:08:59, Wez wrote: > nit: Clarify that you're referring to the Windows completion port for the job, > rather than some primitive in Chrome. Searching Chromium tree reveals that the terms "completion port" and "job object" are used exclusively to refer to NT completion port and NT job object.
Wez, ping.
http://codereview.chromium.org/10831271/diff/5002/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/5002/base/message_pump_win.cc#ne... base/message_pump_win.cc:567: // a valid non-pointer value. Suggest rewording: "If |item.has_valid_io_context| is false then the context does not point to a context structure, and so should not be dereferenced, although it may still hold valid non-pointer data." http://codereview.chromium.org/10831271/diff/5002/remoting/base/stoppable.cc File remoting/base/stoppable.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/base/stoppable.cc#... remoting/base/stoppable.cc:22: // been shutdown correctly. Isn't that implicit in the CHECK itself? You're simply verifying that the implementation is fully stopped? http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... File remoting/host/win/host_service.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... remoting/host/win/host_service.cc:63: // a UAC promt if necessary. typo: prompt http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... remoting/host/win/host_service.cc:446: int CALLBACK WinMain(HINSTANCE instance , nit: remove the errant space after |instance| http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... File remoting/host/win/host_service.h (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... remoting/host/win/host_service.h:64: // Runs the binary specified in the command line elevated. nit: "Runs the binary specified by the command line, elevated." or "... with elevation." http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:89: // MessageLoopForIO::current(). If that's the only limitation then why not move RegisterJobObject() to MessageLoop's interface? Presumably there is something that RegisterJobObject does that may be time-consuming and therefore needs an I/O loop? http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:99: // there still could be pending tasks posted to it. nit: You're checking this here, in addition to the same check in the Stoppable dtor, to make sure that we're properly stopped before you start tearing this object down. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:100: CHECK_EQ(stoppable_state(), Stoppable::kStopped); nit: Doesn't this check belong before the RemoveWtsConsoleObserver call? http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:275: // Ignore the error. Generic |CONTROL_C_EXIT| will be used if the process nit: What does "will be used" mean here? You mean we ignore the error because it will be reported as CONTROL_C_EXIT regardless of the actual cause?
http://codereview.chromium.org/10831271/diff/1/base/message_loop.h File base/message_loop.h (right): http://codereview.chromium.org/10831271/diff/1/base/message_loop.h#newcode635 base/message_loop.h:635: bool RegisterJobObject(HANDLE job, IOHandler* handler); I didn't see the added comments. On 2012/08/13 15:15:14, alexeypa wrote: > On 2012/08/11 02:30:01, jar wrote: > > Please add comment explaining that the return boolean value is meant to be. > > > > Comment for line 636 would also be appreciated. > > Done.
wez@, PTAL. jar@, I've answered your question below. http://codereview.chromium.org/10831271/diff/1/base/message_loop.h File base/message_loop.h (right): http://codereview.chromium.org/10831271/diff/1/base/message_loop.h#newcode635 base/message_loop.h:635: bool RegisterJobObject(HANDLE job, IOHandler* handler); On 2012/08/15 23:42:03, jar wrote: > I didn't see the added comments. rvargas@ asked me not to copy-paste documentation. The added comments are in message_pump_win.h. http://codereview.chromium.org/10831271/diff/5002/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/5002/base/message_pump_win.cc#ne... base/message_pump_win.cc:567: // a valid non-pointer value. On 2012/08/15 23:22:20, Wez wrote: > Suggest rewording: "If |item.has_valid_io_context| is false then the context > does not point to a context structure, and so should not be dereferenced, > although it may still hold valid non-pointer data." Done. http://codereview.chromium.org/10831271/diff/5002/remoting/base/stoppable.cc File remoting/base/stoppable.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/base/stoppable.cc#... remoting/base/stoppable.cc:22: // been shutdown correctly. On 2012/08/15 23:22:20, Wez wrote: > Isn't that implicit in the CHECK itself? You're simply verifying that the > implementation is fully stopped? I removed the comment. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... File remoting/host/win/host_service.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... remoting/host/win/host_service.cc:63: // a UAC promt if necessary. On 2012/08/15 23:22:20, Wez wrote: > typo: prompt Done. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... remoting/host/win/host_service.cc:446: int CALLBACK WinMain(HINSTANCE instance , On 2012/08/15 23:22:20, Wez wrote: > nit: remove the errant space after |instance| Done. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... File remoting/host/win/host_service.h (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/host_serv... remoting/host/win/host_service.h:64: // Runs the binary specified in the command line elevated. On 2012/08/15 23:22:20, Wez wrote: > nit: "Runs the binary specified by the command line, elevated." or "... with > elevation." Done. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:89: // MessageLoopForIO::current(). On 2012/08/15 23:22:20, Wez wrote: > If that's the only limitation then why not move RegisterJobObject() to > MessageLoop's interface? It does not belong there (and to MessageLoopProxy and SingleThreadTaskRunner as well). It is I/O specific just like RegisterIOHandler. > Presumably there is something that RegisterJobObject > does that may be time-consuming and therefore needs an I/O loop? It does not do anything time consuming. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:99: // there still could be pending tasks posted to it. On 2012/08/15 23:22:20, Wez wrote: > nit: You're checking this here, in addition to the same check in the Stoppable > dtor, to make sure that we're properly stopped before you start tearing this > object down. I'm duplicating this check for a reason. First, WtsSessionProcessLauncher is mostly not destroyed here, while in ~Stoppable it is mostly gone. Second, the comment here explains why it is CHECK and not DCHECK. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:100: CHECK_EQ(stoppable_state(), Stoppable::kStopped); On 2012/08/15 23:22:20, Wez wrote: > nit: Doesn't this check belong before the RemoveWtsConsoleObserver call? It does not really matter. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:275: // Ignore the error. Generic |CONTROL_C_EXIT| will be used if the process On 2012/08/15 23:22:20, Wez wrote: > nit: What does "will be used" mean here? You mean we ignore the error because it > will be reported as CONTROL_C_EXIT regardless of the actual cause? OpenProcess can fail. I explain why this error is ignored and what we do if it happens.
base LGTM (rubber stamp of Ricardo's review).
LGTM once my nit-picky comment nits are picked. :) http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:89: // MessageLoopForIO::current(). On 2012/08/15 23:55:50, alexeypa wrote: > On 2012/08/15 23:22:20, Wez wrote: > > If that's the only limitation then why not move RegisterJobObject() to > > MessageLoop's interface? > > It does not belong there (and to MessageLoopProxy and SingleThreadTaskRunner as > well). It is I/O specific just like RegisterIOHandler. > > > Presumably there is something that RegisterJobObject > > does that may be time-consuming and therefore needs an I/O loop? > > It does not do anything time consuming. OK, so the comment should state that job initialization also registers the object for notifications, with the MessageLoop, which requires an I/O message-loop. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:99: // there still could be pending tasks posted to it. On 2012/08/15 23:55:50, alexeypa wrote: > On 2012/08/15 23:22:20, Wez wrote: > > nit: You're checking this here, in addition to the same check in the Stoppable > > dtor, to make sure that we're properly stopped before you start tearing this > > object down. > > I'm duplicating this check for a reason. First, WtsSessionProcessLauncher is > mostly not destroyed here, while in ~Stoppable it is mostly gone. Second, the > comment here explains why it is CHECK and not DCHECK. Right, so the comment should simply clarify that we're checking that we're completely stopped before we start destroying this object. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:100: CHECK_EQ(stoppable_state(), Stoppable::kStopped); On 2012/08/15 23:55:50, alexeypa wrote: > On 2012/08/15 23:22:20, Wez wrote: > > nit: Doesn't this check belong before the RemoveWtsConsoleObserver call? > > It does not really matter. Please move it before the RemoveWtsConsoleObserver() call, then, so that if someone naively adds new code to the dtor, they're less likely to mistakenly put it before this check. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:275: // Ignore the error. Generic |CONTROL_C_EXIT| will be used if the process On 2012/08/15 23:55:50, alexeypa wrote: > On 2012/08/15 23:22:20, Wez wrote: > > nit: What does "will be used" mean here? You mean we ignore the error because > it > > will be reported as CONTROL_C_EXIT regardless of the actual cause? > > OpenProcess can fail. I explain why this error is ignored and what we do if it > happens. My point is just that the explanation is not clear; what you're actually saying is: "If OpenProcess() fails then our later GetExitCodeProcess() call will fail, which we treat as |CONTROL_C_EXIT|, so we don't need to check for errors here." http://codereview.chromium.org/10831271/diff/1017/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/1017/base/message_pump_win.cc#ne... base/message_pump_win.cc:564: // If |item.has_valid_io_context| is false then the context does not point nit: the context -> |item.context| (Sorry, I ham-worded my suggestion...)
http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... File remoting/host/win/wts_session_process_launcher.cc (right): http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:89: // MessageLoopForIO::current(). On 2012/08/16 00:06:33, Wez wrote: > On 2012/08/15 23:55:50, alexeypa wrote: > > On 2012/08/15 23:22:20, Wez wrote: > > > If that's the only limitation then why not move RegisterJobObject() to > > > MessageLoop's interface? > > > > It does not belong there (and to MessageLoopProxy and SingleThreadTaskRunner > as > > well). It is I/O specific just like RegisterIOHandler. > > > > > Presumably there is something that RegisterJobObject > > > does that may be time-consuming and therefore needs an I/O loop? > > > > It does not do anything time consuming. > > OK, so the comment should state that job initialization also registers the > object for notifications, with the MessageLoop, which requires an I/O > message-loop. Done. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:99: // there still could be pending tasks posted to it. On 2012/08/16 00:06:33, Wez wrote: > On 2012/08/15 23:55:50, alexeypa wrote: > > On 2012/08/15 23:22:20, Wez wrote: > > > nit: You're checking this here, in addition to the same check in the > Stoppable > > > dtor, to make sure that we're properly stopped before you start tearing this > > > object down. > > > > I'm duplicating this check for a reason. First, WtsSessionProcessLauncher is > > mostly not destroyed here, while in ~Stoppable it is mostly gone. Second, the > > comment here explains why it is CHECK and not DCHECK. > > Right, so the comment should simply clarify that we're checking that we're > completely stopped before we start destroying this object. Done. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:100: CHECK_EQ(stoppable_state(), Stoppable::kStopped); On 2012/08/16 00:06:33, Wez wrote: > On 2012/08/15 23:55:50, alexeypa wrote: > > On 2012/08/15 23:22:20, Wez wrote: > > > nit: Doesn't this check belong before the RemoveWtsConsoleObserver call? > > > > It does not really matter. > > Please move it before the RemoveWtsConsoleObserver() call, then, so that if > someone naively adds new code to the dtor, they're less likely to mistakenly put > it before this check. Done. http://codereview.chromium.org/10831271/diff/5002/remoting/host/win/wts_sessi... remoting/host/win/wts_session_process_launcher.cc:275: // Ignore the error. Generic |CONTROL_C_EXIT| will be used if the process On 2012/08/16 00:06:33, Wez wrote: > On 2012/08/15 23:55:50, alexeypa wrote: > > On 2012/08/15 23:22:20, Wez wrote: > > > nit: What does "will be used" mean here? You mean we ignore the error > because > > it > > > will be reported as CONTROL_C_EXIT regardless of the actual cause? > > > > OpenProcess can fail. I explain why this error is ignored and what we do if it > > happens. > > My point is just that the explanation is not clear; what you're actually saying > is: > "If OpenProcess() fails then our later GetExitCodeProcess() call will fail, > which we treat as |CONTROL_C_EXIT|, so we don't need to check for errors here." Done. http://codereview.chromium.org/10831271/diff/1017/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10831271/diff/1017/base/message_pump_win.cc#ne... base/message_pump_win.cc:564: // If |item.has_valid_io_context| is false then the context does not point On 2012/08/16 00:06:33, Wez wrote: > nit: the context -> |item.context| > > (Sorry, I ham-worded my suggestion...) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10831271/13005
Try job failure for 10831271-13005 (retry) on win_rel for step "content_browsertests". It's a second try, previously, steps "nacl_integration, content_browsertests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10831271/13005
Change committed as 151973 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
