Support change detection and reloading in ConfigDirPolicyProvider.

Support change detection and reloading in ConfigDirPolicyProvider.

BUG=52418
TEST=unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56832

[Mattias Nissler (ping if slow), 2010-08-18 17:10:47]

Danno, here's the linux policy reload CL so you have something to do when you're
back tomorrow :-D

Note this code has http://codereview.chromium.org/3184010/show as a prerequisite
(not committed yet).

[Paweł Hajdan Jr., 2010-08-18 18:11:08]

Drive-by with a test comment. Do not commit without my explicit "LGTM".

If you want to make sure this won't hang, consider converting it to a
browser_test which has a bullet-proof timeout. Alternatively, we should make
some common place for the timeouts, like chrome/test/test_timeouts, and make it
configurable via command-line flags.

http://codereview.chromium.org/3124025/diff/2001/3003
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/3124025/diff/2001/3003#newcode131
chrome/browser/policy/config_dir_policy_provider_unittest.cc:131:
kWaitForRefreshTimeMilliseconds);
Please do not introduce yet another timeout.

[Mattias Nissler (ping if slow), 2010-08-18 18:26:18]

On 2010/08/18 18:11:08, Paweł Hajdan Jr. wrote:
> Drive-by with a test comment. Do not commit without my explicit "LGTM".
> 
> If you want to make sure this won't hang, consider converting it to a
> browser_test which has a bullet-proof timeout. Alternatively, we should make
> some common place for the timeouts, like chrome/test/test_timeouts, and make
it
> configurable via command-line flags.
> 
> http://codereview.chromium.org/3124025/diff/2001/3003
> File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):
> 
> http://codereview.chromium.org/3124025/diff/2001/3003#newcode131
> chrome/browser/policy/config_dir_policy_provider_unittest.cc:131:
> kWaitForRefreshTimeMilliseconds);
> Please do not introduce yet another timeout.

I've disabled the delay and removed the timeout, it's not subject of this test
anyway. Are you OK with that solution?

[Paweł Hajdan Jr., 2010-08-18 18:28:44]

Code I commented in the drive-by LGTM.

[Mattias Nissler (ping if slow), 2010-08-19 08:21:30]

Danno, please review :)

[danno, 2010-08-19 12:40:31]

here you go!

http://codereview.chromium.org/3124025/diff/8001/9001
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/3124025/diff/8001/9001#newcode45
chrome/browser/policy/config_dir_policy_provider.cc:45: void
PolicyDirLoader::Stop() {
Is this really thread safe? If the destructor of PolicyDirLoader is not called
on the FILE thread, it calls Stop, which Posts the tasks and returns. The
destructor chain then finishes and free is called on this. Later, this method
gets called with a this pointer that has been disposed. YIKES! :-)

http://codereview.chromium.org/3124025/diff/8001/9001#newcode67
chrome/browser/policy/config_dir_policy_provider.cc:67: DictionaryValue*
new_policy = Load();
potential leak if CheckModificationTime(now) returns false? --> scoped_ptr with
release() at policy_reset.

http://codereview.chromium.org/3124025/diff/8001/9001#newcode88
chrome/browser/policy/config_dir_policy_provider.cc:88:
UpdateRefreshTask(base::TimeDelta::FromMinutes(reload_interval_minutes_));
Why do you need this? Doesn't OnFilePathChanged notices the changes that should
obviate the need for this type of explicit polling? A comment would clear this
up.

http://codereview.chromium.org/3124025/diff/8001/9001#newcode96
chrome/browser/policy/config_dir_policy_provider.cc:96: void
PolicyDirLoader::OnFilePathChanged(const FilePath& path) {
This guy has an implicit requirement to be called on the FILE thread, but it's
not documented, and probably also not part of the FilePathWatcher API contract.
Perhaps he should call UpdateRefreshTask(0) instead?

http://codereview.chromium.org/3124025/diff/8001/9001#newcode158
chrome/browser/policy/config_dir_policy_provider.cc:158: void
PolicyDirLoader::UpdateRefreshTask(const base::TimeDelta& delay) {
You probably should use reload/refresh consistently. e.g. the refresh_task_
actually calls the Reload method, shouldn't it be the reload_task_?

http://codereview.chromium.org/3124025/diff/8001/9001#newcode170
chrome/browser/policy/config_dir_policy_provider.cc:170:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
Not sure if you really have to jump through hoops to make make this run on the
UI thread. The NotifyStoreOfPolicyChange method doesn't have a thread
prerequisite. Furthermore, if you have a pending notify on the UI thread, but
the PolicyDirLoader is destroyed on a thread other than the UI thread (a
definite possibility because it's held by a scopted_refptr), then the queued
PolicyDirLoader could reference a this pointer that has already been disposed.
So why not just call provier_->NotifyStoreOfPolicyChange() directly from Reload?

http://codereview.chromium.org/3124025/diff/8001/9001#newcode197
chrome/browser/policy/config_dir_policy_provider.cc:197: loader->Reload();
Perhaps I don't understand how the FilePatchWatcher works, but won't it tell you
when the dir changes? Why do you need the reload here? If this is just for the
first initialization, doesn't the the Loader's constructor guarantee this? If
there is some other reason, please comment.

http://codereview.chromium.org/3124025/diff/8001/9002
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/3124025/diff/8001/9002#newcode14
chrome/browser/policy/config_dir_policy_provider.h:14: #include
"base/scoped_ptr.h"
nit: order of includes should be alphabetic

http://codereview.chromium.org/3124025/diff/8001/9002#newcode58
chrome/browser/policy/config_dir_policy_provider.h:58: bool
CheckModificationTime(const base::Time& now);
Seems like "single responsibility" might make the code a little clearer here,
since I had to read this several times before I really understood what you were
trying to accomplish. How about calling this method "IsSafeToReloadPolicyNow"
and removing the side-effect of scheduling the reload? It would have to return a
delay_until_safe parameter.

http://codereview.chromium.org/3124025/diff/8001/9002#newcode61
chrome/browser/policy/config_dir_policy_provider.h:61: void
UpdateRefreshTask(const base::TimeDelta& delay);
How about calling this ScheduleDeferredReload?

http://codereview.chromium.org/3124025/diff/8001/9002#newcode81
chrome/browser/policy/config_dir_policy_provider.h:81: CancelableTask*
refresh_task_;
I can't find the code that deletes refresh_task. AFAIK, calling Cancel on a
RunnableMethod only releases the callee (i.e. the PolicyDirLoader), but doesn't
actually do anything with the RunnableMethod. That means that you need to manage
the lifecycle of refresh_task_ explicitly.

http://codereview.chromium.org/3124025/diff/8001/9003
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/3124025/diff/8001/9003#newcode82
chrome/browser/policy/config_dir_policy_provider_unittest.cc:82:
provider_.reset(NULL);
release?

http://codereview.chromium.org/3124025/diff/8001/9003#newcode219
chrome/browser/policy/config_dir_policy_provider_unittest.cc:219: }  //
namespace
Did you really intend to put all the tests in the anonymous namespace? Is this a
new idiom that I should be using? ;-)

[Mattias Nissler (ping if slow), 2010-08-19 13:20:44]

Thanks for your comments. I've answered/addressed your feedback and uploaded an
updated version.

http://codereview.chromium.org/3124025/diff/8001/9001
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/3124025/diff/8001/9001#newcode45
chrome/browser/policy/config_dir_policy_provider.cc:45: void
PolicyDirLoader::Stop() {
On 2010/08/19 12:40:32, danno wrote:
> Is this really thread safe? If the destructor of PolicyDirLoader is not called
> on the FILE thread, it calls Stop, which Posts the tasks and returns. The
> destructor chain then finishes and free is called on this. Later, this method
> gets called with a this pointer that has been disposed. YIKES! :-)

Only the ConfigDirPolicyProvider dtor calls stop, the PolicyDirLoader dtor
doesn't. Note that there's nothing wrong with not calling Stop() at all, but
that does mean that the loader only goes away after the reload task has been
run, which can be quite a bit...

http://codereview.chromium.org/3124025/diff/8001/9001#newcode67
chrome/browser/policy/config_dir_policy_provider.cc:67: DictionaryValue*
new_policy = Load();
On 2010/08/19 12:40:32, danno wrote:
> potential leak if CheckModificationTime(now) returns false? --> scoped_ptr
with
> release() at policy_reset.

Done.

http://codereview.chromium.org/3124025/diff/8001/9001#newcode88
chrome/browser/policy/config_dir_policy_provider.cc:88:
UpdateRefreshTask(base::TimeDelta::FromMinutes(reload_interval_minutes_));
On 2010/08/19 12:40:32, danno wrote:
> Why do you need this? Doesn't OnFilePathChanged notices the changes that
should
> obviate the need for this type of explicit polling? A comment would clear this
> up.

Note that the FilePathWatcher::Delegate has a OnError() callback that gets
invoked when the watcher stops working (for various reasons that Should Not
Happen (TM)). So the periodic reload is just a safeguard in case the file
watcher fails to detect a change or bails out entirely. I'll add a comment.

http://codereview.chromium.org/3124025/diff/8001/9001#newcode96
chrome/browser/policy/config_dir_policy_provider.cc:96: void
PolicyDirLoader::OnFilePathChanged(const FilePath& path) {
On 2010/08/19 12:40:32, danno wrote:
> This guy has an implicit requirement to be called on the FILE thread, but it's
> not documented, and probably also not part of the FilePathWatcher API
contract.
> Perhaps he should call UpdateRefreshTask(0) instead?

It actually is part of the FilePathWatcher contract :) I added a DCHECK.

http://codereview.chromium.org/3124025/diff/8001/9001#newcode158
chrome/browser/policy/config_dir_policy_provider.cc:158: void
PolicyDirLoader::UpdateRefreshTask(const base::TimeDelta& delay) {
On 2010/08/19 12:40:32, danno wrote:
> You probably should use reload/refresh consistently. e.g. the refresh_task_
> actually calls the Reload method, shouldn't it be the reload_task_?

Done.

http://codereview.chromium.org/3124025/diff/8001/9001#newcode170
chrome/browser/policy/config_dir_policy_provider.cc:170:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
On 2010/08/19 12:40:32, danno wrote:
> Not sure if you really have to jump through hoops to make make this run on the
> UI thread. The NotifyStoreOfPolicyChange method doesn't have a thread
> prerequisite. Furthermore, if you have a pending notify on the UI thread, but
> the PolicyDirLoader is destroyed on a thread other than the UI thread (a
> definite possibility because it's held by a scopted_refptr), then the queued
> PolicyDirLoader could reference a this pointer that has already been disposed.
> So why not just call provier_->NotifyStoreOfPolicyChange() directly from
Reload?

This definitely has to run on the UI thread. |provider_| is only safe to access
on the UI thread and NotifyStoreOfPolicyChange talks to the NotificationService,
which only works for the thread you're currently on.

If there is a pending notify on the UI thread, PolicyDirLoader won't get
destroyed (the task holds a reference).

http://codereview.chromium.org/3124025/diff/8001/9001#newcode197
chrome/browser/policy/config_dir_policy_provider.cc:197: loader->Reload();
On 2010/08/19 12:40:32, danno wrote:
> Perhaps I don't understand how the FilePatchWatcher works, but won't it tell
you
> when the dir changes? Why do you need the reload here? If this is just for the
> first initialization, doesn't the the Loader's constructor guarantee this? If
> there is some other reason, please comment.

The thing is that there might have been changes before we got around to
initializing the watcher that we missed, so we just recheck. I added a comment.

http://codereview.chromium.org/3124025/diff/8001/9002
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/3124025/diff/8001/9002#newcode14
chrome/browser/policy/config_dir_policy_provider.h:14: #include
"base/scoped_ptr.h"
On 2010/08/19 12:40:32, danno wrote:
> nit: order of includes should be alphabetic

Done.

http://codereview.chromium.org/3124025/diff/8001/9002#newcode58
chrome/browser/policy/config_dir_policy_provider.h:58: bool
CheckModificationTime(const base::Time& now);
On 2010/08/19 12:40:32, danno wrote:
> Seems like "single responsibility" might make the code a little clearer here,
> since I had to read this several times before I really understood what you
were
> trying to accomplish. How about calling this method "IsSafeToReloadPolicyNow"
> and removing the side-effect of scheduling the reload? It would have to return
a
> delay_until_safe parameter.

Done.

http://codereview.chromium.org/3124025/diff/8001/9002#newcode61
chrome/browser/policy/config_dir_policy_provider.h:61: void
UpdateRefreshTask(const base::TimeDelta& delay);
On 2010/08/19 12:40:32, danno wrote:
> How about calling this ScheduleDeferredReload?

Done.

http://codereview.chromium.org/3124025/diff/8001/9002#newcode81
chrome/browser/policy/config_dir_policy_provider.h:81: CancelableTask*
refresh_task_;
On 2010/08/19 12:40:32, danno wrote:
> I can't find the code that deletes refresh_task. AFAIK, calling Cancel on a
> RunnableMethod only releases the callee (i.e. the PolicyDirLoader), but
doesn't
> actually do anything with the RunnableMethod. That means that you need to
manage
> the lifecycle of refresh_task_ explicitly.

Note that the task remains scheduled. It'll be deleted when the message loop
gets around to running it, see MessageLoop::RunTask().

http://codereview.chromium.org/3124025/diff/8001/9003
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/3124025/diff/8001/9003#newcode82
chrome/browser/policy/config_dir_policy_provider_unittest.cc:82:
provider_.reset(NULL);
On 2010/08/19 12:40:32, danno wrote:
> release?

I don't understand? You want me to call release()? That's wrong, because it
doesn't delete :)

http://codereview.chromium.org/3124025/diff/8001/9003#newcode219
chrome/browser/policy/config_dir_policy_provider_unittest.cc:219: }  //
namespace
On 2010/08/19 12:40:32, danno wrote:
> Did you really intend to put all the tests in the anonymous namespace? Is this
a
> new idiom that I should be using? ;-)

I've seen both lately, I'll check the style guide to see what's preferred.

[danno, 2010-08-19 14:59:21]

LGTM pending bot greenness.