Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1838313002:
Restart the host when the third party auth certificate changes (Closed)
|
Created:
4 years, 8 months ago by Yuwei Modified:
4 years, 8 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhen using third-party authentication the host may need to use client certificates. The host currently doesn't handle the case that the certificate file gets updated when the host is running, in which case later session connection will fail due to outdated certificate.
This patch schedules host-restart when the cert folder (~/.pki/nssdb) is changed and restart the host when no client connection is established.
BUG=598819
Committed: https://crrev.com/7821527cafe2199a3603d26daae639c621602a2f
Cr-Commit-Position: refs/heads/master@{#385801}
Patch Set 1 #
Total comments: 53
Patch Set 2 : Applied Feedback From sergeyu@ #
Total comments: 80
Patch Set 3 : Reviewed Feedback from sergeyu@ #
Total comments: 61
Patch Set 4 : Feedback from sergeyu@ #
Total comments: 30
Patch Set 5 : Reviewed #
Total comments: 30
Patch Set 6 : Reviewer's fb; fixed memory leak #
Total comments: 18
Patch Set 7 : Reviewer's Feedback #
Total comments: 12
Patch Set 8 : Reviewer's Feedback #
Total comments: 2
Patch Set 9 : Fix small typo #
Messages
Total messages: 92 (28 generated)
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think the new code can be simplified. CertificateWatcher can be single-threaded. CertificateWatcherInhibitor doesn't need to be separate from CertificateWatcher. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.cc:91: base::TimeDelta::FromSeconds(suicide_delay_)); this line is not indented correctly. Please use clang-format: https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.cc:169: } // namespace remoting https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:5: #ifndef HOST_LINUX_CERTIFICATE_WATCHER_H_ REMOTING_ https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:16: class CertificateWatcherImplInterface { 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. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:33: typedef base::Callback<void(void)> SuicideAction; this is base::Closure. You don't need this typedef. Also "void(void)" is better expressed as "void()" https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:35: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner, 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. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:37: int suicide_delay, const SuicideAction& suicide_action); Don't call it "suicide". The host process is just being restarted cleanly. It's not killing itself. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:56: scoped_ptr<CertificateWatcherImplInterface> impl_; 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. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:60: // A flag to prevent posting multiple suicide tasks nit: here and everywhere else: please add . at the end of the comment. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:66: } // namespace remoting https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher_inhibitor.h (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_inhibitor.h:14: /** Use C++ style comments please: // Blah blah. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_inhibitor.h:21: class CertificateWatcherInhibitor : public remoting::HostStatusObserver { Why does this need to be separate from CertificateWatcher? I don't see any good reason to have these two classes separated from each other. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:5: #include "remoting/host/linux/certificate_watcher.h" nit: emtpy line after this include https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:9: class MockCertificateWatcherImpl : public CertificateWatcherImplInterface { add empty line after namespace. indentation. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:9: class MockCertificateWatcherImpl : public CertificateWatcherImplInterface { So this test mocks the most interesting part of the code being tested. unittests like this are rarely useful. I think it should be easy to test the real implementation without faking any of the internal behavior. Just create a temp directory, install the watcher and verify that it does the right thing. ScopedTempDir class from base/files/scoped_temp_dir.h might be useful to implement this. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:25: private: empty line above this one. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:31: CertificateWatcher** pWatcher) { We don't use Hungarian notation, so this should be just "watcher", not pWatcher. https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:185: const int kCertUpdateShutdownDelaySeconds = 30; 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... https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:832: host_->AddStatusObserver(&cert_watcher_inhibitor_); 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()
https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.cc:91: base::TimeDelta::FromSeconds(suicide_delay_)); On 2016/03/29 19:40:04, Sergey Ulanov wrote: > this line is not indented correctly. Please use clang-format: > https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.cc:169: } On 2016/03/29 19:40:04, Sergey Ulanov wrote: > // namespace remoting Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:16: class CertificateWatcherImplInterface { On 2016/03/29 19:40:04, Sergey Ulanov wrote: > 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. Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:33: typedef base::Callback<void(void)> SuicideAction; On 2016/03/29 19:40:04, Sergey Ulanov wrote: > this is base::Closure. You don't need this typedef. > Also "void(void)" is better expressed as "void()" Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:35: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner, On 2016/03/29 19:40:04, Sergey Ulanov wrote: > 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. Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:37: int suicide_delay, const SuicideAction& suicide_action); On 2016/03/29 19:40:04, Sergey Ulanov wrote: > Don't call it "suicide". The host process is just being restarted cleanly. It's > not killing itself. So should I call it "restart"? https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:56: scoped_ptr<CertificateWatcherImplInterface> impl_; On 2016/03/29 19:40:04, Sergey Ulanov wrote: > 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. 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. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:60: // A flag to prevent posting multiple suicide tasks On 2016/03/29 19:40:04, Sergey Ulanov wrote: > nit: here and everywhere else: please add . at the end of the comment. Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:66: } On 2016/03/29 19:40:04, Sergey Ulanov wrote: > // namespace remoting Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher_inhibitor.h (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_inhibitor.h:14: /** On 2016/03/29 19:40:04, Sergey Ulanov wrote: > Use C++ style comments please: > // Blah blah. Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_inhibitor.h:21: class CertificateWatcherInhibitor : public remoting::HostStatusObserver { On 2016/03/29 19:40:04, Sergey Ulanov wrote: > Why does this need to be separate from CertificateWatcher? I don't see any good > reason to have these two classes separated from each other. Will merge them and use temp files to test https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:5: #include "remoting/host/linux/certificate_watcher.h" On 2016/03/29 19:40:05, Sergey Ulanov wrote: > nit: emtpy line after this include Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:9: class MockCertificateWatcherImpl : public CertificateWatcherImplInterface { On 2016/03/29 19:40:04, Sergey Ulanov wrote: > add empty line after namespace. > indentation. Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:9: class MockCertificateWatcherImpl : public CertificateWatcherImplInterface { On 2016/03/29 19:40:05, Sergey Ulanov wrote: > So this test mocks the most interesting part of the code being tested. unittests > like this are rarely useful. I think it should be easy to test the real > implementation without faking any of the internal behavior. Just create a temp > directory, install the watcher and verify that it does the right thing. > ScopedTempDir class from base/files/scoped_temp_dir.h might be useful to > implement this. Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:25: private: On 2016/03/29 19:40:05, Sergey Ulanov wrote: > empty line above this one. Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:31: CertificateWatcher** pWatcher) { On 2016/03/29 19:40:05, Sergey Ulanov wrote: > We don't use Hungarian notation, so this should be just "watcher", not pWatcher. > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:185: const int kCertUpdateShutdownDelaySeconds = 30; On 2016/03/29 19:40:05, Sergey Ulanov wrote: > 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... Acknowledged. https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:832: host_->AddStatusObserver(&cert_watcher_inhibitor_); On 2016/03/29 19:40:05, Sergey Ulanov wrote: > 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() Acknowledged.
Description was changed from ========== Kill the host when the NSS certificate changes Will defer suicide when the host is connected to the client BUG=http://b/21106876 ========== to ========== Kill the host when the NSS certificate changes Will defer suicide when the host is connected to the client b/21106876 BUG=598819 ==========
https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:832: host_->AddStatusObserver(&cert_watcher_inhibitor_); On 2016/03/29 19:40:05, Sergey Ulanov wrote: > 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() 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)...
https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:185: const int kCertUpdateShutdownDelaySeconds = 30; On 2016/03/29 19:57:03, yuweih wrote: > Acknowledged. 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) https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:832: host_->AddStatusObserver(&cert_watcher_inhibitor_); On 2016/03/29 21:37:41, yuweih wrote: > On 2016/03/29 19:40:05, Sergey Ulanov wrote: > > 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() > > 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 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() > (the code will also look weird since you > need to pass in a reference to the host or something)... 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.
Patch 2 is uploaded. PTAL https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.cc:91: base::TimeDelta::FromSeconds(suicide_delay_)); On 2016/03/29 19:40:04, Sergey Ulanov wrote: > this line is not indented correctly. Please use clang-format: > https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md Done. Have run clang-format https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.cc:169: } On 2016/03/29 19:40:04, Sergey Ulanov wrote: > // namespace remoting Done. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:5: #ifndef HOST_LINUX_CERTIFICATE_WATCHER_H_ On 2016/03/29 19:40:04, Sergey Ulanov wrote: > REMOTING_ Done. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:16: class CertificateWatcherImplInterface { On 2016/03/29 19:40:04, Sergey Ulanov wrote: > 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. Done. Removed interface and merged the impl https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:33: typedef base::Callback<void(void)> SuicideAction; On 2016/03/29 19:40:04, Sergey Ulanov wrote: > this is base::Closure. You don't need this typedef. > Also "void(void)" is better expressed as "void()" Done. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:35: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner, On 2016/03/29 19:40:04, Sergey Ulanov wrote: > 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. 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. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:37: int suicide_delay, const SuicideAction& suicide_action); On 2016/03/29 19:40:04, Sergey Ulanov wrote: > Don't call it "suicide". The host process is just being restarted cleanly. It's > not killing itself. Done. Renamed to restart https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:60: // A flag to prevent posting multiple suicide tasks On 2016/03/29 19:40:04, Sergey Ulanov wrote: > nit: here and everywhere else: please add . at the end of the comment. Done. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher.h:66: } On 2016/03/29 19:40:04, Sergey Ulanov wrote: > // namespace remoting Done. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:5: #include "remoting/host/linux/certificate_watcher.h" On 2016/03/29 19:40:05, Sergey Ulanov wrote: > nit: emtpy line after this include Done. https://codereview.chromium.org/1838313002/diff/1/remoting/host/linux/certifi... remoting/host/linux/certificate_watcher_unittest.cc:9: class MockCertificateWatcherImpl : public CertificateWatcherImplInterface { On 2016/03/29 19:40:05, Sergey Ulanov wrote: > So this test mocks the most interesting part of the code being tested. unittests > like this are rarely useful. I think it should be easy to test the real > implementation without faking any of the internal behavior. Just create a temp > directory, install the watcher and verify that it does the right thing. > ScopedTempDir class from base/files/scoped_temp_dir.h might be useful to > implement this. Class rewritten to test the real situation of watching a file. https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:185: const int kCertUpdateShutdownDelaySeconds = 30; On 2016/03/29 19:40:05, Sergey Ulanov wrote: > 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... Done. DelayedTimer is being used. https://codereview.chromium.org/1838313002/diff/1/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:832: host_->AddStatusObserver(&cert_watcher_inhibitor_); On 2016/03/29 19:40:05, Sergey Ulanov wrote: > 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() Done. Observer will be added or removed in Start() and Stop() (Stop() is also called in dtor).
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:16: const char kNSSEnvironmentPrefix[] = "HOME"; Please use base::PathService with base::DIR_HOME to get home directory path. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:17: const char kNSSWatchPathToHome[] = "/.pki/nssdb"; Maybe call this kNssCertDirectoryPath - notice lower-case 'ss' https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:29: const base::Closure& restart_deferred_action, const base::FilePath& path) one argument per line please (clang-format?) https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:47: if (inhibit_mode_) { I think we want to always start the timer here. Then OnTimer() should check if there is an active client and defer restart until that client is disconnected. E.g. currently this code doesn't handle the case when a new client is connected between OnNssUpdate() and OnTimer(). https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:84: LOG(INFO) << "Started watching certificate changes."; I don't think you need this comment. At least not as LOG(INFO). Maybe replace it with VLOG(1) https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:87: void CertificateWatcher::Stop() { This method is not used anywhere except the destructor. Just move this code to the destructor. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:95: LOG(INFO) << "Stopped watching certificate changes."; same as above. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:103: restart_timer_->Reset(); I don't think you really want to activate the timer in this case. The timer is needed because the certs directory may keep changing. But that doesn't apply to the case when the client disconnects. I think if the certs were updated while a client was connected then you want to restart the host right after the client disconnects (provided that 2 seconds interval has passed since the last change) https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:109: // HostStatusObserver Impl: remove this comment https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:14: remove this empty line https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:33: CertificateWatcher(int delay, Does the delay really need to be passed to the constructor? If it's useful only for tests then add set_delay_for_tests(). https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:35: const base::Closure& restart_deferred_action, This parameter is used only for tests. Do you really need it? https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:36: const base::FilePath& watch_path); This argument is used for tests only. Replace it with set_watch_path_for_tests method. Then you wouldn't need overloaded constructor. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:41: remove extra empty lines https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:48: void StartOn(scoped_refptr<AutoThreadTaskRunner> runner, Why do you need this method? https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:58: void Uninhibit(); Do you still need this? https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:60: // HostStatusObserver interface: s/:/./ https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:62: // inhibits CertificateWatcher when the client connects Don't need comments for interface overrides.. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:75: int delay_; Use base::TimeDelta for time-delta values. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:78: base::WeakPtr<HostStatusMonitor> monitor_; Move this and all other values passed to the constructor, so that they are first in the list. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:87: // The file watcher to watch the certificate. the watcher watches certs directory, which may contain multiple certificates. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:94: base::FilePath nss_watch_path_; Move this above the |file_watcher_|, so the order of definition matches the logical order in which these fields are initialized/used. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:96: // called when the certificate get updated. nit: Suggest rewarding: "Callback passed to |file_watcher_|." Also please notice capital "C". Comments should start with upper-case letter. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:97: void OnNSSUpdate(const base::FilePath& path, bool error); Functions should be defined before data memebers: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:97: void OnNSSUpdate(const base::FilePath& path, bool error); Suggest renaming to OnCertDirectoryChanged(). "NSSUpdate" makes little sense. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:99: // called when the timer ticks. This comment doesn't add anything. Maybe reword it "Called by |restart_timer_| when it's time to restart the host." https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:10: nit: remove this empty line https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:12: #include "remoting/host/linux/certificate_watcher.h" this should be first include: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:17: const std::string WATCH_FILENAME = "testfile.txt"; static constant of std::string are not allowed because they require static initializers. You can use char array: const char kWatchFilename[] = "testfile.txt". https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:17: const std::string WATCH_FILENAME = "testfile.txt"; kWatchFilename https://google.github.io/styleguide/cppguide.html#Constant_Names https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:21: public: None of these fields and methods need to be public. Make the protected? https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:24: watcher_runner_( You don't need this. MessageLoop already has a task runner. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:27: temp_dir(), Don't need this initializer - the compiler will call the default constructor implicitly. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:47: base::MessageLoopForIO watcher_loop_; The convention is to call MessageLoop instances in tests message_loop_ https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:51: CertificateWatcher watcher; watcher_ with '_' suffix. Same for other members https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:69: // watcher_loop_.QuitNow(); Please remove commented code. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:278: don't need this empty line. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:375: // Handler for NSS certificate update event when the host is running. Suggest not referring to NSS in this file. The host needs to be restarted when client certs are changed. The fact that NSS is used to read the certs is an uninteresting detail here. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:376: void OnNSSCertificateUpdate(); Suggest renaming to "OnHostRestartRequested".
https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:29: const base::Closure& restart_deferred_action, const base::FilePath& path) On 2016/03/30 21:02:44, Sergey Ulanov wrote: > one argument per line please (clang-format?) Okay... In fact this file has been processed by clang-format... https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:47: if (inhibit_mode_) { On 2016/03/30 21:02:44, Sergey Ulanov wrote: > I think we want to always start the timer here. Then OnTimer() should check if > there is an active client and defer restart until that client is disconnected. > E.g. currently this code doesn't handle the case when a new client is connected > between OnNssUpdate() and OnTimer(). This sounds better although I don't think the client can still successfully connect to the host after OnNSSUpdate happens... https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:33: CertificateWatcher(int delay, On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Does the delay really need to be passed to the constructor? If it's useful only > for tests then add set_delay_for_tests(). So may just have a constant inside the CertificateWatcher class? https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:35: const base::Closure& restart_deferred_action, On 2016/03/30 21:02:45, Sergey Ulanov wrote: > This parameter is used only for tests. Do you really need it? In fact this constructor is basically for tests only... https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:36: const base::FilePath& watch_path); On 2016/03/30 21:02:45, Sergey Ulanov wrote: > This argument is used for tests only. Replace it with set_watch_path_for_tests > method. Then you wouldn't need overloaded constructor. okay https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:48: void StartOn(scoped_refptr<AutoThreadTaskRunner> runner, On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Why do you need this method? For most of the cases you can simply run watcher on the main thread... Do you suggest that it's the caller's responsibility to run runner->PostTask(&Start...) instead of the callee's? https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:58: void Uninhibit(); On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Do you still need this? Basically only observer's functions are calling these functions, but these functions do exactly what their names say and doesn't take unnecessary arguments like jid. I feel like keeping these functions may make the test code look clearer...
Patchset #3 (id:40001) has been deleted
Patch 3 is uploaded. PTAL https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:16: const char kNSSEnvironmentPrefix[] = "HOME"; On 2016/03/30 21:02:44, Sergey Ulanov wrote: > Please use base::PathService with base::DIR_HOME to get home directory path. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:17: const char kNSSWatchPathToHome[] = "/.pki/nssdb"; On 2016/03/30 21:02:44, Sergey Ulanov wrote: > Maybe call this kNssCertDirectoryPath - notice lower-case 'ss' Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:47: if (inhibit_mode_) { On 2016/03/30 21:02:44, Sergey Ulanov wrote: > I think we want to always start the timer here. Then OnTimer() should check if > there is an active client and defer restart until that client is disconnected. > E.g. currently this code doesn't handle the case when a new client is connected > between OnNssUpdate() and OnTimer(). Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:84: LOG(INFO) << "Started watching certificate changes."; On 2016/03/30 21:02:44, Sergey Ulanov wrote: > I don't think you need this comment. At least not as LOG(INFO). Maybe replace it > with VLOG(1) Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:87: void CertificateWatcher::Stop() { On 2016/03/30 21:02:44, Sergey Ulanov wrote: > This method is not used anywhere except the destructor. Just move this code to > the destructor. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:95: LOG(INFO) << "Stopped watching certificate changes."; On 2016/03/30 21:02:44, Sergey Ulanov wrote: > same as above. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:103: restart_timer_->Reset(); On 2016/03/30 21:02:44, Sergey Ulanov wrote: > I don't think you really want to activate the timer in this case. The timer is > needed because the certs directory may keep changing. But that doesn't apply to > the case when the client disconnects. I think if the certs were updated while a > client was connected then you want to restart the host right after the client > disconnects (provided that 2 seconds interval has passed since the last change) Logic changed so now we don't need to call restart when inhibit turns off... https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:109: // HostStatusObserver Impl: On 2016/03/30 21:02:44, Sergey Ulanov wrote: > remove this comment Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:14: On 2016/03/30 21:02:45, Sergey Ulanov wrote: > remove this empty line Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:33: CertificateWatcher(int delay, On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Does the delay really need to be passed to the constructor? If it's useful only > for tests then add set_delay_for_tests(). Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:36: const base::FilePath& watch_path); On 2016/03/30 21:02:45, Sergey Ulanov wrote: > This argument is used for tests only. Replace it with set_watch_path_for_tests > method. Then you wouldn't need overloaded constructor. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:48: void StartOn(scoped_refptr<AutoThreadTaskRunner> runner, On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Why do you need this method? Removed https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:58: void Uninhibit(); On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Do you still need this? Changed to SetInhibit() https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:60: // HostStatusObserver interface: On 2016/03/30 21:02:45, Sergey Ulanov wrote: > s/:/./ Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:62: // inhibits CertificateWatcher when the client connects On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Don't need comments for interface overrides.. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:75: int delay_; On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Use base::TimeDelta for time-delta values. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:78: base::WeakPtr<HostStatusMonitor> monitor_; On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Move this and all other values passed to the constructor, so that they are first > in the list. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:87: // The file watcher to watch the certificate. On 2016/03/30 21:02:45, Sergey Ulanov wrote: > the watcher watches certs directory, which may contain multiple certificates. Changed comments... https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:94: base::FilePath nss_watch_path_; On 2016/03/30 21:02:44, Sergey Ulanov wrote: > Move this above the |file_watcher_|, so the order of definition matches the > logical order in which these fields are initialized/used. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:96: // called when the certificate get updated. On 2016/03/30 21:02:45, Sergey Ulanov wrote: > nit: Suggest rewarding: "Callback passed to |file_watcher_|." Also please notice > capital "C". Comments should start with upper-case letter. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:97: void OnNSSUpdate(const base::FilePath& path, bool error); On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Functions should be defined before data memebers: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:99: // called when the timer ticks. On 2016/03/30 21:02:45, Sergey Ulanov wrote: > This comment doesn't add anything. Maybe reword it "Called by |restart_timer_| > when it's time to restart the host." Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:10: On 2016/03/30 21:02:46, Sergey Ulanov wrote: > nit: remove this empty line Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:12: #include "remoting/host/linux/certificate_watcher.h" On 2016/03/30 21:02:45, Sergey Ulanov wrote: > this should be first include: > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:17: const std::string WATCH_FILENAME = "testfile.txt"; On 2016/03/30 21:02:45, Sergey Ulanov wrote: > kWatchFilename > https://google.github.io/styleguide/cppguide.html#Constant_Names Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:21: public: On 2016/03/30 21:02:46, Sergey Ulanov wrote: > None of these fields and methods need to be public. Make the protected? Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:24: watcher_runner_( On 2016/03/30 21:02:46, Sergey Ulanov wrote: > You don't need this. MessageLoop already has a task runner. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:27: temp_dir(), On 2016/03/30 21:02:46, Sergey Ulanov wrote: > Don't need this initializer - the compiler will call the default constructor > implicitly. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:47: base::MessageLoopForIO watcher_loop_; On 2016/03/30 21:02:46, Sergey Ulanov wrote: > The convention is to call MessageLoop instances in tests message_loop_ Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:51: CertificateWatcher watcher; On 2016/03/30 21:02:45, Sergey Ulanov wrote: > watcher_ with '_' suffix. Same for other members Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:69: // watcher_loop_.QuitNow(); On 2016/03/30 21:02:45, Sergey Ulanov wrote: > Please remove commented code. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:278: On 2016/03/30 21:02:46, Sergey Ulanov wrote: > don't need this empty line. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:375: // Handler for NSS certificate update event when the host is running. On 2016/03/30 21:02:46, Sergey Ulanov wrote: > Suggest not referring to NSS in this file. The host needs to be restarted when > client certs are changed. The fact that NSS is used to read the certs is an > uninteresting detail here. Done. https://codereview.chromium.org/1838313002/diff/20001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:376: void OnNSSCertificateUpdate(); On 2016/03/30 21:02:46, Sergey Ulanov wrote: > Suggest renaming to "OnHostRestartRequested". Done.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:17: // This is to repeating restarts when continuous writes to the database occur. s/to repeating/to avoid repeating/ https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:21: const base::BasePathKey kNssBasePath = base::DIR_HOME; Don't need this. Just pass base::DIR_HOME when calling PathService::Get(). https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:30: base::PathService::Get(kNssBasePath, &nss_watch_path_); Verify that you get correct result from here. If we don't then I think we want to log the error and keep the watcher disabled. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:31: nss_watch_path_ = nss_watch_path_.AppendASCII( call it watch_path_ instead of nss_watch_path_ https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:32: std::string(kNssCertDirectoryPath)); Don't need std::string(). Compiler will convert it implicitly. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:50: restart_deferred_action_.Run(); Why do you need this? https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:56: restart_timer_->Reset(); I don't think you want to rest the timer here. It doesn't make sense to keep the timer running while we are waiting the user to disconnect. Instead I suggest you add a boolean flag that would indicate that restart is pending. So here you will have restart_pending_ = true; https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:78: auto raw_timer = Don't need this variable: restart_timer.reset(new base::DelayTimer(...)); https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:80: delay_, Please clang-format this code. It will move this line to the previous one. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:88: void CertificateWatcher::SetInhibit(bool inhibit) { inhibit_mode_ = inhibit; } Don't need this method. Just set the variable in OnClientConnected() and OnClientDisconnected() https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:96: } if (restart_pending_) { restart_action_.Run(); } https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:98: void CertificateWatcher::SetDelay(const base::TimeDelta& delay) { Call it SetDelayForTests. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:106: void CertificateWatcher::SetRestartDeferredAction( Why do you need this? https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:20: // This class watches the NSS database and kills the host when a change of the s/NSS database/certificate database/ https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:40: removw this empty line. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:47: // Used for tests. No change will be made if the watcher has already started use ForTests suffix for test-only methods please. Also add DCHECK() to enforce that these methods are called only in proper state. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:60: bool inhibit_mode_ = false; move this below. Normally the values passed to the constructor should proceed everything else. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:78: base::Closure restart_action_; move this after monitor_. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:18: const char kWatchFileName[] = "testfile.txt"; nit: add empty line. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:100: watcher_.SetInhibit(true); Call OnClientConnected() and OnClientDisconnected() here instead of SetInhibit() and remove SetInhibit(). Otherwise the test doesn't verify that OnClientDisconnected() is handled correctly. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:819: cert_watcher_.reset(new CertificateWatcher( CreateAuthenticatorFactory() is called every time the host is restarted, but we don't want to create a new instance of CertificateWatcher every time - certs may be updated while the host is being restarted and we would loose the state in that case. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:822: context_->file_task_runner()->PostTask( I don't think you need to use file_task_runner() here. CertificateWatcher should work just fin on the network thread.
https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:50: restart_deferred_action_.Run(); On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Why do you need this? For testing reason. We need some way to quit the message loop if the restart action is deferred... https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:56: restart_timer_->Reset(); On 2016/03/31 22:36:15, Sergey Ulanov wrote: > I don't think you want to rest the timer here. It doesn't make sense to keep the > timer running while we are waiting the user to disconnect. Instead I suggest you > add a boolean flag that would indicate that restart is pending. So here you will > have > restart_pending_ = true; I guess I misunderstood your last comment about always starting the timer in inhibit mode. So we still want to use a flag to mark a pending restart but we should defer it at the beginning of OnTimer instead of at the end of OnUpdate, right? https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:106: void CertificateWatcher::SetRestartDeferredAction( On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Why do you need this? See above... For quitting the message loop https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:819: cert_watcher_.reset(new CertificateWatcher( On 2016/03/31 22:36:16, Sergey Ulanov wrote: > CreateAuthenticatorFactory() is called every time the host is restarted, but we > don't want to create a new instance of CertificateWatcher every time - certs may > be updated while the host is being restarted and we would loose the state in > that case. Currently the watcher get constructed just before the authorization starts and destructed when the host dies. Not sure why we need to care about during the interval of (old host died, new host authorized)... We can keep using the same watcher for the whole process then it will need to reset the host reference anyways... https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:822: context_->file_task_runner()->PostTask( On 2016/03/31 22:36:16, Sergey Ulanov wrote: > I don't think you need to use file_task_runner() here. CertificateWatcher should > work just fin on the network thread. I tried this but it complained something like it's not on IO thread...
https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:50: restart_deferred_action_.Run(); On 2016/03/31 23:08:33, yuweih wrote: > On 2016/03/31 22:36:15, Sergey Ulanov wrote: > > Why do you need this? > > For testing reason. We need some way to quit the message loop if the restart > action is deferred... Just post a delayed Quit task on that message loop with delay slightly larger than |delay_|? https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:56: restart_timer_->Reset(); On 2016/03/31 23:08:33, yuweih wrote: > On 2016/03/31 22:36:15, Sergey Ulanov wrote: > > I don't think you want to rest the timer here. It doesn't make sense to keep > the > > timer running while we are waiting the user to disconnect. Instead I suggest > you > > add a boolean flag that would indicate that restart is pending. So here you > will > > have > > restart_pending_ = true; > > I guess I misunderstood your last comment about always starting the timer in > inhibit mode. > > So we still want to use a flag to mark a pending restart but we should defer it > at the beginning of OnTimer instead of at the end of OnUpdate, right? Starting/resetting the timer makes sense only in response to file change notifications. I.e. you shouldn't be calling timer_->Reset() anywhere except from OnCertDirectoryChanged() https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:819: cert_watcher_.reset(new CertificateWatcher( On 2016/03/31 23:08:33, yuweih wrote: > On 2016/03/31 22:36:16, Sergey Ulanov wrote: > > CreateAuthenticatorFactory() is called every time the host is restarted, but > we > > don't want to create a new instance of CertificateWatcher every time - certs > may > > be updated while the host is being restarted and we would loose the state in > > that case. > > Currently the watcher get constructed just before the authorization starts and > destructed when the host dies. This code also resets it every time the host is restarted (without restarting the process), and we want to avoid that. > Not sure why we need to care about during the > interval of (old host died, new host authorized)... We need to care about it because restarting the host doesn't refresh the caches that NSS may hold internally. > We can keep using the same > watcher for the whole process then it will need to reset the host reference > anyways... CreateAuthenticatorFactory() is called in two case: 1. Host is being started the first time. 2. Host is being restarted, e.g. because policies have been updated. In the second case we don't want to create new instance of CertificateWatcher. So I think here you want to create the object if it didn't exist before and then then give it reference to the host, i.e.: if (!cert_watcher_) { cert_watcher_.reset(new CertificateWatcher( base::Bind(&HostProcess::OnHostRestartRequested, this))); } cert_watcher_->Start(host_->AsWeakPtr()); No Start() will be called every time host is restarted. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:822: context_->file_task_runner()->PostTask( On 2016/03/31 23:08:33, yuweih wrote: > On 2016/03/31 22:36:16, Sergey Ulanov wrote: > > I don't think you need to use file_task_runner() here. CertificateWatcher > should > > work just fin on the network thread. > > I tried this but it complained something like it's not on IO thread... Network thread is an IO thread (see https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/chro...), so it should work. Could it be that you tried it on main thread, instead of the network thread?
https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:50: restart_deferred_action_.Run(); On 2016/04/01 17:49:23, Sergey Ulanov wrote: > On 2016/03/31 23:08:33, yuweih wrote: > > On 2016/03/31 22:36:15, Sergey Ulanov wrote: > > > Why do you need this? > > > > For testing reason. We need some way to quit the message loop if the restart > > action is deferred... > > Just post a delayed Quit task on that message loop with delay slightly larger > than |delay_|? That's time dependent and sounds a little bit flaky? I am not sure how long it takes for the file watcher to notice the changes and how precise the delayed timer will be... Or we can quit the message loop with delay say 2*|delay_|? https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:56: restart_timer_->Reset(); On 2016/04/01 17:49:23, Sergey Ulanov wrote: > On 2016/03/31 23:08:33, yuweih wrote: > > On 2016/03/31 22:36:15, Sergey Ulanov wrote: > > > I don't think you want to rest the timer here. It doesn't make sense to keep > > the > > > timer running while we are waiting the user to disconnect. Instead I suggest > > you > > > add a boolean flag that would indicate that restart is pending. So here you > > will > > > have > > > restart_pending_ = true; > > > > I guess I misunderstood your last comment about always starting the timer in > > inhibit mode. > > > > So we still want to use a flag to mark a pending restart but we should defer > it > > at the beginning of OnTimer instead of at the end of OnUpdate, right? > > Starting/resetting the timer makes sense only in response to file change > notifications. I.e. you shouldn't be calling timer_->Reset() anywhere except > from OnCertDirectoryChanged() Okay... https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:819: cert_watcher_.reset(new CertificateWatcher( On 2016/04/01 17:49:23, Sergey Ulanov wrote: > On 2016/03/31 23:08:33, yuweih wrote: > > On 2016/03/31 22:36:16, Sergey Ulanov wrote: > > > CreateAuthenticatorFactory() is called every time the host is restarted, but > > we > > > don't want to create a new instance of CertificateWatcher every time - certs > > may > > > be updated while the host is being restarted and we would loose the state in > > > that case. > > > > Currently the watcher get constructed just before the authorization starts and > > destructed when the host dies. > > This code also resets it every time the host is restarted (without restarting > the process), and we want to avoid that. > > > > Not sure why we need to care about during the > > interval of (old host died, new host authorized)... > > We need to care about it because restarting the host doesn't refresh the caches > that NSS may hold internally. > > > > We can keep using the same > > watcher for the whole process then it will need to reset the host reference > > anyways... > > CreateAuthenticatorFactory() is called in two case: > 1. Host is being started the first time. > 2. Host is being restarted, e.g. because policies have been updated. > > In the second case we don't want to create new instance of CertificateWatcher. > So I think here you want to create the object if it didn't exist before and then > then give it reference to the host, i.e.: > > if (!cert_watcher_) { > cert_watcher_.reset(new CertificateWatcher( > base::Bind(&HostProcess::OnHostRestartRequested, this))); > } > cert_watcher_->Start(host_->AsWeakPtr()); > > No Start() will be called every time host is restarted. Okay. I guess I had mixed up the concept of HostProcess and the host... https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:822: context_->file_task_runner()->PostTask( On 2016/04/01 17:49:23, Sergey Ulanov wrote: > On 2016/03/31 23:08:33, yuweih wrote: > > On 2016/03/31 22:36:16, Sergey Ulanov wrote: > > > I don't think you need to use file_task_runner() here. CertificateWatcher > > should > > > work just fin on the network thread. > > > > I tried this but it complained something like it's not on IO thread... > > Network thread is an IO thread (see > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/chro...), > so it should work. Could it be that you tried it on main thread, instead of the > network thread? Interesting... I tried context_->network_task_runner()->PostTask( FROM_HERE, base::Bind(&CertificateWatcher::Start, base::Unretained(cert_watcher_.get()))); Then I got [0401/112513:FATAL:thread_restrictions.cc(38)] Function marked as IO-only was called from a thread that disallows IO! If this thread really should be allowed to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() in this thread's startup.
https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:822: context_->file_task_runner()->PostTask( On 2016/04/01 17:49:23, Sergey Ulanov wrote: > On 2016/03/31 23:08:33, yuweih wrote: > > On 2016/03/31 22:36:16, Sergey Ulanov wrote: > > > I don't think you need to use file_task_runner() here. CertificateWatcher > > should > > > work just fin on the network thread. > > > > I tried this but it complained something like it's not on IO thread... > > Network thread is an IO thread (see > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/chro...), > so it should work. Could it be that you tried it on main thread, instead of the > network thread? Line 114: network_task_runner->PostTask(FROM_HERE, base::Bind(&DisallowBlockingOperations)); And: void DisallowBlockingOperations() { base::ThreadRestrictions::SetIOAllowed(false); base::ThreadRestrictions::DisallowWaiting(); } Looks like we have explicitly disallowed IO's on network thread...
https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:50: restart_deferred_action_.Run(); On 2016/04/01 18:28:04, yuweih wrote: > That's time dependent and sounds a little bit flaky? I am not sure how long it > takes for the file watcher to notice the changes and how precise the delayed > timer will be... Or we can quit the message loop with delay say 2*|delay_|? The test can work something like this: 1. OnClientConnected(); 2. Update watch path. 3. Post quit delayed task for delay_ + 0.1s 4. message_loop_.Run(); 5. Verify that the restart closure wasn't called. 6. OnClientDisconnected(); 7. If the restart closure still hasn't been called then message_loop_.Run() which quits when it's called. I don't think this will be flaky. Even if the watcher doesn't notice the change in step 4 it should notice it in 7. The test may sometimes still succeed when restart is not inhibited properly, but I think that's acceptable. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:822: context_->file_task_runner()->PostTask( On 2016/04/01 18:28:04, yuweih wrote: > Interesting... I tried > > context_->network_task_runner()->PostTask( > FROM_HERE, base::Bind(&CertificateWatcher::Start, > base::Unretained(cert_watcher_.get()))); > > Then I got > > [0401/112513:FATAL:thread_restrictions.cc(38)] Function marked as IO-only was > called from a thread that disallows IO! If this thread really should be allowed > to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() in > this thread's startup. I see. This error message is somewhat wrong. Network thread is an IO thread, but we disallow _blocking_ IO. Looks like FilePathWatcher needs to use blocking IO and that's the reason we cannot run it on network_thread. Sorry I assumed it doesn't require blocking IO. So given that FilePathWatcher needs to run on the file thread then I think it's better if CertificateWatcher was responsible of posting the task to the file thread. Pass the file_task_runner() and store the reference there. Then internally it should use that task_runner to call FilePathWatcher::Watch(). Whenever it receives notification from the watcher it will need to post a task back to the network thread to handle it there. In the current version of this CL CertificateWatcher uses inhibit flag on two threads and this is not safe (timer works on the file thread, while the host notifications are handled on the network thread). Keep as much logic as possible on the network thread and use file thread only to receive notification from the file watcher. Let's discuss this offline if what I wrote above doesn't make sense.
https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:50: restart_deferred_action_.Run(); On 2016/04/01 20:11:00, Sergey Ulanov wrote: > On 2016/04/01 18:28:04, yuweih wrote: > > That's time dependent and sounds a little bit flaky? I am not sure how long it > > takes for the file watcher to notice the changes and how precise the delayed > > timer will be... Or we can quit the message loop with delay say 2*|delay_|? > > The test can work something like this: > 1. OnClientConnected(); > 2. Update watch path. > 3. Post quit delayed task for delay_ + 0.1s > 4. message_loop_.Run(); > 5. Verify that the restart closure wasn't called. > 6. OnClientDisconnected(); > 7. If the restart closure still hasn't been called then message_loop_.Run() > which quits when it's called. > > I don't think this will be flaky. Even if the watcher doesn't notice the change > in step 4 it should notice it in 7. The test may sometimes still succeed when > restart is not inhibited properly, but I think that's acceptable. Okay... Sounds reasonable https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:822: context_->file_task_runner()->PostTask( On 2016/04/01 20:11:00, Sergey Ulanov wrote: > On 2016/04/01 18:28:04, yuweih wrote: > > Interesting... I tried > > > > context_->network_task_runner()->PostTask( > > FROM_HERE, base::Bind(&CertificateWatcher::Start, > > base::Unretained(cert_watcher_.get()))); > > > > Then I got > > > > [0401/112513:FATAL:thread_restrictions.cc(38)] Function marked as IO-only was > > called from a thread that disallows IO! If this thread really should be > allowed > > to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() > in > > this thread's startup. > > I see. This error message is somewhat wrong. Network thread is an IO thread, but > we disallow _blocking_ IO. Looks like FilePathWatcher needs to use blocking IO > and that's the reason we cannot run it on network_thread. Sorry I assumed it > doesn't require blocking IO. > > So given that FilePathWatcher needs to run on the file thread then I think it's > better if CertificateWatcher was responsible of posting the task to the file > thread. Pass the file_task_runner() and store the reference there. Then > internally it should use that task_runner to call FilePathWatcher::Watch(). > Whenever it receives notification from the watcher it will need to post a task > back to the network thread to handle it there. > > In the current version of this CL CertificateWatcher uses inhibit flag on two > threads and this is not safe (timer works on the file thread, while the host > notifications are handled on the network thread). Keep as much logic as possible > on the network thread and use file thread only to receive notification from the > file watcher. > > Let's discuss this offline if what I wrote above doesn't make sense. That sounds like a problem... I think I can start the timer on the network thread and always post task to network if we need to reset the timer, then now the inhibit flag will only be accessed on the network thread.
Patchset #4 (id:80001) has been deleted
Uploaded patch 4. PTAL https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:17: // This is to repeating restarts when continuous writes to the database occur. On 2016/03/31 22:36:15, Sergey Ulanov wrote: > s/to repeating/to avoid repeating/ Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:21: const base::BasePathKey kNssBasePath = base::DIR_HOME; On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Don't need this. Just pass base::DIR_HOME when calling PathService::Get(). Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:30: base::PathService::Get(kNssBasePath, &nss_watch_path_); On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Verify that you get correct result from here. If we don't then I think we want > to log the error and keep the watcher disabled. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:31: nss_watch_path_ = nss_watch_path_.AppendASCII( On 2016/03/31 22:36:15, Sergey Ulanov wrote: > call it watch_path_ instead of nss_watch_path_ Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:32: std::string(kNssCertDirectoryPath)); On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Don't need std::string(). Compiler will convert it implicitly. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:50: restart_deferred_action_.Run(); On 2016/04/01 20:11:00, Sergey Ulanov wrote: > On 2016/04/01 18:28:04, yuweih wrote: > > That's time dependent and sounds a little bit flaky? I am not sure how long it > > takes for the file watcher to notice the changes and how precise the delayed > > timer will be... Or we can quit the message loop with delay say 2*|delay_|? > > The test can work something like this: > 1. OnClientConnected(); > 2. Update watch path. > 3. Post quit delayed task for delay_ + 0.1s > 4. message_loop_.Run(); > 5. Verify that the restart closure wasn't called. > 6. OnClientDisconnected(); > 7. If the restart closure still hasn't been called then message_loop_.Run() > which quits when it's called. > > I don't think this will be flaky. Even if the watcher doesn't notice the change > in step 4 it should notice it in 7. The test may sometimes still succeed when > restart is not inhibited properly, but I think that's acceptable. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:56: restart_timer_->Reset(); On 2016/03/31 22:36:15, Sergey Ulanov wrote: > I don't think you want to rest the timer here. It doesn't make sense to keep the > timer running while we are waiting the user to disconnect. Instead I suggest you > add a boolean flag that would indicate that restart is pending. So here you will > have > restart_pending_ = true; Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:78: auto raw_timer = On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Don't need this variable: > restart_timer.reset(new base::DelayTimer(...)); Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:80: delay_, On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Please clang-format this code. It will move this line to the previous one. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:88: void CertificateWatcher::SetInhibit(bool inhibit) { inhibit_mode_ = inhibit; } On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Don't need this method. Just set the variable in OnClientConnected() and > OnClientDisconnected() Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:96: } On 2016/03/31 22:36:15, Sergey Ulanov wrote: > if (restart_pending_) { > restart_action_.Run(); > } Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:98: void CertificateWatcher::SetDelay(const base::TimeDelta& delay) { On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Call it SetDelayForTests. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.cc:106: void CertificateWatcher::SetRestartDeferredAction( On 2016/03/31 22:36:15, Sergey Ulanov wrote: > Why do you need this? Removed. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:20: // This class watches the NSS database and kills the host when a change of the On 2016/03/31 22:36:16, Sergey Ulanov wrote: > s/NSS database/certificate database/ Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:40: On 2016/03/31 22:36:16, Sergey Ulanov wrote: > removw this empty line. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:47: // Used for tests. No change will be made if the watcher has already started On 2016/03/31 22:36:15, Sergey Ulanov wrote: > use ForTests suffix for test-only methods please. Also add DCHECK() to enforce > that these methods are called only in proper state. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:60: bool inhibit_mode_ = false; On 2016/03/31 22:36:15, Sergey Ulanov wrote: > move this below. Normally the values passed to the constructor should proceed > everything else. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher.h:78: base::Closure restart_action_; On 2016/03/31 22:36:16, Sergey Ulanov wrote: > move this after monitor_. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:18: const char kWatchFileName[] = "testfile.txt"; On 2016/03/31 22:36:16, Sergey Ulanov wrote: > nit: add empty line. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/linux/cer... remoting/host/linux/certificate_watcher_unittest.cc:100: watcher_.SetInhibit(true); On 2016/03/31 22:36:16, Sergey Ulanov wrote: > Call OnClientConnected() and OnClientDisconnected() here instead of SetInhibit() > and remove SetInhibit(). Otherwise the test doesn't verify that > OnClientDisconnected() is handled correctly. Done. https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/1838313002/diff/60001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:819: cert_watcher_.reset(new CertificateWatcher( On 2016/03/31 22:36:16, Sergey Ulanov wrote: > CreateAuthenticatorFactory() is called every time the host is restarted, but we > don't want to create a new instance of CertificateWatcher every time - certs may > be updated while the host is being restarted and we would loose the state in > that case. Done.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:5: #include "certificate_watcher.h" Specify pull path here please. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:29: network_runner_(network_runner), Don't need to pass it as parameter explicitly. Instead use base::ThreadTaskRunnerHandle::Get() to get task runner for the current thread. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:30: io_runner_(io_runner), Call this io_task_runner_. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:36: nit: remove this empty line. Otherwise the following line looks as if it's not related to the code above. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:37: cert_watch_path_ = cert_watch_path_.AppendASCII(kCertDirectoryPath); nit: incorrect indentation. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:76: void CertificateWatcher::Start() { Do you really need this separate from the constructor? I suggest moving all this code to the constructor. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:78: LOG(ERROR) << "Failed to start watching. " This essentially duplicates the error you have in the constructor. You can avoid this by moving all code to the constructor. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:84: &CertificateWatcher::StartWatcherOnIo, base::Unretained(this))); base::Unretained() is not safe here. CertificateWatcher may be destroyed by the time this task gets executed on the IO thread. Instead you can create FilePathWatcher here and post a task directly for the Watch() method: file_watcher_.reset(new base::FilePathWatcher()); io_runner_->PostTask(FROM_HERE, base::Bind( &FilePathWatcher::Watch, base::Unretained(file_watcher.get()), cert_watch_path_, true, base::Bind(&OnCertDirectoryChanged)); Then from the destructor post a task on io thread to delete the watcher: io_runner_->DeleteSoon(FROM_HERE, file_watcher_.release()); https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:87: &CertificateWatcher::StartTimerOnNetwork, base::Unretained(this))); Why do you need to post a separate task here? network_runner_ must be the same thread on which this code is executed. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:99: base::Unretained(this))); It's not safe to use base::Unretained() here. OnCertDirectoryChanged() is called on the IO thread, while CertificateWatcher is destroyed on the network thread. I suggest you make OnCertDirectoryChanged a static method that gets a weak pointer for the CertficateWatcher to post the task back to the network thread: OnCertDirectoryChanged(scoped_refptr<TaskRunner> network_task_runner, base::WeakPtr<CertificateWatcher> cert_watcher, const base::FilePath& path, bool error) { network_task_runner->PostTask( FROM_HERE, base::Bind(&CertificateWatcher::DirectoryChanged, cert_watcher, path, error)); } You will need to bind the first two parameters for the callback passed to FilePathWatcher::Watch(). https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:117: if (monitor) { I don't think this you need this condition: monitor must exist when SetMonitor() is called. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:46: bool Started() const; call it is_started()
https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:5: #include "certificate_watcher.h" On 2016/04/04 19:29:37, Sergey Ulanov wrote: > Specify pull path here please. full path I guess? https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:87: &CertificateWatcher::StartTimerOnNetwork, base::Unretained(this))); On 2016/04/04 19:29:37, Sergey Ulanov wrote: > Why do you need to post a separate task here? network_runner_ must be the same > thread on which this code is executed. I was thinking it's the main thread calling this function... Looks like I can clean up the code a little bit. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:46: bool Started() const; On 2016/04/04 19:29:37, Sergey Ulanov wrote: > call it is_started() Is it Chromium's convention to name getters using underscore-separated naming?
https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:5: #include "certificate_watcher.h" On 2016/04/04 21:40:24, yuweih wrote: > On 2016/04/04 19:29:37, Sergey Ulanov wrote: > > Specify pull path here please. > > full path I guess? yes https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:46: bool Started() const; On 2016/04/04 21:40:24, yuweih wrote: > On 2016/04/04 19:29:37, Sergey Ulanov wrote: > > call it is_started() > > Is it Chromium's convention to name getters using underscore-separated naming? See https://google.github.io/styleguide/cppguide.html#Function_Names
https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:76: void CertificateWatcher::Start() { On 2016/04/04 19:29:37, Sergey Ulanov wrote: > Do you really need this separate from the constructor? I suggest moving all this > code to the constructor. It's basically to allow unit tests to override parameters before starting the test. If we move this to the constructor then we need to have separate constructor for testing...
https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:76: void CertificateWatcher::Start() { On 2016/04/04 21:45:20, yuweih wrote: > On 2016/04/04 19:29:37, Sergey Ulanov wrote: > > Do you really need this separate from the constructor? I suggest moving all > this > > code to the constructor. > > It's basically to allow unit tests to override parameters before starting the > test. If we move this to the constructor then we need to have separate > constructor for testing... I see. Then maybe make it return a bool value (false in case it fails). Then you can make is_started() private or remove it completely.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/120001
Patch 5 PTAL https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:5: #include "certificate_watcher.h" On 2016/04/04 19:29:37, Sergey Ulanov wrote: > Specify pull path here please. Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:29: network_runner_(network_runner), On 2016/04/04 19:29:37, Sergey Ulanov wrote: > Don't need to pass it as parameter explicitly. Instead use > base::ThreadTaskRunnerHandle::Get() to get task runner for the current thread. Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:30: io_runner_(io_runner), On 2016/04/04 19:29:37, Sergey Ulanov wrote: > Call this io_task_runner_. Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:36: On 2016/04/04 19:29:37, Sergey Ulanov wrote: > nit: remove this empty line. Otherwise the following line looks as if it's not > related to the code above. Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:37: cert_watch_path_ = cert_watch_path_.AppendASCII(kCertDirectoryPath); On 2016/04/04 19:29:37, Sergey Ulanov wrote: > nit: incorrect indentation. Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:76: void CertificateWatcher::Start() { On 2016/04/04 21:47:15, Sergey Ulanov wrote: > On 2016/04/04 21:45:20, yuweih wrote: > > On 2016/04/04 19:29:37, Sergey Ulanov wrote: > > > Do you really need this separate from the constructor? I suggest moving all > > this > > > code to the constructor. > > > > It's basically to allow unit tests to override parameters before starting the > > test. If we move this to the constructor then we need to have separate > > constructor for testing... > > I see. Then maybe make it return a bool value (false in case it fails). Then you > can make is_started() private or remove it completely. Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:84: &CertificateWatcher::StartWatcherOnIo, base::Unretained(this))); On 2016/04/04 19:29:37, Sergey Ulanov wrote: > base::Unretained() is not safe here. CertificateWatcher may be destroyed by the > time this task gets executed on the IO thread. Instead you can create > FilePathWatcher here and post a task directly for the Watch() method: > > file_watcher_.reset(new base::FilePathWatcher()); > io_runner_->PostTask(FROM_HERE, base::Bind( > &FilePathWatcher::Watch, base::Unretained(file_watcher.get()), > cert_watch_path_, true, base::Bind(&OnCertDirectoryChanged)); > > Then from the destructor post a task on io thread to delete the watcher: > io_runner_->DeleteSoon(FROM_HERE, file_watcher_.release()); Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:87: &CertificateWatcher::StartTimerOnNetwork, base::Unretained(this))); On 2016/04/04 19:29:37, Sergey Ulanov wrote: > Why do you need to post a separate task here? network_runner_ must be the same > thread on which this code is executed. Not posting task now https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:99: base::Unretained(this))); On 2016/04/04 19:29:37, Sergey Ulanov wrote: > It's not safe to use base::Unretained() here. OnCertDirectoryChanged() is called > on the IO thread, while CertificateWatcher is destroyed on the network thread. I > suggest you make OnCertDirectoryChanged a static method that gets a weak pointer > for the CertficateWatcher > to post the task back to the network thread: > > OnCertDirectoryChanged(scoped_refptr<TaskRunner> network_task_runner, > base::WeakPtr<CertificateWatcher> cert_watcher, > const base::FilePath& path, > bool error) { > network_task_runner->PostTask( > FROM_HERE, > base::Bind(&CertificateWatcher::DirectoryChanged, cert_watcher, > path, error)); > } > > You will need to bind the first two parameters for the callback passed to > FilePathWatcher::Watch(). Done. https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:117: if (monitor) { On 2016/04/04 19:29:37, Sergey Ulanov wrote: > I don't think this you need this condition: monitor must exist when SetMonitor() > is called. Changed to DCHECK https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/100001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:46: bool Started() const; On 2016/04/04 19:29:37, Sergey Ulanov wrote: > call it is_started() Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/120001
Almost there! I just have a few more comments about style and formatting. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:29: network_task_runner_(base::ThreadTaskRunnerHandle::Get()), nit: rename this to caller_task_runner_, to avoid assumptions about the thread on which this class is created. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:33: LOG(ERROR) << "Failed to get path of the home directory. "; 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 https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:39: void CertificateWatcher::OnCertDirectoryChanged( add "// static" comment above this line to make it clear that this method is static. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path, One argument per line please (run clang-format) https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:74: CertificateWatcher::~CertificateWatcher() { Move this after the constructor. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:74: CertificateWatcher::~CertificateWatcher() { Add DCHECK(network_task_runner_->BelongsToCurrentThread()); to make sure it's called on the right thread. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:78: DCHECK(io_task_runner_->DeleteSoon(FROM_HERE, file_watcher_.release())); Don't put any statements with side-effects in [D]CHECK(). It should be safe to ignore DeleteSoon() result here. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:83: bool CertificateWatcher::Start() { Reorder methods in this file to match the declaration order in the header. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:86: if (cert_watch_path_.empty()) { Given that base::PathService::Get() should never fail this can be replaced with DCHECK(). and you can remove EmptyPathFailToStart test. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:109: if (monitor_) { Add DCHECK(is_started()); https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:112: DCHECK(monitor); 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 https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:118: if (!is_started()) { Don't need this if you add DCHECK(is_started()); https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:126: if (!is_started()) { don't need this.
https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path, On 2016/04/05 01:14:32, Sergey Ulanov wrote: > One argument per line please (run clang-format) I think clang-format actually moved |path| to the line above... https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:78: DCHECK(io_task_runner_->DeleteSoon(FROM_HERE, file_watcher_.release())); On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Don't put any statements with side-effects in [D]CHECK(). It should be safe to > ignore DeleteSoon() result here. Would we do `bool result = ...; DCHECK(result);` or is it just better to not DCHECK code with side effect at all...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path, On 2016/04/05 01:25:36, yuweih wrote: > On 2016/04/05 01:14:32, Sergey Ulanov wrote: > > One argument per line please (run clang-format) > > I think clang-format actually moved |path| to the line above... maybe it's somehow configured incorrectly on your machine? How do you run it? Just running "git cl format" should do the right thing. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:78: DCHECK(io_task_runner_->DeleteSoon(FROM_HERE, file_watcher_.release())); On 2016/04/05 01:25:36, yuweih wrote: > On 2016/04/05 01:14:32, Sergey Ulanov wrote: > > Don't put any statements with side-effects in [D]CHECK(). It should be safe to > > ignore DeleteSoon() result here. > > Would we do `bool result = ...; DCHECK(result);` or is it just better to not > DCHECK code with side effect at all... 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.
On 2016/04/05 18:35:21, Sergey Ulanov wrote: > https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... > File remoting/host/linux/certificate_watcher.cc (right): > > https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... > remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> > watcher_, const base::FilePath& path, > On 2016/04/05 01:25:36, yuweih wrote: > > On 2016/04/05 01:14:32, Sergey Ulanov wrote: > > > One argument per line please (run clang-format) > > > > I think clang-format actually moved |path| to the line above... > > maybe it's somehow configured incorrectly on your machine? How do you run it? > Just running "git cl format" should do the right thing. > > https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... > remoting/host/linux/certificate_watcher.cc:78: > DCHECK(io_task_runner_->DeleteSoon(FROM_HERE, file_watcher_.release())); > On 2016/04/05 01:25:36, yuweih wrote: > > On 2016/04/05 01:14:32, Sergey Ulanov wrote: > > > Don't put any statements with side-effects in [D]CHECK(). It should be safe > to > > > ignore DeleteSoon() result here. > > > > Would we do `bool result = ...; DCHECK(result);` or is it just better to not > > DCHECK code with side effect at all... > > 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. I see...
https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path, 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?
https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path, On 2016/04/05 19:15:51, yuweih wrote: > 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? 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 .
https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:78: DCHECK(io_task_runner_->DeleteSoon(FROM_HERE, file_watcher_.release())); On 2016/04/05 18:35:21, Sergey Ulanov wrote: > On 2016/04/05 01:25:36, yuweih wrote: > > On 2016/04/05 01:14:32, Sergey Ulanov wrote: > > > Don't put any statements with side-effects in [D]CHECK(). It should be safe > to > > > ignore DeleteSoon() result here. > > > > Would we do `bool result = ...; DCHECK(result);` or is it just better to not > > DCHECK code with side effect at all... > > 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. 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?
PTAL https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:29: network_task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2016/04/05 01:14:32, Sergey Ulanov wrote: > nit: rename this to caller_task_runner_, to avoid assumptions about the thread > on which this class is created. Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:33: LOG(ERROR) << "Failed to get path of the home directory. "; On 2016/04/05 01:14:32, Sergey Ulanov wrote: > 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 Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path, On 2016/04/05 19:34:46, Sergey Ulanov wrote: > On 2016/04/05 19:15:51, yuweih wrote: > > 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? > > 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 > . Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:74: CertificateWatcher::~CertificateWatcher() { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Add DCHECK(network_task_runner_->BelongsToCurrentThread()); to make sure it's > called on the right thread. Checked in Stop() https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:83: bool CertificateWatcher::Start() { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Reorder methods in this file to match the declaration order in the header. Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:86: if (cert_watch_path_.empty()) { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Given that base::PathService::Get() should never fail this can be replaced with > DCHECK(). and you can remove EmptyPathFailToStart test. Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:109: if (monitor_) { On 2016/04/05 01:14:31, Sergey Ulanov wrote: > Add DCHECK(is_started()); Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:112: DCHECK(monitor); On 2016/04/05 01:14:32, Sergey Ulanov wrote: > 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 Removed https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:118: if (!is_started()) { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Don't need this if you add DCHECK(is_started()); Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:126: if (!is_started()) { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > don't need this. Done.
PTAL https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:29: network_task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2016/04/05 01:14:32, Sergey Ulanov wrote: > nit: rename this to caller_task_runner_, to avoid assumptions about the thread > on which this class is created. Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:33: LOG(ERROR) << "Failed to get path of the home directory. "; On 2016/04/05 01:14:32, Sergey Ulanov wrote: > 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 Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:41: base::WeakPtr<CertificateWatcher> watcher_, const base::FilePath& path, On 2016/04/05 19:34:46, Sergey Ulanov wrote: > On 2016/04/05 19:15:51, yuweih wrote: > > 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? > > 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 > . Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:74: CertificateWatcher::~CertificateWatcher() { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Add DCHECK(network_task_runner_->BelongsToCurrentThread()); to make sure it's > called on the right thread. Checked in Stop() https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:83: bool CertificateWatcher::Start() { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Reorder methods in this file to match the declaration order in the header. Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:86: if (cert_watch_path_.empty()) { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Given that base::PathService::Get() should never fail this can be replaced with > DCHECK(). and you can remove EmptyPathFailToStart test. Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:109: if (monitor_) { On 2016/04/05 01:14:31, Sergey Ulanov wrote: > Add DCHECK(is_started()); Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:112: DCHECK(monitor); On 2016/04/05 01:14:32, Sergey Ulanov wrote: > 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 Removed https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:118: if (!is_started()) { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > Don't need this if you add DCHECK(is_started()); Done. https://codereview.chromium.org/1838313002/diff/120001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:126: if (!is_started()) { On 2016/04/05 01:14:32, Sergey Ulanov wrote: > don't need this. Done.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/140001
https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:11: #include "base/message_loop/message_loop.h" Don't need this include https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:42: bool CertificateWatcher::Start() { No longer need to return bool, as it cannot fail anymore. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:130: if (error || path != cert_watch_path_) { The watcher watches a directory, but here |path| may point to a file in that directory, so I'm not sure that second condition is correct. Why do you need it? I suggest removing it. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:14: #include "remoting/base/auto_thread_task_runner.h" Don't need this. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:30: public base::SupportsWeakPtr<CertificateWatcher> { Please use WeakPtrFactory<> instead of SupportsWeakPtr<> https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:39: // network thread if necessary. Suggest rewording: // Starts watching file changes calling |restart_action| when the // host should be restarted. No need to mention threads since from the consumer's point of view this is a single-thread class. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:40: // returns true if the watcher has successfully started. Don't need to return anything as it never fails. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:49: void Stop(); I don't think you need this method. For the tests just allocate and destroy the watcher on the heap to control when it frees the resources. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:69: // Callback passed to |file_watcher_|. Run in IO thread. s/Run/Runs/
https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:130: if (error || path != cert_watch_path_) { On 2016/04/05 21:12:55, Sergey Ulanov wrote: > The watcher watches a directory, but here |path| may point to a file in that > directory, so I'm not sure that second condition is correct. Why do you need it? > I suggest removing it. Looks like |path| will never point to something other than the directory even if the file being changed is inside it. I guess the path param is useful when binding multiple file watchers but not very useful in this case. Maybe just DCHECK it?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:39: // network thread if necessary. On 2016/04/05 21:12:56, Sergey Ulanov wrote: > Suggest rewording: > // Starts watching file changes calling |restart_action| when the > // host should be restarted. > > No need to mention threads since from the consumer's point of view this is a > single-thread class. This comment may be unnecessary information for the consumer but I may disagree that a consumer can view this class as a single-thread class since the constructor takes in a task runner and the thread must be running when cleaning up the file watcher...
PTAL https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.cc (right): https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:11: #include "base/message_loop/message_loop.h" On 2016/04/05 21:12:56, Sergey Ulanov wrote: > Don't need this include Done. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.cc:42: bool CertificateWatcher::Start() { On 2016/04/05 21:12:55, Sergey Ulanov wrote: > No longer need to return bool, as it cannot fail anymore. Done. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:14: #include "remoting/base/auto_thread_task_runner.h" On 2016/04/05 21:12:56, Sergey Ulanov wrote: > Don't need this. Done. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:30: public base::SupportsWeakPtr<CertificateWatcher> { On 2016/04/05 21:12:56, Sergey Ulanov wrote: > Please use WeakPtrFactory<> instead of SupportsWeakPtr<> Done. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:39: // network thread if necessary. On 2016/04/05 21:12:56, Sergey Ulanov wrote: > Suggest rewording: > // Starts watching file changes calling |restart_action| when the > // host should be restarted. > > No need to mention threads since from the consumer's point of view this is a > single-thread class. Done. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:40: // returns true if the watcher has successfully started. On 2016/04/05 21:12:56, Sergey Ulanov wrote: > Don't need to return anything as it never fails. Done. https://codereview.chromium.org/1838313002/diff/140001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:69: // Callback passed to |file_watcher_|. Run in IO thread. On 2016/04/05 21:12:56, Sergey Ulanov wrote: > s/Run/Runs/ Done.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please update CL description to provide more details about why this change is needed. Also don't need to refer 'b/21106876' in the description. I have a few more comments about the unittests. LGTM after they are addressed. Thank you for your patience getting all of my nits addressed! https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:19: // This class watches the cert database and kills the host when a change of the Suggest rewording the first sentence: // This class watches the cert database and notifies when the // host needs to be restarted. This class doesn't stop the host itself. It also doesn't notify about changes immediately. https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:14: #include "remoting/base/auto_thread.h" Don't need this. https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:38: RunAndWait(); You can just call RunUntilIdle() here - that will guarantee that the DeleteSoon() task is executed. Also see my comments below about RunLoop. I.e. this should be base::RunLoop().RunUntilIdle(); https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:45: message_loop_.Run(); MessageLoop::Run() is deprecated. New code is supposed to use base::RunLoop to run message loop. So change this to base::RunLoop().Run(); https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:56: message_loop_.Run(); Use RunLoop here as well. This function will be simpler with RunLoop: base::RunLoop run_loop; task_runner_->PostDelayedTask( FROM_HERE, run_loop.QuitClosure(), loop_wait_); run_loop.Run(); https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:94: CertificateWatcher* watcher_; use scoped_ptr<> here to indicate ownership
Description was changed from ========== Kill the host when the NSS certificate changes Will defer suicide when the host is connected to the client b/21106876 BUG=598819 ========== to ========== When using third-party authentication the host may need to use client certificates. The host currently doesn't handle the case that the certificate file gets updated when the host is running, in which case later session connection will fail due to outdated certificate. This patch schedules host-restart when the cert folder (~/.pki/nssdb) is changed and restart the host when no client connection is established. BUG=598819 ==========
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Thanks for reviewing my code for two weeks :) https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher.h (right): https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher.h:19: // This class watches the cert database and kills the host when a change of the On 2016/04/06 21:04:06, Sergey Ulanov wrote: > Suggest rewording the first sentence: > // This class watches the cert database and notifies when the > // host needs to be restarted. > > This class doesn't stop the host itself. It also doesn't notify about changes > immediately. Done. https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:14: #include "remoting/base/auto_thread.h" On 2016/04/06 21:04:06, Sergey Ulanov wrote: > Don't need this. removed. https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:38: RunAndWait(); On 2016/04/06 21:04:06, Sergey Ulanov wrote: > You can just call RunUntilIdle() here - that will guarantee that the > DeleteSoon() task is executed. Also see my comments below about RunLoop. I.e. > this should be base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:45: message_loop_.Run(); On 2016/04/06 21:04:06, Sergey Ulanov wrote: > MessageLoop::Run() is deprecated. New code is supposed to use base::RunLoop to > run message loop. So change this to > base::RunLoop().Run(); Done. https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:56: message_loop_.Run(); On 2016/04/06 21:04:06, Sergey Ulanov wrote: > Use RunLoop here as well. This function will be simpler with RunLoop: > base::RunLoop run_loop; > task_runner_->PostDelayedTask( > FROM_HERE, run_loop.QuitClosure(), loop_wait_); > run_loop.Run(); Done. https://codereview.chromium.org/1838313002/diff/160001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:94: CertificateWatcher* watcher_; On 2016/04/06 21:04:06, Sergey Ulanov wrote: > use scoped_ptr<> here to indicate ownership Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/180001
lgtm https://codereview.chromium.org/1838313002/diff/180001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/180001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:97: base::Closure quit_loop_closure; this should be quit_loop_closure with _ at the end.
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
https://codereview.chromium.org/1838313002/diff/180001/remoting/host/linux/ce... File remoting/host/linux/certificate_watcher_unittest.cc (right): https://codereview.chromium.org/1838313002/diff/180001/remoting/host/linux/ce... remoting/host/linux/certificate_watcher_unittest.cc:97: base::Closure quit_loop_closure; On 2016/04/07 00:42:19, Sergey Ulanov wrote: > this should be quit_loop_closure with _ at the end. Oops typo...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yuweih@chromium.org
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/1838313002/#ps200001 (title: "Fix small typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838313002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838313002/200001
Message was sent while issue was closed.
Description was changed from ========== When using third-party authentication the host may need to use client certificates. The host currently doesn't handle the case that the certificate file gets updated when the host is running, in which case later session connection will fail due to outdated certificate. This patch schedules host-restart when the cert folder (~/.pki/nssdb) is changed and restart the host when no client connection is established. BUG=598819 ========== to ========== When using third-party authentication the host may need to use client certificates. The host currently doesn't handle the case that the certificate file gets updated when the host is running, in which case later session connection will fail due to outdated certificate. This patch schedules host-restart when the cert folder (~/.pki/nssdb) is changed and restart the host when no client connection is established. BUG=598819 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== When using third-party authentication the host may need to use client certificates. The host currently doesn't handle the case that the certificate file gets updated when the host is running, in which case later session connection will fail due to outdated certificate. This patch schedules host-restart when the cert folder (~/.pki/nssdb) is changed and restart the host when no client connection is established. BUG=598819 ========== to ========== When using third-party authentication the host may need to use client certificates. The host currently doesn't handle the case that the certificate file gets updated when the host is running, in which case later session connection will fail due to outdated certificate. This patch schedules host-restart when the cert folder (~/.pki/nssdb) is changed and restart the host when no client connection is established. BUG=598819 Committed: https://crrev.com/7821527cafe2199a3603d26daae639c621602a2f Cr-Commit-Position: refs/heads/master@{#385801} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7821527cafe2199a3603d26daae639c621602a2f Cr-Commit-Position: refs/heads/master@{#385801} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
