Restart the host when the third party auth certificate changes

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 eac1eec54792476712f96409c0062e1e17299a34..f87a0e47cdf3c05fbe5fa29463752a9dd6568f57 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -114,6 +114,8 @@
 #include <X11/Xlib.h>
 #include <base/linux_util.h>
 #include "remoting/host/audio_capturer_linux.h"
+#include "remoting/host/linux/certificate_watcher.h"
+#include "remoting/host/linux/certificate_watcher_inhibitor.h"
 #endif  // defined(OS_LINUX)
 
 #if defined(OS_WIN)
@@ -178,6 +180,10 @@ const int kShutdownTimeoutSeconds = 15;
 // before continuing normal process shutdown.
 const int kHostOfflineReasonTimeoutSeconds = 10;
 
+// Delay time to shutdown the host when a change of NSS database is detected.
+// This is to repeating restarts when continuous writes to the database occur.
+const int kCertUpdateShutdownDelaySeconds = 30;

[Sergey Ulanov, 2016/03/29 19:40:05]

I don't think we need delay restart that long. Currently you have timeout of 30
seconds starting from the first change in the cert directory. I think it's
better to have shorter delay starting at the last change. I.e. if the directory
keeps changing for 5 seconds then you keep the host running and restart it 2
seconds after that 5 second interval. It should be easy to implement this
behavior using base::DelayTimer, see
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h...

[Yuwei, 2016/03/29 19:57:03]

Acknowledged.

[Sergey Ulanov, 2016/03/29 23:09:54]

Normally we use "Acknowledged." response only for optional or no-op comments.
For comments that require some action like this one please mark them as "Done"
once they are done (unless you disagree with the comment, in which case you
should click "Reply" and write your response)

[Yuwei, 2016/03/30 18:47:45]

Done. DelayedTimer is being used.

+
 // Host offline reasons not associated with shutting down the host process
 // and therefore not expressible through HostExitCodes enum.
 const char kHostOfflineReasonPolicyReadError[] = "POLICY_READ_ERROR";
@@ -269,6 +275,7 @@ class HostProcess : public ConfigWatcher::Delegate,
       IPC::PlatformFileForTransit unprivileged_key);
 
  private:
+
   // See SetState method for a list of allowed state transitions.
   enum HostState {
     // Waiting for valid config and policies to be read from the disk.
@@ -389,6 +396,12 @@ class HostProcess : public ConfigWatcher::Delegate,
 
   scoped_ptr<ChromotingHostContext> context_;
 
+#if defined(OS_LINUX)
+  // Watch for NSS database changes and kill the host when changes occur
+  CertificateWatcher cert_watcher_;
+  CertificateWatcherInhibitor cert_watcher_inhibitor_;
+#endif
+
   // XMPP server/remoting bot configuration (initialized from the command line).
   XmppSignalStrategy::XmppServerConfig xmpp_server_config_;
   std::string directory_bot_jid_;
@@ -482,6 +495,14 @@ HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
                          int* exit_code_out,
                          ShutdownWatchdog* shutdown_watchdog)
     : context_(std::move(context)),
+#if defined(OS_LINUX)
+      cert_watcher_(context_->file_task_runner(),
+                    context_->network_task_runner(),
+                    kCertUpdateShutdownDelaySeconds,
+                    base::Bind(&HostProcess::ShutdownHost,
+                               this, kSuccessExitCode)),
+      cert_watcher_inhibitor_(cert_watcher_),
+#endif
       state_(HOST_STARTING),
       use_service_account_(false),
       enable_vp9_(false),
@@ -806,6 +827,11 @@ void HostProcess::CreateAuthenticatorFactory() {
     DCHECK(third_party_auth_config_.token_url.is_valid());
     DCHECK(third_party_auth_config_.token_validation_url.is_valid());
 
+#if defined(OS_LINUX)
+    cert_watcher_.Start();
+    host_->AddStatusObserver(&cert_watcher_inhibitor_);

[Sergey Ulanov, 2016/03/29 19:40:05]

You also need to call RemoveStatusObserver() before host_ is destroyed. In some
cases host needs to be restarted, e.g. when system policies change. When this
happens we keep the process running but destroy the old ChromotingHost object
and create a new one. 
I think HostStatusObserver::OnShutdown() is the right place to call
RemoveStatusObserver()

[Yuwei, 2016/03/29 19:57:03]

Acknowledged.

[Yuwei, 2016/03/29 21:37:41]

I am a little bit confused... So the host will not clear the observer list when
it shuts down? It seems a little bit weird to have an observer removing itself
from the host when the host shuts down (the code will also look weird since you
need to pass in a reference to the host or something)...

[Sergey Ulanov, 2016/03/29 23:09:54]

In general when using the observer pattern it's necessary to make sure that if
the observer is deleted before the observable then it is removed from the list
of observers. So to ensure that usually observable objects verify in the
destructor that the observer list is empty (because if it's not empty it
indicates observable/observer are not destroyed properly). ObserverList has a
flag for that (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
but we don't have it enabled in ChromotingHost.
This particular case is a bit different because the observer outlives the
observable. Still as a rule of thumb there should always be one
RemoveStatusObserver() call for each AddStatusObserver()
Actually it's more common to have observer register/unregister itself, e.g. see
remoting/host/host_status_logger.cc .
So I suggest you use the same approach here as well in CertificateWatche. It
just needs to be notified when there is a new instance of ChromotingHost being
created. So it will look something like this:
  cert_watcher_->OnHostCreated(host_.get());

and in cert_watcher.cc:

void CertWatcher::OnHostCreated(Host* host) {
  host_ = host;
  host_->AddObserver(this);
}

void CertWatcher::OnHostShutdown(Host* host) {
  host_->RemoveObserver(this);
}

This also make sense because it hides the fact that the CertWatcher is a
HostStatusObserver, which is an internal detail of its implementation.

[Yuwei, 2016/03/30 18:47:45]

Done. Observer will be added or removed in Start() and Stop() (Stop() is also
called in dtor).

+#endif
+
     scoped_refptr<protocol::TokenValidatorFactory> token_validator_factory =
         new TokenValidatorFactoryImpl(third_party_auth_config_, key_pair_,
                                       context_->url_request_context_getter());