[Chromoting] The daemon process now starts the networking process and passes the host configuration…

remoting/host/remoting_me2me_host.cc

Index: remoting/host/remoting_me2me_host.cc
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index 9a6d8d583280dd7cfdf2d934fe32fa5ed954b7e4..9c559f40af042115afc44b11af30647641950c9b 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -12,9 +12,9 @@
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
-#include "base/files/file_path_watcher.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/scoped_native_library.h"
 #include "base/string_util.h"
@@ -34,14 +34,16 @@
 #include "remoting/host/branding.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
-#include "remoting/host/composite_host_config.h"
+#include "remoting/host/chromoting_messages.h"
 #include "remoting/host/constants.h"
+#include "remoting/host/config_file_watcher.h"
 #include "remoting/host/desktop_environment.h"
 #include "remoting/host/event_executor.h"
 #include "remoting/host/heartbeat_sender.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/host_event_logger.h"
 #include "remoting/host/host_user_interface.h"
+#include "remoting/host/json_host_config.h"
 #include "remoting/host/log_to_server.h"
 #include "remoting/host/network_settings.h"
 #include "remoting/host/policy_hack/policy_watcher.h"
@@ -52,10 +54,6 @@
 #include "remoting/jingle_glue/xmpp_signal_strategy.h"
 #include "remoting/protocol/me2me_host_authenticator_factory.h"
 
-#if defined(OS_POSIX)
-#include "remoting/host/posix/sighup_listener.h"
-#endif  // defined(OS_POSIX)
-
 #if defined(OS_MACOSX)
 #include "base/mac/scoped_cftyperef.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
@@ -76,16 +74,8 @@ namespace {
 // This is used for tagging system event logs.
 const char kApplicationName[] = "chromoting";
 
-// The command line switch specifying the name of the Chromoting IPC channel.
-const char kDaemonIpcSwitchName[] = "chromoting-ipc";
-
-// These are used for parsing the config-file locations from the command line,
-// and for defining the default locations if the switches are not present.
-const char kAuthConfigSwitchName[] = "auth-config";
-const char kHostConfigSwitchName[] = "host-config";
-
-const FilePath::CharType kDefaultHostConfigFile[] =
-    FILE_PATH_LITERAL("host.json");
+// The command line switch specifying the name of the daemon IPC endpoint.
+const char kDaemonIpcSwitchName[] = "daemon-pipe";
 
 const int kMinPortNumber = 12400;
 const int kMaxPortNumber = 12409;
@@ -108,10 +98,12 @@ namespace remoting {
 
 class HostProcess
     : public HeartbeatSender::Listener,
-      public IPC::Listener {
+      public IPC::Listener,
+      public ConfigFileWatcher::Delegate {
  public:
   HostProcess(scoped_ptr<ChromotingHostContext> context)
       : context_(context.Pass()),
+        config_(FilePath()),
 #ifdef OFFICIAL_BUILD
         oauth_use_official_client_id_(true),
 #else
@@ -120,18 +112,16 @@ class HostProcess
         allow_nat_traversal_(true),
         restarting_(false),
         shutting_down_(false),
-        exit_code_(kSuccessExitCode)
+        exit_code_(kSuccessExitCode),
 #if defined(OS_MACOSX)
-      , curtain_(base::Bind(&HostProcess::OnDisconnectRequested,
+        curtain_(base::Bind(&HostProcess::OnDisconnectRequested,
                             base::Unretained(this)),
                  base::Bind(&HostProcess::OnDisconnectRequested,
                             base::Unretained(this)))

[Wez, 2012/08/30 20:36:04]

nit: Missing a comma here?

[alexeypa (please no reviews), 2012/08/30 23:15:13]

Yes, but not anymore.

 #endif
+        ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this))
   {
     network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
-    config_updated_timer_.reset(new base::DelayTimer<HostProcess>(
-        FROM_HERE, base::TimeDelta::FromSeconds(2), this,
-        &HostProcess::ConfigUpdatedDelayed));
   }
 
   bool InitWithCommandLine(const CommandLine* cmd_line) {
@@ -150,85 +140,69 @@ class HostProcess
           context_->network_task_runner()));
     }
 
+#if !defined(REMOTING_MULTI_PROCESS)
     FilePath default_config_dir = remoting::GetConfigDir();
-    if (cmd_line->HasSwitch(kAuthConfigSwitchName)) {
-      FilePath path = cmd_line->GetSwitchValuePath(kAuthConfigSwitchName);
-      config_.AddConfigPath(path);
-    }
-
     host_config_path_ = default_config_dir.Append(kDefaultHostConfigFile);
     if (cmd_line->HasSwitch(kHostConfigSwitchName)) {
       host_config_path_ = cmd_line->GetSwitchValuePath(kHostConfigSwitchName);
     }
-    config_.AddConfigPath(host_config_path_);
+#endif  // !defined(REMOTING_MULTI_PROCESS)
 
     return true;
   }
 
-  void ConfigUpdated() {
+  virtual void OnConfigUpdated(const std::string& config) OVERRIDE {

[Wez, 2012/08/30 20:36:04]

nit: config -> config_str

[alexeypa (please no reviews), 2012/08/30 23:15:13]

The style guide does not allow Hungarian notation and alike. I agree with it!

[Wez, 2012/08/31 00:23:04]

: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.

[alexeypa (please no reviews), 2012/08/31 21:26:55]

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?

[Wez, 2012/09/04 20:18:26]

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

[alexeypa (please no reviews), 2012/09/05 16:08:34]

Done.

     DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
-    // Call ConfigUpdatedDelayed after a short delay, so that this object won't
-    // try to read the updated configuration file before it has been
-    // completely written.
-    // If the writer moves the new configuration file into place atomically,
-    // this delay may not be necessary.
-    config_updated_timer_->Reset();
-  }
+    LOG(INFO) << "Processing new host configuration.";
 
-  void ConfigUpdatedDelayed() {
-    DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
+    if (!config_.SetSerializedData(config)) {
+      LOG(ERROR) << "Invalid configuration.";
+      OnConfigWatcherError();
+      return;
+    }
+
+    if (!ApplyConfig()) {
+      LOG(ERROR) << "Failed to apply the configuration.";
+      OnConfigWatcherError();
+      return;
+    }
 
-    if (LoadConfig()) {
+    // Start watching the policy (and eventually start the host) if this is
+    // the first configuration update. Otherwise, post a task to create new
+    // authenticator factory in case PIN has changed.
+    if (policy_watcher_.get() == NULL) {
+#if defined(OS_MACOSX) || defined(OS_WIN)
+      host_user_interface_.reset(new HostUserInterface(context_.get()));
+#endif
+
+      StartWatchingPolicy();
+    } else {
       // PostTask to create new authenticator factory in case PIN has changed.
       context_->network_task_runner()->PostTask(
           FROM_HERE,
           base::Bind(&HostProcess::CreateAuthenticatorFactory,
                      base::Unretained(this)));
-    } else {
-      LOG(ERROR) << "Invalid configuration.";
     }
   }
 
-#if defined(OS_WIN)
-  class ConfigChangedDelegate : public base::files::FilePathWatcher::Delegate {
-   public:
-    ConfigChangedDelegate(
-        scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-        const base::Closure& callback)
-        : task_runner_(task_runner),
-          callback_(callback) {
-    }
-
-    void OnFilePathChanged(const FilePath& path) OVERRIDE {
-      task_runner_->PostTask(FROM_HERE, callback_);
-    }
-
-    void OnFilePathError(const FilePath& path) OVERRIDE {
-    }
-
-   private:
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-    base::Closure callback_;
+  virtual void OnConfigWatcherError() OVERRIDE {
+    DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
-    DISALLOW_COPY_AND_ASSIGN(ConfigChangedDelegate);
-  };
-#endif  // defined(OS_WIN)
+    context_->network_task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&HostProcess::Shutdown, base::Unretained(this),
+                   kInvalidHostConfigurationExitCode));
+  }
 
-  void ListenForConfigChanges() {
-#if defined(OS_POSIX)
-    remoting::RegisterHupSignalHandler(
-        base::Bind(&HostProcess::ConfigUpdatedDelayed, base::Unretained(this)));
-#elif defined(OS_WIN)
-    scoped_refptr<base::files::FilePathWatcher::Delegate> delegate(
-        new ConfigChangedDelegate(
-            context_->ui_task_runner(),
-            base::Bind(&HostProcess::ConfigUpdated, base::Unretained(this))));
-    config_watcher_.reset(new base::files::FilePathWatcher());
-    if (!config_watcher_->Watch(host_config_path_, delegate)) {
-      LOG(ERROR) << "Couldn't watch file " << host_config_path_.value();
-    }
-#endif  // defined (OS_WIN)
+  void StartWatchingConfigChanges() {
+#if !defined(REMOTING_MULTI_PROCESS)
+    // Start watching the host configuration file.
+    config_watcher_.reset(new ConfigFileWatcher(context_->ui_task_runner(),
+                                                  context_->file_task_runner(),

[Wez, 2012/08/30 20:36:04]

nit: Indentation.

[alexeypa (please no reviews), 2012/08/30 23:15:13]

Done.

+                                                  weak_factory_.GetWeakPtr()));
+    config_watcher_->Watch(host_config_path_);
+#endif  // !defined(REMOTING_MULTI_PROCESS)
   }
 
   void CreateAuthenticatorFactory() {
@@ -242,38 +216,43 @@ class HostProcess
 
   // IPC::Listener implementation.
   virtual bool OnMessageReceived(const IPC::Message& message) {
+    DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
+
+#if defined(REMOTING_MULTI_PROCESS)
+    bool handled = true;
+    IPC_BEGIN_MESSAGE_MAP(HostProcess, message)
+        IPC_MESSAGE_HANDLER(ChromotingDaemonNetworkMsg_Configuration,
+                            OnConfigUpdated)
+        IPC_MESSAGE_UNHANDLED(handled = false)
+    IPC_END_MESSAGE_MAP()
+    return handled;
+#else  // !defined(REMOTING_MULTI_PROCESS)
     return false;
+#endif  // !defined(REMOTING_MULTI_PROCESS)
   }
 
   void StartHostProcess() {
     DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
-    if (!InitWithCommandLine(CommandLine::ForCurrentProcess()) ||
-        !LoadConfig()) {
-      context_->network_task_runner()->PostTask(
-          FROM_HERE,
-          base::Bind(&HostProcess::Shutdown, base::Unretained(this),
-                     kInvalidHostConfigurationExitCode));
+    if (!InitWithCommandLine(CommandLine::ForCurrentProcess())) {
+      OnConfigWatcherError();
       return;
     }
 
-#if defined(OS_MACOSX) || defined(OS_WIN)
-    host_user_interface_.reset(new HostUserInterface(context_.get()));
-#endif
-
-    StartWatchingPolicy();
-
-#if defined(OS_MACOSX) || defined(OS_WIN)
-    context_->file_task_runner()->PostTask(
-        FROM_HERE,
-        base::Bind(&HostProcess::ListenForConfigChanges,
-                   base::Unretained(this)));
-#endif
+    StartWatchingConfigChanges();
   }
 
   void ShutdownHostProcess() {
     DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
+    weak_factory_.InvalidateWeakPtrs();

[Wez, 2012/08/30 20:36:04]

nit: Add a short comment to explain why we're doing this.

[alexeypa (please no reviews), 2012/08/30 23:15:13]

Not needed anymore.

+
+#if !defined(REMOTING_MULTI_PROCESS)
+    if (config_watcher_.get()) {
+      config_watcher_.release()->StopAndDelete();
+    }
+#endif  // !defined(REMOTING_MULTI_PROCESS)
+
     daemon_channel_.reset();
 
 #if defined(OS_MACOSX) || defined(OS_WIN)
@@ -309,19 +288,10 @@ class HostProcess
         base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)));
   }
 
-  // Read host config, returning true if successful.
-  bool LoadConfig() {
+  // Applies the host config, returning true if successful.
+  bool ApplyConfig() {
     DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
-    // TODO(sergeyu): There is a potential race condition: this function is
-    // called on the main thread while the class members it mutates are used on
-    // the network thread. Fix it. http://crbug.com/140986 .
-
-    if (!config_.Read()) {
-      LOG(ERROR) << "Failed to read config file.";
-      return false;
-    }
-
     if (!config_.GetString(kHostIdConfigPath, &host_id_)) {
       LOG(ERROR) << "host_id is not defined in the config.";
       return false;
@@ -627,8 +597,11 @@ class HostProcess
   scoped_ptr<IPC::ChannelProxy> daemon_channel_;
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
 
+  JsonHostConfig config_;
+#if !defined(REMOTING_MULTI_PROCESS)
   FilePath host_config_path_;
-  CompositeHostConfig config_;
+  scoped_ptr<ConfigFileWatcher> config_watcher_;
+#endif  // !defined(REMOTING_MULTI_PROCESS)
 
   std::string host_id_;
   HostKeyPair key_pair_;
@@ -642,8 +615,6 @@ class HostProcess
 
   scoped_ptr<policy_hack::PolicyWatcher> policy_watcher_;
   bool allow_nat_traversal_;
-  scoped_ptr<base::files::FilePathWatcher> config_watcher_;
-  scoped_ptr<base::DelayTimer<HostProcess> > config_updated_timer_;
 
   bool restarting_;
   bool shutting_down_;
@@ -666,6 +637,9 @@ class HostProcess
 #if defined(OS_MACOSX)
     remoting::CurtainMode curtain_;
 #endif
+
+  // WeakPtr used to avoid tasks accessing |this| after it is deleted.
+  base::WeakPtrFactory<HostProcess> weak_factory_;
 };
 
 }  // namespace remoting
@@ -723,13 +697,9 @@ int main(int argc, char** argv) {
   if (!context->Start())
     return remoting::kHostInitializationFailed;
 
-  // Create the host process instance and run the rest of the initialization on
-  // the main message loop.
+  // Create the host process instance and enter the main message loop.
   remoting::HostProcess me2me_host(context.Pass());

[Wez, 2012/08/30 20:36:04]

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?

[alexeypa (please no reviews), 2012/08/30 23:15:13]

The ownership is transferred to HostProcess.
It is not going to outlive HostProcess.

[Wez, 2012/08/31 00:23:04]

Good point!

-  message_loop.PostTask(
-      FROM_HERE,
-      base::Bind(&remoting::HostProcess::StartHostProcess,
-                 base::Unretained(&me2me_host)));
+  me2me_host.StartHostProcess();
   return me2me_host.Run(&message_loop);
 }