[remoting host] Only watch for material changes to NSS DB files

[remoting host] Only watch for material changes to NSS DB files

The remoting host process (on Linux) self-restarts if it detects
changes to the NSS DB directory. This modifies the watcher to only
trigger on material changes to the NSS DB files. This should reduce
incidences of false-restarts of the host.

BUG=609096
Committed: https://crrev.com/81e5e053c458bd919fff0575aaaaea9a2a4a050d
Cr-Commit-Position: refs/heads/master@{#393879}

[Lambros, 2016-05-13 19:30:41]

lambroslambrou@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Lambros, 2016-05-13 19:30:41]

I had to refactor this because of the extra data and work done on the IO thread.
The new ..Impl class takes the place of the old file_watcher_.

[Sergey Ulanov, 2016-05-13 22:03:52]

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
File remoting/host/linux/certificate_watcher.cc (right):

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:28: const char* const kCertFiles[] =
{"cert9.db", "key4.db", "pkcs11.txt"};
Declare it as const char kCertFiles[][]

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:34: class CertificateWatcherImpl {
It's confusing to have CertificateWatcherImpl while CertificateWatcher is not an
interface. rename this, e.g. CertDbContentWatcher

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:47: typedef size_t HashType;
Call this HashValue? We normally don't use Type suffix for types and this name
is confusing (at first I thought it's used to indicate type of a hash function)

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:97: true,
this can fit on the previous line. clang-format please (git cl format)

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:104: HashType new_hash =
ComputeHash();
whenever we get notification that the directory has changed we need to wait
before we actually read the data, in case the process that updates cert
databases in several steps and we get multiple notification.
If a process changes the DB and then reverts it back quickly it shouldn't cause
the host to be restarted.

Move restart_timer_ to this class?

[Lambros, 2016-05-14 01:27:52]

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
File remoting/host/linux/certificate_watcher.cc (right):

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:28: const char* const kCertFiles[] =
{"cert9.db", "key4.db", "pkcs11.txt"};
On 2016/05/13 22:03:51, Sergey Ulanov wrote:
> Declare it as const char kCertFiles[][]

This line fails to compile:
const char kCertFiles[][] = {"cert9.db", "key4.db", "pkcs11.txt"};
../../remoting/host/linux/certificate_watcher.cc:26:22: error: array has
incomplete element type 'const char []'

I brought this question up a few years ago, see:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/abtbE...

const char* const kFoo[] = {...}
seems to be the preferred way.

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:34: class CertificateWatcherImpl {
On 2016/05/13 22:03:51, Sergey Ulanov wrote:
> It's confusing to have CertificateWatcherImpl while CertificateWatcher is not
an
> interface. rename this, e.g. CertDbContentWatcher

Done.

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:47: typedef size_t HashType;
On 2016/05/13 22:03:51, Sergey Ulanov wrote:
> Call this HashValue? We normally don't use Type suffix for types and this name
> is confusing (at first I thought it's used to indicate type of a hash
function)

Done.

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:97: true,
On 2016/05/13 22:03:51, Sergey Ulanov wrote:
> this can fit on the previous line. clang-format please (git cl format)

Done.

https://codereview.chromium.org/1977963002/diff/1/remoting/host/linux/certifi...
remoting/host/linux/certificate_watcher.cc:104: HashType new_hash =
ComputeHash();
On 2016/05/13 22:03:51, Sergey Ulanov wrote:
> whenever we get notification that the directory has changed we need to wait
> before we actually read the data, in case the process that updates cert
> databases in several steps and we get multiple notification.
> If a process changes the DB and then reverts it back quickly it shouldn't
cause
> the host to be restarted.
> 
> Move restart_timer_ to this class?

Done.

[Sergey Ulanov, 2016-05-14 05:43:46]

lgtm

https://codereview.chromium.org/1977963002/diff/20001/remoting/host/linux/cer...
File remoting/host/linux/certificate_watcher.cc (right):

https://codereview.chromium.org/1977963002/diff/20001/remoting/host/linux/cer...
remoting/host/linux/certificate_watcher.cc:32: class CertDbContentWatcher {
Please add ThreadChecker in this class to make sure it's called in the right
thread.

[Lambros, 2016-05-16 17:35:00]

The CQ bit was checked by lambroslambrou@chromium.org

[Lambros, 2016-05-16 17:35:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1977963002/#ps40001
(title: "ThreadChecker")

[Lambros, 2016-05-16 17:35:15]

https://codereview.chromium.org/1977963002/diff/20001/remoting/host/linux/cer...
File remoting/host/linux/certificate_watcher.cc (right):

https://codereview.chromium.org/1977963002/diff/20001/remoting/host/linux/cer...
remoting/host/linux/certificate_watcher.cc:32: class CertDbContentWatcher {
On 2016/05/14 05:43:46, Sergey Ulanov wrote:
> Please add ThreadChecker in this class to make sure it's called in the right
> thread.

Done.

[commit-bot: I haz the power, 2016-05-16 17:35:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1977963002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1977963002/40001

[commit-bot: I haz the power, 2016-05-16 17:38:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-16 17:38:07]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Lambros, 2016-05-16 18:00:08]

The CQ bit was checked by lambroslambrou@chromium.org

[Lambros, 2016-05-16 18:00:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1977963002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-16 18:00:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1977963002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1977963002/60001

[commit-bot: I haz the power, 2016-05-16 18:36:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-16 18:38:06]

Description was changed from

==========
[remoting host] Only watch for material changes to NSS DB files

The remoting host process (on Linux) self-restarts if it detects
changes to the NSS DB directory. This modifies the watcher to only
trigger on material changes to the NSS DB files. This should reduce
incidences of false-restarts of the host.

BUG=609096
==========

to

==========
[remoting host] Only watch for material changes to NSS DB files

The remoting host process (on Linux) self-restarts if it detects
changes to the NSS DB directory. This modifies the watcher to only
trigger on material changes to the NSS DB files. This should reduce
incidences of false-restarts of the host.

BUG=609096

Committed: https://crrev.com/81e5e053c458bd919fff0575aaaaea9a2a4a050d
Cr-Commit-Position: refs/heads/master@{#393879}
==========

[commit-bot: I haz the power, 2016-05-16 18:38:07]

Patchset 4 (id:??) landed as
https://crrev.com/81e5e053c458bd919fff0575aaaaea9a2a4a050d
Cr-Commit-Position: refs/heads/master@{#393879}