Restart the host when the third party auth certificate changes

remoting/host/linux/certificate_watcher.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef HOST_LINUX_CERTIFICATE_WATCHER_H_

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

REMOTING_

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

Done.

+#define HOST_LINUX_CERTIFICATE_WATCHER_H_
+
+#include "base/files/file_path.h"
+#include "base/files/file_path_watcher.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/timer/timer.h"
+
+namespace remoting {
+
+class CertificateWatcherImplInterface {

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

We normally don't use Interface suffix for interfaces. Interface name should
generally be shorter than the name of the implementing class. But ImplInterface
suffix makes no sense. See my comment about impl_. I don't think you need any of
this at all.

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

Acknowledged.

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

Done. Removed interface and merged the impl

+ public:
+  virtual void Start() = 0;
+  virtual void Stop() = 0;
+  virtual void ScheduleSuicide() = 0;
+};
+
+/*
+ * This class watches the NSS database and kills the host when a change of the
+ * database is detected. The runner script will restart the host when the host
+ * is killed then the new host will capture any new changes of the database.
+ *
+ * Acceptable false positives will be caused by desktop sessions and other
+ * external programs.
+ */
+class CertificateWatcher {
+ public:
+  typedef base::Callback<void(void)> SuicideAction;

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

this is base::Closure. You don't need this typedef.
Also "void(void)" is better expressed as "void()"

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

Acknowledged.

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

Done.

+  CertificateWatcher(
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,

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

I think this class can be made single-threaded and live on the network thread.
Then you wouldn't need to pass any threads here and will not need any of the
impl_ stuff.

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

Acknowledged.

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

Looks like it doesn't work... There are checks in FilePathWatcher and
HostProcess to prevent being called on the wrong thread... Basically
FilePathWatcher needs IO thread and Host process needs network thread.

I changed the code a little bit so now StarOn() takes in the file runner and
there is a callback `OnNSSCertificateUpdate()` in HostProcess that dispatches
the shutdown task to the network thread.

+      scoped_refptr<base::SingleThreadTaskRunner> suicide_task_runner,
+      int suicide_delay, const SuicideAction& suicide_action);

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

Don't call it "suicide". The host process is just being restarted cleanly. It's
not killing itself.

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

So should I call it "restart"?

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

Done. Renamed to restart

+
+  // Constructs watcher with given impl. Will take ownership.
+  CertificateWatcher(CertificateWatcherImplInterface* impl);
+  virtual ~CertificateWatcher();
+  void Start();
+  void Stop();
+
+  // Marks suicide_scheduled_ flag without calling the suicide action
+  void Inhibit();
+
+  // Leaves inhibit mode and schedule suicide action if suicide_scheduled_
+  // is marked in previous inhibit mode.
+  void Uninhibit();
+
+  // Called when the certificate is updated.
+  void OnUpdate();
+
+ private:
+  scoped_ptr<CertificateWatcherImplInterface> impl_;

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

Normally we define nested Core class for pimpl classes. You don't need the
interface here - just forward-declare the class. e.g. see
remoting/host/audio_pump.h for example. But I don't think you need any of this
stuff here. This class can be simple single-threaded watcher running on the
network thread.

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

I basically separated the impl class so that I can test the logic in a cleaner
way. I agree that creating temp files may be better way for testing after seeing
your comments.

+
+  bool inhibit_mode_ = false;
+
+  // A flag to prevent posting multiple suicide tasks

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

nit: here and everywhere else: please add . at the end of the comment.

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

Acknowledged.

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

Done.

+  bool suicide_scheduled_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(CertificateWatcher);
+};
+
+}

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

// namespace remoting

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

Acknowledged.

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

Done.

+
+#endif  // HOST_LINUX_CERTIFICATE_WATCHER_H_