Restart the host when the third party auth certificate changes

remoting/host/linux/certificate_watcher.cc

+// 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.
+
+#include "remoting/host/linux/certificate_watcher.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
+#include "base/thread_task_runner_handle.h"
+
+namespace remoting {
+
+// Delay time to restart the host when a change of certificate is detected.
+// This is to avoid repeating restarts when continuous writes to the database
+// occur.
+const int kRestartDelayInSecond = 2;
+
+// Full Path: $HOME/.pki/nssdb
+const char kCertDirectoryPath[] = ".pki/nssdb";
+
+CertificateWatcher::CertificateWatcher(
+    const base::Closure& restart_action,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
+    : restart_action_(restart_action),
+      network_task_runner_(base::ThreadTaskRunnerHandle::Get()),

[Sergey Ulanov, 2016/04/05 01:14:32]

nit: rename this to caller_task_runner_, to avoid assumptions about the thread
on which this class is created.

[Yuwei, 2016/04/05 20:46:23]

Done.

+      io_task_runner_(io_task_runner),
+      delay_(base::TimeDelta::FromSeconds(kRestartDelayInSecond)) {
+  if (!base::PathService::Get(base::DIR_HOME, &cert_watch_path_)) {
+    LOG(ERROR) << "Failed to get path of the home directory. ";

[Sergey Ulanov, 2016/04/05 01:14:32]

Looking at the implementation of base::PathService::Get() I don't think we
should expect it ever to fail, so this can be LOG(FATAL) and you can remove
return stament

[Yuwei, 2016/04/05 20:46:22]

Done.

+    return;
+  }
+  cert_watch_path_ = cert_watch_path_.AppendASCII(kCertDirectoryPath);
+}
+
+void CertificateWatcher::OnCertDirectoryChanged(

[Sergey Ulanov, 2016/04/05 01:14:32]

add "// static" comment above this line to make it clear that this method is
static.

+    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+    base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path,

[Sergey Ulanov, 2016/04/05 01:14:32]

One argument per line please (run clang-format)

[Yuwei, 2016/04/05 01:25:36]

I think clang-format actually moved |path| to the line above...

[Sergey Ulanov, 2016/04/05 18:35:21]

maybe it's somehow configured incorrectly on your machine? How do you run it?
Just running "git cl format" should do the right thing.

[Yuwei, 2016/04/05 19:15:51]

Tried git cl format and looks like it's not in the same line now... 

Just looked at
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
and looks like there are two params in a line in the second example. Is it like
you can have multiple params in the first line but not in other lines?

[Sergey Ulanov, 2016/04/05 19:34:46]

It's a chromium-specific rule, see
https://www.chromium.org/developers/coding-style#TOC-Code-formatting .
Multiple arguments are allowed on one line only if they all fit on a single line
.

[Yuwei, 2016/04/05 20:46:22]

Done.

+    bool error) {
+  network_task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&CertificateWatcher::DirectoryChanged, watcher_, path, error));
+}
+
+void CertificateWatcher::DirectoryChanged(const base::FilePath& path,
+                                          bool error) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  if (error || path != cert_watch_path_) {
+    LOG(WARNING) << "Unexpected file update callback. "
+                 << "Path: " << path.MaybeAsASCII() << "; "
+                 << "Error: " << error;
+    return;
+  }
+
+  restart_timer_->Reset();
+}
+
+void CertificateWatcher::OnTimer() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  if (inhibit_mode_) {
+    restart_pending_ = true;
+    return;
+  }
+
+  VLOG(1) << "Certificate was updated. Calling restart...";
+  restart_action_.Run();
+}
+
+CertificateWatcher::~CertificateWatcher() {

[Sergey Ulanov, 2016/04/05 01:14:32]

Move this after the constructor.

[Sergey Ulanov, 2016/04/05 01:14:32]

Add DCHECK(network_task_runner_->BelongsToCurrentThread()); to make sure it's
called on the right thread.

[Yuwei, 2016/04/05 20:46:22]

Checked in Stop()

+  if (monitor_) {
+    monitor_->RemoveStatusObserver(this);
+  }
+  DCHECK(io_task_runner_->DeleteSoon(FROM_HERE, file_watcher_.release()));

[Sergey Ulanov, 2016/04/05 01:14:32]

Don't put any statements with side-effects in [D]CHECK(). It should be safe to
ignore DeleteSoon() result here.

[Yuwei, 2016/04/05 01:25:36]

Would we do `bool result = ...; DCHECK(result);` or is it just better to not
DCHECK code with side effect at all...

[Sergey Ulanov, 2016/04/05 18:35:21]

Sure you can do that. The statement _inside_ DCHECK() is executed only in debug
builds, as long as it doesn't have side effects you are fine. Alternatively you
could use LOG(DFATAL), which has the safe effect as DCHECK(false):
if (!DeleteSoon()) {
  LOG(DFATAL) << "DeleteSoon failed";
}

But it's just not necessary in this case and we don't check DeleteSoon() result
in other places. io_task_runner_ here will normally be AutoThreadTaskRunner,
which guarantees that the thread outlives all references, so DeleteSoon() will
never return false here. And even if it does it means that the process is being
shut down and so it's safe to leak the object.

[Yuwei, 2016/04/05 20:09:30]

DeleteSoon here is causing memory leakage for the test... This function posts
job to the IO thread but in test context the dtor is called at the end of the
test case and there is no chance to manually run the loop.

What about having a Stop function to do this cleanups?

+
+  VLOG(1) << "Stopped watching certificate changes.";
+}
+
+bool CertificateWatcher::Start() {

[Sergey Ulanov, 2016/04/05 01:14:32]

Reorder methods in this file to match the declaration order in the header.

[Yuwei, 2016/04/05 20:46:22]

Done.

+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  if (cert_watch_path_.empty()) {

[Sergey Ulanov, 2016/04/05 01:14:32]

Given that base::PathService::Get() should never fail this can be replaced with
DCHECK(). and you can remove EmptyPathFailToStart test.

[Yuwei, 2016/04/05 20:46:22]

Done.

+    LOG(ERROR) << "Failed to start watching. "
+               << "The certificate path is not correctly set up.";
+    return false;
+  }
+
+  file_watcher_.reset(new base::FilePathWatcher());
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(base::IgnoreResult(&base::FilePathWatcher::Watch),
+                 base::Unretained(file_watcher_.get()), cert_watch_path_, true,
+                 base::Bind(&CertificateWatcher::OnCertDirectoryChanged,
+                            network_task_runner_, this->AsWeakPtr())));
+  restart_timer_.reset(new base::DelayTimer(FROM_HERE, delay_, this,
+                                            &CertificateWatcher::OnTimer));
+
+  VLOG(1) << "Started watching certificate changes.";
+  return true;
+}
+
+bool CertificateWatcher::is_started() const { return file_watcher_ != nullptr; }
+
+void CertificateWatcher::SetMonitor(base::WeakPtr<HostStatusMonitor> monitor) {
+  if (monitor_) {

[Sergey Ulanov, 2016/04/05 01:14:31]

Add DCHECK(is_started());

[Yuwei, 2016/04/05 20:46:22]

Done.

+    monitor_->RemoveStatusObserver(this);
+  }
+  DCHECK(monitor);

[Sergey Ulanov, 2016/04/05 01:14:32]

DCHECKs for arguments should normally be in the beginning of the function. But
this one can be removed. operator->() for WeakPtr<> contains the same DCHECK

[Yuwei, 2016/04/05 20:46:22]

Removed

+  monitor->AddStatusObserver(this);
+  monitor_ = monitor;
+}
+
+void CertificateWatcher::OnClientConnected(const std::string& jid) {
+  if (!is_started()) {

[Sergey Ulanov, 2016/04/05 01:14:32]

Don't need this if you add DCHECK(is_started());

[Yuwei, 2016/04/05 20:46:22]

Done.

+    return;
+  }
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  inhibit_mode_ = true;
+}
+
+void CertificateWatcher::OnClientDisconnected(const std::string& jid) {
+  if (!is_started()) {

[Sergey Ulanov, 2016/04/05 01:14:32]

don't need this.

[Yuwei, 2016/04/05 20:46:22]

Done.

+    return;
+  }
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  inhibit_mode_ = false;
+  if (restart_pending_) {
+    restart_pending_ = false;
+    restart_action_.Run();
+  }
+}
+
+void CertificateWatcher::SetDelayForTests(const base::TimeDelta& delay) {
+  DCHECK(!is_started());
+  delay_ = delay;
+}
+
+void CertificateWatcher::SetWatchPathForTests(
+    const base::FilePath& watch_path) {
+  DCHECK(!is_started());
+  cert_watch_path_ = watch_path;
+}
+
+}  // namespace remoting