Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9476017:
Chromoting service now launches the host process in the session the is currently attached to the ph… (Closed)
|
Created:
8 years, 9 months ago by alexeypa (please no reviews) Modified:
8 years, 9 months ago Reviewers:
Wez CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChromoting service now launches the host process in the session the is currently attached to the physical console. The name of the host process binary is specified when the service is installed.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124390
Patch Set 1 #
Total comments: 53
Patch Set 2 : CR feedback. Part 1. #Patch Set 3 : Removing scoped_ptr<ScopedHandle>. #
Total comments: 12
Patch Set 4 : Cosmetic. #
Messages
Total messages: 10 (0 generated)
Please take a look.
https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... File remoting/host/host_service_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:65: // "--host-binary" specifies the host bonary to run in console session. typo: binary https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:192: } else if (host_binary_required) { nit: I think this would be clearer as: if (host_binary_required) { if (HasSwitch()) { } else { } } https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:194: << " is required."; nit: Indentation. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... File remoting/host/wts_session_process_launcher_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:34: scoped_ptr<ScopedHandle>* token_out) { Why does this need wrapping in a scoped_ptr<>? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:45: process_token.reset(new base::win::ScopedHandle(handle)); This could become: ScopedHandle process_token(handle); surely? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:57: token_out->reset(new ScopedHandle(handle)); And this would become: token_out->Set(process_token.Take()); https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:62: bool CreatePrivilegedToken(scoped_ptr<ScopedHandle>* token_out) { Similarly, why wrap in a scoped_ptr<>? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:118: // Launches |binary| in the security context of the user represented by nit: ... of the supplied |user_token|. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:122: base::Process* process_out) { Why not return a scoped_ptr<base::Process>, rather than having to have an out-parameter? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:189: } else { nit: Is there anything meaningful to log in this case, or does StartWatching() log something on failure? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:235: DCHECK(state_ == StateDetached); This should probably be a CHECK, since OnSessionAttached is called based on notifications from the OS and so whether it gets called in the way we expect is not 100% in our control. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:240: // Temporary enable the SE_TCB_NAME priviledge by impersonating a privileged typo: Temporarily typo: privilege https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:240: // Temporary enable the SE_TCB_NAME priviledge by impersonating a privileged Suggest rewording: ... by impersonating a copy of our process token with the privilege enabled. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:256: BOOL result = CreateSessionToken(session_id, &session_token_); nit: CreateSessionToken returns bool, not BOOL. :) https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:260: if (!RevertToSelf()) { The process should crash if this fails. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:279: case StateDetached: Why does OnSessionDetached cope with being called when already detached, but OnSessionAttached DCHECK in the equivalent situation? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... File remoting/host/wts_session_process_launcher_win.h (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:12: #include "base/memory/scoped_ptr.h" nit: This include goes after compiler_specific.h https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:38: // class. |host_binary| is the host executable name to be launched in nit: ... is the name of the executable to be launched ... https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:39: // the session(s) attached to the console. nit: the console session. There can't be >1 console session, i.e. no plurals, AFAIK. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:76: // A handle of the process injected into the watched session. nit: A -> The https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:82: // A token to be used to launch a process in a different session. nit: A -> The This is the token with the console session-Id set in it, ready for launch, right? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:85: // Defines the states the process launcher can be. nit: ... can be in. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:95: // This time is used to delay next attempt to launch the host. nit: suggest "Timer used to schedule the next attempt to launch the process." https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:96: base::OneShotTimer<WtsSessionProcessLauncher> timer_; Why is this timer down here when the |launch_time_| and |launch_backoff_| appear earlier in the file?
PTAL. I'll make more changes after we sort out scoped_ptr<ScopedHandle>. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... File remoting/host/host_service_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:65: // "--host-binary" specifies the host bonary to run in console session. On 2012/02/28 00:22:07, Wez wrote: > typo: binary Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:192: } else if (host_binary_required) { On 2012/02/28 00:22:07, Wez wrote: > nit: I think this would be clearer as: > > if (host_binary_required) { > if (HasSwitch()) { > } else { > } > } Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:194: << " is required."; On 2012/02/28 00:22:07, Wez wrote: > nit: Indentation. This is how C++ style guide recommends to indent '<<'. We use same indentation in other places too. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... File remoting/host/wts_session_process_launcher_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:34: scoped_ptr<ScopedHandle>* token_out) { On 2012/02/28 00:22:07, Wez wrote: > Why does this need wrapping in a scoped_ptr<>? ScopedHandle does not provide either copy or ownership semantic. So scoped_ptr provides the latter. I agree it makes code a little bit messier. I can give up the strict ownership semantic and use naked ScopedHandle instead. It is just that I'll have to be more careful. So you think it worth a shot? https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:45: process_token.reset(new base::win::ScopedHandle(handle)); On 2012/02/28 00:22:07, Wez wrote: > This could become: > > ScopedHandle process_token(handle); > > surely? Yes. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:57: token_out->reset(new ScopedHandle(handle)); On 2012/02/28 00:22:07, Wez wrote: > And this would become: > > token_out->Set(process_token.Take()); Yes. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:62: bool CreatePrivilegedToken(scoped_ptr<ScopedHandle>* token_out) { On 2012/02/28 00:22:07, Wez wrote: > Similarly, why wrap in a scoped_ptr<>? Again, to underline ownership. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:118: // Launches |binary| in the security context of the user represented by On 2012/02/28 00:22:07, Wez wrote: > nit: ... of the supplied |user_token|. Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:122: base::Process* process_out) { On 2012/02/28 00:22:07, Wez wrote: > Why not return a scoped_ptr<base::Process>, rather than having to have an > out-parameter? This makes the caller's code look like all other code that calls Win32 APIs. So it looks more uniform. But, I could wrap base::Process into scoped_ptr to make the point about the ownership. This however should done only if ScopedHandle instances above remain wrapped in scoped_ptr. So I'll wait changing the code until we settle on something. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:189: } else { On 2012/02/28 00:22:07, Wez wrote: > nit: Is there anything meaningful to log in this case, or does StartWatching() > log something on failure? StartWatching() will print details to the log. I think it is still worth logging this error from our code. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:235: DCHECK(state_ == StateDetached); On 2012/02/28 00:22:07, Wez wrote: > This should probably be a CHECK, since OnSessionAttached is called based on > notifications from the OS and so whether it gets called in the way we expect is > not 100% in our control. It is actually in our control. WtsConsoleObserver's contract is that Attached and Detached calls are properly ordered. We actually make sure that this happens even if the OS does not give us proper ordering. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:240: // Temporary enable the SE_TCB_NAME priviledge by impersonating a privileged On 2012/02/28 00:22:07, Wez wrote: > typo: Temporarily > typo: privilege Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:240: // Temporary enable the SE_TCB_NAME priviledge by impersonating a privileged On 2012/02/28 00:22:07, Wez wrote: > Suggest rewording: ... by impersonating a copy of our process token with the > privilege enabled. Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:256: BOOL result = CreateSessionToken(session_id, &session_token_); On 2012/02/28 00:22:07, Wez wrote: > nit: CreateSessionToken returns bool, not BOOL. :) Indeed. :-) https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:260: if (!RevertToSelf()) { On 2012/02/28 00:22:07, Wez wrote: > The process should crash if this fails. Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.cc:279: case StateDetached: On 2012/02/28 00:22:07, Wez wrote: > Why does OnSessionDetached cope with being called when already detached, but > OnSessionAttached DCHECK in the equivalent situation? Because the caller makes sure that ordering in right. Token manipulations can fail, so we can stay in StateDetached. The reason we don't retry if any of the token operations fail is that it is very likely to be a permanent failure, so there is not point to keep trying. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... File remoting/host/wts_session_process_launcher_win.h (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:12: #include "base/memory/scoped_ptr.h" On 2012/02/28 00:22:07, Wez wrote: > nit: This include goes after compiler_specific.h Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:38: // class. |host_binary| is the host executable name to be launched in On 2012/02/28 00:22:07, Wez wrote: > nit: ... is the name of the executable to be launched ... Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:39: // the session(s) attached to the console. On 2012/02/28 00:22:07, Wez wrote: > nit: the console session. > > There can't be >1 console session, i.e. no plurals, AFAIK. Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:68: int launch_attempts_; launch_attempts_ is not used BTW. I removed it. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:76: // A handle of the process injected into the watched session. On 2012/02/28 00:22:07, Wez wrote: > nit: A -> The Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:82: // A token to be used to launch a process in a different session. On 2012/02/28 00:22:07, Wez wrote: > nit: A -> The Done. > This is the token with the console session-Id set in it, ready for launch, > right? Correct. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:85: // Defines the states the process launcher can be. On 2012/02/28 00:22:07, Wez wrote: > nit: ... can be in. Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:95: // This time is used to delay next attempt to launch the host. On 2012/02/28 00:22:07, Wez wrote: > nit: suggest "Timer used to schedule the next attempt to launch the process." Done. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:96: base::OneShotTimer<WtsSessionProcessLauncher> timer_; On 2012/02/28 00:22:07, Wez wrote: > Why is this timer down here when the |launch_time_| and |launch_backoff_| appear > earlier in the file? The members are sorted alphabetically. It makes sense to group related members together.
scoped_ptr<ScopedHandle> -> ScopedHandle
https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... File remoting/host/host_service_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:194: << " is required."; On 2012/02/28 01:14:04, alexeypa wrote: > On 2012/02/28 00:22:07, Wez wrote: > > nit: Indentation. > > This is how C++ style guide recommends to indent '<<'. We use same indentation > in other places too. Yes, but LOG() is mis-indented. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... File remoting/host/wts_session_process_launcher_win.h (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:96: base::OneShotTimer<WtsSessionProcessLauncher> timer_; On 2012/02/28 01:14:04, alexeypa wrote: > On 2012/02/28 00:22:07, Wez wrote: > > Why is this timer down here when the |launch_time_| and |launch_backoff_| > appear > > earlier in the file? > > The members are sorted alphabetically. It makes sense to group related members > together. I'd group them logically and sort alphabetically only to provide ordering within groupings, ideally. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... File remoting/host/wts_session_process_launcher_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.cc:65: if (!CopyProcessToken(desired_access, &privileged_token)) { I'm [not all that] surprised that the compiler lets you get away with this, but it really shouldn't. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.cc:147: CloseHandle(process_info.hThread); nit: Various places in the codebase use ::CloseHandle(); I'm afraid I'm not sure what the recommended form is. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... File remoting/host/wts_session_process_launcher_win.h (right): https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:38: // the session attached to the console. nit: the console session. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:52: // Attempts to launch the host process in the currently attached session. nit: current console session? https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:75: // The handle of the process injected into the watched session. nit: watched -> console? https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:78: // Monitors the launched process calling a callback when it terminates. nit: "Used to determine when the launched process terminates"
PTAL. https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... File remoting/host/host_service_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/host_serv... remoting/host/host_service_win.cc:194: << " is required."; On 2012/02/28 22:55:36, Wez wrote: > Yes, but LOG() is mis-indented. Oh, I see. My autopilot fixed it, apparently. :-) https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... File remoting/host/wts_session_process_launcher_win.h (right): https://chromiumcodereview.appspot.com/9476017/diff/1/remoting/host/wts_sessi... remoting/host/wts_session_process_launcher_win.h:96: base::OneShotTimer<WtsSessionProcessLauncher> timer_; On 2012/02/28 22:55:36, Wez wrote: > I'd group them logically and sort alphabetically only to provide ordering within > groupings, ideally. Make sense. Coincidentally they are grouped and sorted already. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... File remoting/host/wts_session_process_launcher_win.cc (right): https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.cc:65: if (!CopyProcessToken(desired_access, &privileged_token)) { On 2012/02/28 22:55:36, Wez wrote: > I'm [not all that] surprised that the compiler lets you get away with this, but > it really shouldn't. CopyProcessToken takes a pointer to the wrapper class, so it is OK. :-) https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.cc:147: CloseHandle(process_info.hThread); I can see that both variants are used in Chrome's code. The style guide does not mandate anything. I think :: would be too verbose in Windows-only code. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... File remoting/host/wts_session_process_launcher_win.h (right): https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:38: // the session attached to the console. On 2012/02/28 22:55:36, Wez wrote: > nit: the console session. Done. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:52: // Attempts to launch the host process in the currently attached session. On 2012/02/28 22:55:36, Wez wrote: > nit: current console session? Done. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:75: // The handle of the process injected into the watched session. On 2012/02/28 22:55:36, Wez wrote: > nit: watched -> console? Done. https://chromiumcodereview.appspot.com/9476017/diff/7001/remoting/host/wts_se... remoting/host/wts_session_process_launcher_win.h:78: // Monitors the launched process calling a callback when it terminates. On 2012/02/28 22:55:36, Wez wrote: > nit: "Used to determine when the launched process terminates" Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9476017/14001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9476017/14001
Change committed as 124390 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
