Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10855249:
[Chromoting] The daemon process now starts the networking process and passes the host configuration… (Closed)
|
Created:
8 years, 4 months ago by alexeypa (please no reviews) Modified:
8 years, 3 months ago 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. |
Description[Chromoting] The daemon process now starts the networking process and passes the host configuration (and updates) over the IPC channel.
This CL also switches to FilePathWatcher (to detect the configuration file changes) on all platforms.
BUG=123696, 134694
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154999
Patch Set 1 #
Total comments: 63
Patch Set 2 : CR feedback and rebased on top of https://chromiumcodereview.appspot.com/10829467/ #
Total comments: 20
Patch Set 3 : CR feedback + rebased on top of https://chromiumcodereview.appspot.com/10829467/ (Patch Set 5) #
Total comments: 51
Patch Set 4 : CR feedback. #Patch Set 5 : CR feedback and made ConfigFileWatcher compatible with scoped_ptr" #Patch Set 6 : Restore sighup_listener.h/.cc since they are going to be reused by Jamie's CL and ignore SIGHUP for… #
Total comments: 2
Patch Set 7 : CR feedback. #Patch Set 8 : rebased on top of master. #Patch Set 9 : Added missing signal.h #Messages
Total messages: 20 (0 generated)
PTAL. Please ping me if you are swamped with other reviews.
The REMOTING_MULTI_PROCESS ifdefs make a lot of this code more complex than it needs to be; if we need to make architectural changes to be able to cleanly support multi-process and single-process in the same codebase then let's make them! https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/base/stoppabl... File remoting/base/stoppable.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/base/stoppabl... remoting/base/stoppable.cc:28: base::Unretained(this))); Using base::Unretained here means that multiple Stop() calls may result in the object being used after being freed, surely? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:20: // configuration file name. nit: Suggest "The name of the command-line switch used to specify the host configuration file to use." https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:42: DCHECK(main_task_runner()->BelongsToCurrentThread()); nit: You don't need this check if all you're going to do is return false. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:47: DaemonProcess::DaemonProcess( nit: Although this ordering matches that in the class definition, I think it's clearer to put the ctor immediately before the dtor in the implementation. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:58: base::Bind(&DaemonProcess::Initialize, base::Unretained(this))); Is there any stronger guarantee that DaemonProcess still exists when this task is processed than that we have to ping-pong a message off the IO thread before we becomes "stopped"? That seems a little fragile to me. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:67: nit: No need for this blank line. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:76: base::Bind(&DaemonProcess::StartConfigWatcher, base::Unretained(this))); Similarly, what is the guarantee that DaemonProcess still exist when this task is processed? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:90: } I think you need to clear the reload timer here, otherwise the timer may go off after the stopped task has been posted, and cause a new task to be posted to this object that won't be processed until the stopped task has deleted it? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:137: // Create the timer that will be used for dealyed-reading the configuration typo: delayed https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:48: // unless the network process is connected over the IPC channel. Is this comment correct? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:63: virtual void LaunchNetworkProcess() = 0; nit: I think this should be public, according to the style guide, or better still a method on a delegate interface, rather than having callers derived from DaemonProcess. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:38: // line to the host process. nit: host process -> network process? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:42: // The security descriptor of the Chromoting IPC channel. It gives full access nit: Chromoting -> daemon, or just remove qualifier? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:61: // WorkerProcessDelegate implementation. nit: WorkerProcessDelegate is inherited via DaemonProcess, so it's a little confusing to see you refer to it directly here. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:79: // Called when the launcher reports the process to be stopped. nit: ... reports the worker process has stopped. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:82: // True if the network process is connected. nit: Connected to the Daemon, connected to Talk, or has at least one client connected, or something else? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:112: // the object before destruction. But you're not examining any state; aren't you really sanity-checking before teardown of the object's members? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:168: // See http://crbug.com/134694. nit: It's not clear what this comment means, even in the context of the bug; I think you really want a comment on the LaunchProcessWithToken() call saying that we should create a restricted token to launch under, rather than here, where we have no control over the token we'll get back from OpenProcessToken(). https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:173: LOG_GETLASTERROR(ERROR) << "Failed to open process token"; Is there ever a situation in which this can happen which isn't essentially fatal? i.e. should we just CHECK here? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:217: launch_backoff_ = base::TimeDelta(); Why zero the backoff here? Isn't it useful to let the backoff also apply even if calling code is misbehaving & starting/stopping us rapidly? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:224: // Wait for |launcher_| to be completely stopped. nit: Suggest: "Early exit if we're still waiting for |launcher_| to stop." https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:236: if (network_process_.IsValid()) { Should it be possible for |network_process_| not to be valid here? If not then CHECK it? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:59: // Creates the launcher that outsources specific of launching the worker to nit: I think you mean you're creating a launcher that will use the supplied delegate to actually manage the worker process, and the supplied WorkerProcessDelegate to handle IPCs? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, Why do we not just use IPC::Listener here?
Here are some answers to your questions. This CL needs more work. I'll make the changes and post an update that will also address the rest of the comments. > The REMOTING_MULTI_PROCESS ifdefs make a lot of this code more complex than it > needs to be; if we need to make architectural changes to be able to cleanly > support multi-process and single-process in the same codebase then let's make > them! Will do. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/base/stoppabl... File remoting/base/stoppable.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/base/stoppabl... remoting/base/stoppable.cc:28: base::Unretained(this))); On 2012/08/21 00:18:57, Wez wrote: > Using base::Unretained here means that multiple Stop() calls may result in the > object being used after being freed, surely? Indeed, your are right. Stop() should not be ever called from a different thread. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:47: DaemonProcess::DaemonProcess( On 2012/08/21 00:18:57, Wez wrote: > nit: Although this ordering matches that in the class definition, I think it's > clearer to put the ctor immediately before the dtor in the implementation. There was a long thread in chrome-dev about this and the outcome was that the order should match. At least majority of folks thought this is a better practice. So I'll stick to matching order in .h and .cc even if I personally think that grouping constructors and destructors in .cc makes more sense. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:48: // unless the network process is connected over the IPC channel. On 2012/08/21 00:18:57, Wez wrote: > Is this comment correct? It appears correct to me. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:112: // the object before destruction. On 2012/08/21 00:18:57, Wez wrote: > But you're not examining any state; aren't you really sanity-checking before > teardown of the object's members? In this case "us" are human beings equipped with a debugger. :-) https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:217: launch_backoff_ = base::TimeDelta(); On 2012/08/21 00:18:57, Wez wrote: > Why zero the backoff here? Isn't it useful to let the backoff also apply even if > calling code is misbehaving & starting/stopping us rapidly? It will not start starting/stopping rapidly because at this time the object is being torn down. I.e. not more processes will be started. Zeroing |launch_backoff_| is not really necessary here as well. It seems to be a remnant of old code I took from WtsSessionProcessLauncher. I'll remove it. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, On 2012/08/21 00:18:57, Wez wrote: > Why do we not just use IPC::Listener here? Because IPC::Listener is a different interface. IPC::Listener has more methods that we don't use or support.
OK, here is what came up after addressing the feedback. It has been rebased on top of "auto threads" change. I'm sorry. I had to. This CL removes SIGHUP handling from Mac and Linux completely relying on file watcher. Lambros, could you please take a look? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:20: // configuration file name. On 2012/08/21 00:18:57, Wez wrote: > nit: Suggest "The name of the command-line switch used to specify the host > configuration file to use." Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:42: DCHECK(main_task_runner()->BelongsToCurrentThread()); On 2012/08/21 00:18:57, Wez wrote: > nit: You don't need this check if all you're going to do is return false. I'll add useful code to this method in the nearest future. Meanwhile this DCHECK is a documentation describing thread affinity of the method. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:58: base::Bind(&DaemonProcess::Initialize, base::Unretained(this))); On 2012/08/21 00:18:57, Wez wrote: > Is there any stronger guarantee that DaemonProcess still exists when this task > is processed than that we have to ping-pong a message off the IO thread before > we becomes "stopped"? That seems a little fragile to me. Making the object ref-countable (or passing scoped_ptr reference to it) merely guarantees that posted tasks will run while the object is alive. It does not guarantee that the object will be deleted only after it was properly shutdown because the owner can still release the last reference too soon. This means that CHECK in the destructor validating that the object is properly stopped is needed in either case. This CHECK in the destructor appears to be a strong enough guarantee that posting to base::Unretained(this) is safe. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:67: On 2012/08/21 00:18:57, Wez wrote: > nit: No need for this blank line. Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:76: base::Bind(&DaemonProcess::StartConfigWatcher, base::Unretained(this))); On 2012/08/21 00:18:57, Wez wrote: > Similarly, what is the guarantee that DaemonProcess still exist when this task > is processed? CHECK in the destructor seems to be it. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:90: } On 2012/08/21 00:18:57, Wez wrote: > I think you need to clear the reload timer here, otherwise the timer may go off > after the stopped task has been posted, and cause a new task to be posted to > this object that won't be processed until the stopped task has deleted it? It is done in DaemonProcess::StopConfigWatcher. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:137: // Create the timer that will be used for dealyed-reading the configuration On 2012/08/21 00:18:57, Wez wrote: > typo: delayed Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:63: virtual void LaunchNetworkProcess() = 0; On 2012/08/21 00:18:57, Wez wrote: > nit: I think this should be public, according to the style guide, The style guide said that inheritance should be public but it says nothing whether inherited methods (including virtual once) should be public. > or better > still a method on a delegate interface, rather than having callers derived from > DaemonProcess. I could probably split DaemonProcess<Os> and DaemonProcess into independent classes. I think it will complicate shutdown without bringing much value. It does not seem we are going to take advantage from the separation in the nearest future. Factoring out the configuration watcher on the other hand should help to define the shutdown sequence better. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:38: // line to the host process. On 2012/08/21 00:18:57, Wez wrote: > nit: host process -> network process? Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:42: // The security descriptor of the Chromoting IPC channel. It gives full access On 2012/08/21 00:18:57, Wez wrote: > nit: Chromoting -> daemon, or just remove qualifier? Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:61: // WorkerProcessDelegate implementation. On 2012/08/21 00:18:57, Wez wrote: > nit: WorkerProcessDelegate is inherited via DaemonProcess, so it's a little > confusing to see you refer to it directly here. Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:79: // Called when the launcher reports the process to be stopped. On 2012/08/21 00:18:57, Wez wrote: > nit: ... reports the worker process has stopped. Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:82: // True if the network process is connected. On 2012/08/21 00:18:57, Wez wrote: > nit: Connected to the Daemon, connected to Talk, or has at least one client > connected, or something else? Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:168: // See http://crbug.com/134694. On 2012/08/21 00:18:57, Wez wrote: > nit: It's not clear what this comment means, even in the context of the bug; I > think you really want a comment on the LaunchProcessWithToken() call saying that > we should create a restricted token to launch under, rather than here, where we > have no control over the token we'll get back from OpenProcessToken(). Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:173: LOG_GETLASTERROR(ERROR) << "Failed to open process token"; On 2012/08/21 00:18:57, Wez wrote: > Is there ever a situation in which this can happen which isn't essentially > fatal? i.e. should we just CHECK here? Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:224: // Wait for |launcher_| to be completely stopped. On 2012/08/21 00:18:57, Wez wrote: > nit: Suggest: "Early exit if we're still waiting for |launcher_| to stop." Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:236: if (network_process_.IsValid()) { On 2012/08/21 00:18:57, Wez wrote: > Should it be possible for |network_process_| not to be valid here? If not then > CHECK it? Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:59: // Creates the launcher that outsources specific of launching the worker to On 2012/08/21 00:18:57, Wez wrote: > nit: I think you mean you're creating a launcher that will use the supplied > delegate to actually manage the worker process, and the supplied > WorkerProcessDelegate to handle IPCs? Done.
> This CL removes SIGHUP handling from Mac and Linux completely relying on file > watcher. Can you add a link to bug 123696, and change its status to Fixed once this has landed?
sighup removal LGTM.
https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:48: // unless the network process is connected over the IPC channel. On 2012/08/21 17:16:36, alexeypa wrote: > On 2012/08/21 00:18:57, Wez wrote: > > Is this comment correct? > > It appears correct to me. Why is the message to the worker process dropped if the network process is not connected? https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:112: // the object before destruction. On 2012/08/21 17:16:36, alexeypa wrote: > On 2012/08/21 00:18:57, Wez wrote: > > But you're not examining any state; aren't you really sanity-checking before > > teardown of the object's members? > > In this case "us" are human beings equipped with a debugger. :-) OK, so the comment is just misleading. ;) It's implicit in there being a CHECK that the aim is to trap a bad condition at a point where a human can step in and see what's wrong in a debugger; the point of putting the CHECK here is to trap *before* we potentially crashing while tearing stuff down. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, On 2012/08/21 17:16:36, alexeypa wrote: > On 2012/08/21 00:18:57, Wez wrote: > > Why do we not just use IPC::Listener here? > > Because IPC::Listener is a different interface. IPC::Listener has more methods > that we don't use or support. IPC::Listener just has OnMessageReceived, OnChannelConnected and OnChannelError (cross-platform); the only one you don't use is OnChannelError, and in any case the interface is designed such that implementations need not override calls they don't care about. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... File remoting/host/daemon_config_watcher.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.cc:90: // this delay may not be necessary. If we fixed our config-writing code to write & rename, or fixed the OnConfigUpdated handling to ignore incomplete configs, then we could get rid of this hack this class would become a simple wrapper to run FilePathWatcher on the IO thread? https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... File remoting/host/daemon_config_watcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.h:30: class DaemonConfigWatcher { I realise that we're only going to use this for reading the host configuration in the Daemon, but this class just watches a file and dispatches a callback every time it changes, with the file's contents; rename it ConfigFileReader? https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.h:45: // thread but posts config file updates on on |main_task_runner|. nit: Clarify that this means |delegate| must be created & destroyed on |main_task_runner| https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.h:57: void Stop(scoped_ptr<DaemonConfigWatcher> self); Why does the caller need to pass in a scoped_ptr to the Watcher? https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_process.cc:86: DaemonConfigWatcher* config_watcher = config_watcher_.get(); Why do you need this? https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/me2me... File remoting/host/me2me_preference_pane.mm (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/me2me... remoting/host/me2me_preference_pane.mm:526: // Start the service. nit: "Ensure the service is started". https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:160: kInvalidHostConfigurationExitCode)); nit: Just call OnConfigWatcherError()? https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:169: kInvalidHostConfigurationExitCode)); nit: Just invoke OnConfigWatcherError()? https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:243: kInvalidHostConfigurationExitCode)); Feels like this thread-jump should be coded into Shutdown() itself, so that OnConfigWatcherError() and pals don't need this boilerplate? https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:708: base::Unretained(&me2me_host))); Why do you need to post a task for this, rather than calling StartHostProcess directly, since you're already on the right thread?
https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:48: // unless the network process is connected over the IPC channel. On 2012/08/24 22:20:37, Wez wrote: > Why is the message to the worker process dropped if the network process is not > connected? We don't want to send a message that will become outdated when the child process receives it. For instance: 1. A configuration update is sent with empty configuration. 2. The actual configuration is read and another message is sent. 2. The network process connects and receives two configuration update messages: an empty and the real one. The messages are noty ignored once the child process is connected and we get a callback when it connects. Once the channel is broken the child process will be restarted. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:112: // the object before destruction. On 2012/08/24 22:20:37, Wez wrote: > On 2012/08/21 17:16:36, alexeypa wrote: > > On 2012/08/21 00:18:57, Wez wrote: > > > But you're not examining any state; aren't you really sanity-checking before > > > teardown of the object's members? > > > > In this case "us" are human beings equipped with a debugger. :-) > > OK, so the comment is just misleading. ;) It's implicit in there being a CHECK > that the aim is to trap a bad condition at a point where a human can step in and > see what's wrong in a debugger; the point of putting the CHECK here is to trap > *before* we potentially crashing while tearing stuff down. OK, let try another version. In the cases like this it would help greatly help if you suggest alternative wording at once. This is forth or so iteration of this comment (and it went thought one another review already), which surely means that I've run out of ideas already. :P https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, On 2012/08/24 22:20:37, Wez wrote: > On 2012/08/21 17:16:36, alexeypa wrote: > > On 2012/08/21 00:18:57, Wez wrote: > > > Why do we not just use IPC::Listener here? > > > > Because IPC::Listener is a different interface. IPC::Listener has more methods > > that we don't use or support. > > IPC::Listener just has OnMessageReceived, OnChannelConnected and OnChannelError > (cross-platform); the only one you don't use is OnChannelError, and in any case > the interface is designed such that implementations need not override calls they > don't care about. Yes, IPC::Listener is very similar but different interface. There are no consumers expecting IPC::Listener, so using IPC::Listener has cost but no advantages. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... File remoting/host/daemon_config_watcher.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.cc:90: // this delay may not be necessary. On 2012/08/24 22:20:38, Wez wrote: > If we fixed our config-writing code to write & rename, or fixed the > OnConfigUpdated handling to ignore incomplete configs, then we could get rid of > this hack this class would become a simple wrapper to run FilePathWatcher on the > IO thread? More or less. I'd keep the delay regardless of the actual implementation of the writer because this one allows us to handle both atomic and non/atomic writers (to some extent). https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... File remoting/host/daemon_config_watcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.h:30: class DaemonConfigWatcher { On 2012/08/24 22:20:38, Wez wrote: > I realise that we're only going to use this for reading the host configuration > in the Daemon, but this class just watches a file and dispatches a callback > every time it changes, with the file's contents; rename it ConfigFileReader? Done. ConfigFileWatcher. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.h:45: // thread but posts config file updates on on |main_task_runner|. On 2012/08/24 22:20:38, Wez wrote: > nit: Clarify that this means |delegate| must be created & destroyed on > |main_task_runner| It does not mean that. |delegate| can be created and destroyed on any thread as long as it can receive calls in |main_task_runner| for as long as |DaemonConfigWatcher| is alive. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_config_watcher.h:57: void Stop(scoped_ptr<DaemonConfigWatcher> self); On 2012/08/24 22:20:38, Wez wrote: > Why does the caller need to pass in a scoped_ptr to the Watcher? It is needed to delete the object once the last task completes. It does not look nice so I renamed it to StopAdnDelete(). https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/daemo... remoting/host/daemon_process.cc:86: DaemonConfigWatcher* config_watcher = config_watcher_.get(); On 2012/08/24 22:20:38, Wez wrote: > Why do you need this? I wasn't sure that -> will be evaluated before config_watcher_.Pass(). This is not needed any more. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/me2me... File remoting/host/me2me_preference_pane.mm (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/me2me... remoting/host/me2me_preference_pane.mm:526: // Start the service. On 2012/08/24 22:20:38, Wez wrote: > nit: "Ensure the service is started". Done. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:160: kInvalidHostConfigurationExitCode)); On 2012/08/24 22:20:38, Wez wrote: > nit: Just call OnConfigWatcherError()? Done. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:169: kInvalidHostConfigurationExitCode)); On 2012/08/24 22:20:38, Wez wrote: > nit: Just invoke OnConfigWatcherError()? Done. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:243: kInvalidHostConfigurationExitCode)); On 2012/08/24 22:20:38, Wez wrote: > Feels like this thread-jump should be coded into Shutdown() itself, so that > OnConfigWatcherError() and pals don't need this boilerplate? Maybe, but there is a whole bunch of other assumptions about Shutdown() exists in the code elsewhere, so, I think, it should not be a part of this CL. https://chromiumcodereview.appspot.com/10855249/diff/8001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:708: base::Unretained(&me2me_host))); On 2012/08/24 22:20:38, Wez wrote: > Why do you need to post a task for this, rather than calling StartHostProcess > directly, since you're already on the right thread? Done.
Re: removing the SIGHUP handler: Ideally, this should be in a separate CL, but I appreciate that that might not be feasible at this stage. If it remains as part of this CL, then most (all?) of the SIGHUP handling in me2me_virtual_host.py needs to be removed as well, otherwise the host process will probably be killed when it receives SIGHUP due to the --reload command-line option (unfortunately, we still need the --reload option due to a forthcoming change that copies the config file to a temporary location).
On 2012/08/29 17:10:46, Jamie wrote: > Re: removing the SIGHUP handler: Ideally, this should be in a separate CL, but I > appreciate that that might not be feasible at this stage. If it remains as part > of this CL, then most (all?) of the SIGHUP handling in me2me_virtual_host.py > needs to be removed as well, otherwise the host process will probably be killed > when it receives SIGHUP due to the --reload command-line option (unfortunately, > we still need the --reload option due to a forthcoming change that copies the > config file to a temporary location). Alternatively we can just fix the host to ignore SIGHUP, to avoid being killed. It's just one line that should be added in the host: signal(SIGHUP, SIG_IGN);
On 2012/08/29 20:19:15, sergeyu wrote: > On 2012/08/29 17:10:46, Jamie wrote: > > Re: removing the SIGHUP handler: Ideally, this should be in a separate CL, but > I > > appreciate that that might not be feasible at this stage. If it remains as > part > > of this CL, then most (all?) of the SIGHUP handling in me2me_virtual_host.py > > needs to be removed as well, otherwise the host process will probably be > killed > > when it receives SIGHUP due to the --reload command-line option > (unfortunately, > > we still need the --reload option due to a forthcoming change that copies the > > config file to a temporary location). > > Alternatively we can just fix the host to ignore SIGHUP, to avoid being killed. > It's just one line that should be added in the host: > signal(SIGHUP, SIG_IGN); That would be fine as a way of avoiding bloat in this CL, but we should should remove the SIGHUP logic from the script shortly afterwards.
https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:58: base::Bind(&DaemonProcess::Initialize, base::Unretained(this))); On 2012/08/23 23:53:01, alexeypa wrote: > On 2012/08/21 00:18:57, Wez wrote: > > Is there any stronger guarantee that DaemonProcess still exists when this task > > is processed than that we have to ping-pong a message off the IO thread before > > we becomes "stopped"? That seems a little fragile to me. > > Making the object ref-countable (or passing scoped_ptr reference to it) merely > guarantees that posted tasks will run while the object is alive. It does not > guarantee that the object will be deleted only after it was properly shutdown > because the owner can still release the last reference too soon. This means that > CHECK in the destructor validating that the object is properly stopped is needed > in either case. > > This CHECK in the destructor appears to be a strong enough guarantee that > posting to base::Unretained(this) is safe. For a follow-up CL, I wonder if we could add a helper to Stoppable to get a base::Unretained(this), to use in place of scary-looking Unretaineds, so I don't have to keep asking this question? :P https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:48: // unless the network process is connected over the IPC channel. On 2012/08/27 23:16:41, alexeypa wrote: > On 2012/08/24 22:20:37, Wez wrote: > > Why is the message to the worker process dropped if the network process is not > > connected? > > We don't want to send a message that will become outdated when the child process > receives it. For instance: > > 1. A configuration update is sent with empty configuration. > 2. The actual configuration is read and another message is sent. > 2. The network process connects and receives two configuration update messages: > an empty and the real one. > > The messages are noty ignored once the child process is connected and we get a > callback when it connects. Once the channel is broken the child process will be > restarted. The point I'm making is that the wording sounds like the "worker" and "network" processes are different, so it's not clear why the absence of one should affect message-posting to the other. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process_win.cc:112: // the object before destruction. On 2012/08/27 23:16:41, alexeypa wrote: > On 2012/08/24 22:20:37, Wez wrote: > > On 2012/08/21 17:16:36, alexeypa wrote: > > > On 2012/08/21 00:18:57, Wez wrote: > > > > But you're not examining any state; aren't you really sanity-checking > before > > > > teardown of the object's members? > > > > > > In this case "us" are human beings equipped with a debugger. :-) > > > > OK, so the comment is just misleading. ;) It's implicit in there being a CHECK > > that the aim is to trap a bad condition at a point where a human can step in > and > > see what's wrong in a debugger; the point of putting the CHECK here is to trap > > *before* we potentially crashing while tearing stuff down. > > OK, let try another version. > > In the cases like this it would help greatly help if you suggest alternative > wording at once. This is forth or so iteration of this comment (and it went > thought one another review already), which surely means that I've run out of > ideas already. :P You're right - sorry, I'll try to do that in future. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, On 2012/08/27 23:16:41, alexeypa wrote: > On 2012/08/24 22:20:37, Wez wrote: > > On 2012/08/21 17:16:36, alexeypa wrote: > > > On 2012/08/21 00:18:57, Wez wrote: > > > > Why do we not just use IPC::Listener here? > > > > > > Because IPC::Listener is a different interface. IPC::Listener has more > methods > > > that we don't use or support. > > > > IPC::Listener just has OnMessageReceived, OnChannelConnected and > OnChannelError > > (cross-platform); the only one you don't use is OnChannelError, and in any > case > > the interface is designed such that implementations need not override calls > they > > don't care about. > > Yes, IPC::Listener is very similar but different interface. There are no > consumers expecting IPC::Listener, so using IPC::Listener has cost but no > advantages. The advantage is simply that we don't define a new interface that just provides a subset of the functionality of an existing one. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... File remoting/host/config_file_watcher.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.cc:40: base::Unretained(this), nit: Indentation; either align w/ base::Bind bracket, or indent four spaces from io_task_runner_ https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.cc:53: &ConfigFileWatcher::ReloadConfig)); nit: Could you use a raw timer here, and base::Bind() it to ReloadConfig along with the FilePath, to avoid the need for |config_path_|? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... File remoting/host/config_file_watcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:34: virtual ~Delegate(); nit: Should this be protected, so that only implementations can delete themselves? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:49: const base::WeakPtr<Delegate>& delegate); nit: You're forcing the caller to arrange to provide a WeakPtr<Delegate>; you can avoid that requirement by instead having the ConfigFileWatcher manage the WeakPtrFactory<Delegate>, and requiring that the caller call StopAndDelete() before tearing down the Delegate so that you can invalidate the WeakPtr. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:57: void StopAndDelete(); Should the destructor be public if it's intended that deletion be performed by StopAndDelete()? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:70: // Monitors the host configuration file changes. nit: Remove "changes", or add "for" before it. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process.cc:73: io_task_runner(), nit: Indentation. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process.h:78: void ConfigUpdated(const FilePath& path, bool error); You don't need this, or the next few methods, I don't think? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process_win.cc:111: // predictably. Perfect! :D https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process_win.cc:157: // Create the host process command line passing the name of the IPC channel nit: ... command line passing ... -> ... command line, passing ... https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process_win.cc:215: launch_backoff_ = base::TimeDelta(); Didn't we agree this wasn't necessary? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process_win.cc:238: exit_code = CONTROL_C_EXIT; nit: Doesn't GetExitCodeProcess() guarantee not to touch |exit_code| if it fails? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:120: base::Unretained(this))) nit: Missing a comma here? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:154: virtual void OnConfigUpdated(const std::string& config) OVERRIDE { nit: config -> config_str https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:202: context_->file_task_runner(), nit: Indentation. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:248: weak_factory_.InvalidateWeakPtrs(); nit: Add a short comment to explain why we're doing this. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:701: remoting::HostProcess me2me_host(context.Pass()); The HostProcess is created after |context|, so the ChromotingHostContext will out-live it; what guarantee do we have that PostTask(...Unretained(this)) tasks won't be processed against the HostProcess after it's been deleted, before the context threads are torn down? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... remoting/host/win/worker_process_launcher.h:44: class Delegate { Delegate -> LauncherDelegate; otherwise it's not clear why we have both Delegate and WorkerDelegate. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/work... File remoting/host/worker_process_delegate.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/work... remoting/host/worker_process_delegate.h:18: // channel. From the caller's perspective the worker process is represented by the WorkerProcessLauncher through which it launches & sends messages to it; this interfaces actually represents the _parent_ process to the IPC layer. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/work... remoting/host/worker_process_delegate.h:19: class WorkerProcessDelegate { WorkerProcessDelegate -> WorkerIpcDelegate, or just use the existing IPC delegate interface IPC::Listener. ;)
https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:58: base::Bind(&DaemonProcess::Initialize, base::Unretained(this))); On 2012/08/30 20:36:04, Wez wrote: > For a follow-up CL, I wonder if we could add a helper to Stoppable to get a > base::Unretained(this), to use in place of scary-looking Unretaineds, so I don't > have to keep asking this question? :P Maybe. I have a whole bunch of other important stuff in my to do list, so please feel free to jump in and send me a review. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.h:48: // unless the network process is connected over the IPC channel. On 2012/08/30 20:36:04, Wez wrote: > The point I'm making is that the wording sounds like the "worker" and "network" > processes are different, so it's not clear why the absence of one should affect > message-posting to the other. This was a very obscure way of asking this question. Done. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, On 2012/08/30 20:36:04, Wez wrote: > The advantage is simply that we don't define a new interface that just provides > a subset of the functionality of an existing one. It provides different functionality, that is not a subset of IPC::Listener. OnChannelConnected() for example does not have client's PID. This is done on purpose. There is no OnChannelError(). That is because the error signalling is different, so supporting OnChannelError() on either side will be wasted code. There are also OnChannelDenied() and OnChannelDenied() on POSIX (WorkedProcessDelegate is cross-platform) that we don't need here. Imagine it would be IPC::Listener. You would be the first one who asked me to document all of those differences. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... File remoting/host/config_file_watcher.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.cc:40: base::Unretained(this), On 2012/08/30 20:36:04, Wez wrote: > nit: Indentation; either align w/ base::Bind bracket, or indent four spaces from > io_task_runner_ Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.cc:53: &ConfigFileWatcher::ReloadConfig)); On 2012/08/30 20:36:04, Wez wrote: > nit: Could you use a raw timer here, and base::Bind() it to ReloadConfig along > with the FilePath, to avoid the need for |config_path_|? |config_path_| is needed because of Linux we get notifications for the whole directory. It is documented in FilePathWatcher. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... File remoting/host/config_file_watcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:34: virtual ~Delegate(); On 2012/08/30 20:36:04, Wez wrote: > nit: Should this be protected, so that only implementations can delete > themselves? None of delegate declarations use protected destructors. I'll stick to defacto standard. PS. I actually think this might have some unexpected implications. Other interfaces also define public destructors. The style guide also does that. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:49: const base::WeakPtr<Delegate>& delegate); On 2012/08/30 20:36:04, Wez wrote: > nit: You're forcing the caller to arrange to provide a WeakPtr<Delegate>; you > can avoid that requirement by instead having the ConfigFileWatcher manage the > WeakPtrFactory<Delegate>, and requiring that the caller call StopAndDelete() > before tearing down the Delegate so that you can invalidate the WeakPtr. Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:57: void StopAndDelete(); On 2012/08/30 20:36:04, Wez wrote: > Should the destructor be public if it's intended that deletion be performed by > StopAndDelete()? scoped_ptr<ConfigFileWatcher> will not compile if the destructor is private. I Added DCHECK to the destructor to validate that the object has shutdown correctly. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:70: // Monitors the host configuration file changes. On 2012/08/30 20:36:04, Wez wrote: > nit: Remove "changes", or add "for" before it. Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process.cc:73: io_task_runner(), On 2012/08/30 20:36:04, Wez wrote: > nit: Indentation. Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... File remoting/host/daemon_process.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process.h:78: void ConfigUpdated(const FilePath& path, bool error); On 2012/08/30 20:36:04, Wez wrote: > You don't need this, or the next few methods, I don't think? Yes. I forgot to remove them. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... File remoting/host/daemon_process_win.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process_win.cc:157: // Create the host process command line passing the name of the IPC channel On 2012/08/30 20:36:04, Wez wrote: > nit: ... command line passing ... -> ... command line, passing ... Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process_win.cc:215: launch_backoff_ = base::TimeDelta(); On 2012/08/30 20:36:04, Wez wrote: > Didn't we agree this wasn't necessary? Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/daem... remoting/host/daemon_process_win.cc:238: exit_code = CONTROL_C_EXIT; On 2012/08/30 20:36:04, Wez wrote: > nit: Doesn't GetExitCodeProcess() guarantee not to touch |exit_code| if it > fails? No. Very few functions provide such a guarantee. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:120: base::Unretained(this))) On 2012/08/30 20:36:04, Wez wrote: > nit: Missing a comma here? Yes, but not anymore. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:154: virtual void OnConfigUpdated(const std::string& config) OVERRIDE { On 2012/08/30 20:36:04, Wez wrote: > nit: config -> config_str The style guide does not allow Hungarian notation and alike. I agree with it! https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:202: context_->file_task_runner(), On 2012/08/30 20:36:04, Wez wrote: > nit: Indentation. Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:248: weak_factory_.InvalidateWeakPtrs(); On 2012/08/30 20:36:04, Wez wrote: > nit: Add a short comment to explain why we're doing this. Not needed anymore. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:701: remoting::HostProcess me2me_host(context.Pass()); On 2012/08/30 20:36:04, Wez wrote: > The HostProcess is created after |context|, so the ChromotingHostContext will > out-live it; The ownership is transferred to HostProcess. > what guarantee do we have that PostTask(...Unretained(this)) tasks > won't be processed against the HostProcess after it's been deleted, before the > context threads are torn down? It is not going to outlive HostProcess. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... remoting/host/win/worker_process_launcher.h:44: class Delegate { On 2012/08/30 20:36:04, Wez wrote: > Delegate -> LauncherDelegate; otherwise it's not clear why we have both Delegate > and WorkerDelegate. 1. This is a common pattern in the existing code. 2. Delegate is in fact WorkerProcessLauncher::Delegate with is fairly different from WorkerProcessDelegate. 3. Having to words "Launcher" in WorkerProcessLauncher::LauncherDelegate is ugly. 4. Why don't rename base::win::ObjectWatcher::Delegate as well? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/work... File remoting/host/worker_process_delegate.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/work... remoting/host/worker_process_delegate.h:18: // channel. On 2012/08/30 20:36:04, Wez wrote: > From the caller's perspective the worker process is represented by the > WorkerProcessLauncher through which it launches & sends messages to it; this > interfaces actually represents the _parent_ process to the IPC layer. Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/work... remoting/host/worker_process_delegate.h:19: class WorkerProcessDelegate { On 2012/08/30 20:36:04, Wez wrote: > WorkerProcessDelegate -> WorkerIpcDelegate, WorkerProcessIpcDelegate. > or just use the existing IPC delegate interface IPC::Listener. ;) See my other comment.
LGTM once the scoped_ptr<> issue is resolved. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, On 2012/08/30 23:15:13, alexeypa wrote: > On 2012/08/30 20:36:04, Wez wrote: > > The advantage is simply that we don't define a new interface that just > provides > > a subset of the functionality of an existing one. > > It provides different functionality, that is not a subset of IPC::Listener. > OnChannelConnected() for example does not have client's PID. This is done on > purpose. There is no OnChannelError(). That is because the error signalling is > different, so supporting OnChannelError() on either side will be wasted code. > There are also OnChannelDenied() and OnChannelDenied() on POSIX > (WorkedProcessDelegate is cross-platform) that we don't need here. Those are all optional for the IPC::Listener implementation to override, though. > Imagine it would be IPC::Listener. You would be the first one who asked me to > document all of those differences. True, but it would be a one or two line comment in this implementation, rather than a whole new interface. ;) https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... File remoting/host/config_file_watcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:34: virtual ~Delegate(); On 2012/08/30 23:15:13, alexeypa wrote: > On 2012/08/30 20:36:04, Wez wrote: > > nit: Should this be protected, so that only implementations can delete > > themselves? > > None of delegate declarations use protected destructors. I'll stick to defacto > standard. > > PS. I actually think this might have some unexpected implications. Other > interfaces also define public destructors. The style guide also does that. It doesn't seem right, does it? Either way, not a debate for this CL. :) https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:57: void StopAndDelete(); On 2012/08/30 23:15:13, alexeypa wrote: > On 2012/08/30 20:36:04, Wez wrote: > > Should the destructor be public if it's intended that deletion be performed by > > StopAndDelete()? > > scoped_ptr<ConfigFileWatcher> will not compile if the destructor is private. I > Added DCHECK to the destructor to validate that the object has shutdown > correctly. If the calling code isn't at liberty to delete the object then it surely shouldn't be holding it in a scoped_ptr<> at all; presumably we only need that in case the caller is torn down without ever having called Watch()? Given that the caller isn't at liberty to destroy the object once Watch() has been called, you could simply require that they call StopAndDelete() on the object, and have it delete itself in there if Watch() has never been called? Then the dtor can be private and the only risk is that the caller leaks it. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:154: virtual void OnConfigUpdated(const std::string& config) OVERRIDE { On 2012/08/30 23:15:13, alexeypa wrote: > On 2012/08/30 20:36:04, Wez wrote: > > nit: config -> config_str > > The style guide does not allow Hungarian notation and alike. I agree with it! :P I actually mean "str" as a well-known abbreviation for "string", similarly to our use of "config" for "configuration", to make the distinction between the stringified |config| and |config_| more obvious. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:701: remoting::HostProcess me2me_host(context.Pass()); On 2012/08/30 23:15:13, alexeypa wrote: > On 2012/08/30 20:36:04, Wez wrote: > > The HostProcess is created after |context|, so the ChromotingHostContext will > > out-live it; > > The ownership is transferred to HostProcess. Good point! > > what guarantee do we have that PostTask(...Unretained(this)) tasks > > won't be processed against the HostProcess after it's been deleted, before the > > context threads are torn down? > > It is not going to outlive HostProcess. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... remoting/host/win/worker_process_launcher.h:44: class Delegate { On 2012/08/30 23:15:13, alexeypa wrote: > On 2012/08/30 20:36:04, Wez wrote: > > Delegate -> LauncherDelegate; otherwise it's not clear why we have both > Delegate > > and WorkerDelegate. > > 1. This is a common pattern in the existing code. > 2. Delegate is in fact WorkerProcessLauncher::Delegate with is fairly different > from WorkerProcessDelegate. > 3. Having to words "Launcher" in WorkerProcessLauncher::LauncherDelegate is > ugly. > 4. Why don't rename base::win::ObjectWatcher::Delegate as well? base::win::ObjectWatcher doesn't take two *Delegate parameters, though. The issue here is that we have two Delegates, one of which is used for process launch, and one that handles IPC traffic from the launched process, so it's not clear that one deserves to be WorkerProcessLauncher::Delegate while the other be WorkerProcessIpcDelegate.
I moved most of ConfigFileWatcher's functionality to ConfigFileWatcherImpl. ConfigFileWatcher is a proxy that can be deleted synchronously. ConfigFileWatcherImpl is an implementation that deletes itself asynchronously. https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/1/remoting/host/win/work... remoting/host/win/worker_process_launcher.h:70: WorkerProcessDelegate* worker_delegate, On 2012/08/31 00:23:04, Wez wrote: > Those are all optional for the IPC::Listener implementation to override, though. Implemented may mistakenly assume that the optional methods are supported and add code that relies on them. > True, but it would be a one or two line comment in this implementation, rather > than a whole new interface. ;) A comment does not enforce the difference. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... File remoting/host/config_file_watcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/conf... remoting/host/config_file_watcher.h:57: void StopAndDelete(); On 2012/08/31 00:23:04, Wez wrote: > If the calling code isn't at liberty to delete the object then it surely > shouldn't be holding it in a scoped_ptr<> at all; presumably we only need that > in case the caller is torn down without ever having called Watch()? No, the only real need is to detect that StopAndDelete hasn't been called (by DCHECKing in the destructor). > Given that the caller isn't at liberty to destroy the object once Watch() has > been called, you could simply require that they call StopAndDelete() on the > object, and have it delete itself in there if Watch() has never been called? > Then the dtor can be private and the only risk is that the caller leaks it. Leaking the object is pretty dangerous because it will render the host unstoppable. I split moved most of ConfigFileWatcher to a private ConfigFileWatcherImpl. What remained in ConfigFileWatcher now implements fire-and-forget destruction, so scoped_ptr can be used. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:154: virtual void OnConfigUpdated(const std::string& config) OVERRIDE { On 2012/08/31 00:23:04, Wez wrote: > I actually mean "str" as a well-known abbreviation for "string", similarly to > our use of "config" for "configuration", to make the distinction between the > stringified |config| and |config_| more obvious. I understood. It is still a sort of Hungarian notation. I tried 'new_config' instead. It looked OK in OnConfigUpdated implementation, but declarations were odd. They raise the question why other setter(ish) methods don't call their parameters new_xxx? https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... remoting/host/win/worker_process_launcher.h:44: class Delegate { On 2012/08/31 00:23:04, Wez wrote: > base::win::ObjectWatcher doesn't take two *Delegate parameters, though. But some other class could implement base::win::ObjectWatcher::Delegate and some other delegate. By this logic it would not be clear enough why it deserves two.
https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:154: virtual void OnConfigUpdated(const std::string& config) OVERRIDE { On 2012/08/31 21:26:55, alexeypa wrote: > On 2012/08/31 00:23:04, Wez wrote: > > I actually mean "str" as a well-known abbreviation for "string", similarly to > > our use of "config" for "configuration", to make the distinction between the > > stringified |config| and |config_| more obvious. > > I understood. It is still a sort of Hungarian notation. > > I tried 'new_config' instead. It looked OK in OnConfigUpdated implementation, > but declarations were odd. They raise the question why other setter(ish) methods > don't call their parameters new_xxx? The difference here is that the types differ between the member and the parameter; normally the foo_ = foo pattern works fine, because foo and foo_ have equivalent types. |config_string| or |serialized_config| would be more suitable names for the parameter https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... remoting/host/win/worker_process_launcher.h:44: class Delegate { On 2012/08/31 21:26:55, alexeypa wrote: > On 2012/08/31 00:23:04, Wez wrote: > > base::win::ObjectWatcher doesn't take two *Delegate parameters, though. > > But some other class could implement base::win::ObjectWatcher::Delegate and some > other delegate. By this logic it would not be clear enough why it deserves two. That's not what's happening here, though; in this case you've defined two distinct delegate interface, both of which are specific to WorkerProcessLauncher, so their names need to distinguish their functions. However, I don't think WorkerProcessIpcDelegate is a delegate at all, since it doesn't do work on the WorkerProcessLauncher's behalf, so I'd suggest moving it into the WorkerProcessLauncher and naming it IpcHandler. https://chromiumcodereview.appspot.com/10855249/diff/40024/remoting/host/conf... File remoting/host/config_file_watcher.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/40024/remoting/host/conf... remoting/host/config_file_watcher.cc:28: // Creates a configuration file watcher that lives at the |io_task_runner| nit: "lives at" -> "runs on" or "lives on"
FYI https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/remo... remoting/host/remoting_me2me_host.cc:154: virtual void OnConfigUpdated(const std::string& config) OVERRIDE { On 2012/09/04 20:18:26, Wez wrote: > On 2012/08/31 21:26:55, alexeypa wrote: > The difference here is that the types differ between the member and the > parameter; normally the foo_ = foo pattern works fine, because foo and foo_ have > equivalent types. > > |config_string| or |serialized_config| would be more suitable names for the > parameter Done. https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... File remoting/host/win/worker_process_launcher.h (right): https://chromiumcodereview.appspot.com/10855249/diff/13004/remoting/host/win/... remoting/host/win/worker_process_launcher.h:44: class Delegate { On 2012/09/04 20:18:26, Wez wrote: > That's not what's happening here, though; in this case you've defined two > distinct delegate interface, both of which are specific to > WorkerProcessLauncher, so their names need to distinguish their functions. > > However, I don't think WorkerProcessIpcDelegate is a delegate at all, since it > doesn't do work on the WorkerProcessLauncher's behalf, so I'd suggest moving it > into the WorkerProcessLauncher and naming it IpcHandler. The reason for the split is that WorkerProcessIpcDelegate is cross-platform, while WorkerProcessLauncher::Delegate is Windows-specific. (WorkerProcessLauncher is Windows specific as well). I don't think at this point that removing "WorkerProcess" from WorkerProcessIpcDelegate is a good thing. This interface is intended to be very worker process specific (despite of its resemblance with IPC::Listener). I believe we should check it in as it is and revise it when Linux/Mac hosts will undergo the same refactoring. https://chromiumcodereview.appspot.com/10855249/diff/40024/remoting/host/conf... File remoting/host/config_file_watcher.cc (right): https://chromiumcodereview.appspot.com/10855249/diff/40024/remoting/host/conf... remoting/host/config_file_watcher.cc:28: // Creates a configuration file watcher that lives at the |io_task_runner| On 2012/09/04 20:18:26, Wez wrote: > nit: "lives at" -> "runs on" or "lives on" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10855249/26004
Try job failure for 10855249-26004 (retry) on linux_clang for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10855249/35028 |
