Always run PolicyWatcher on UI thread in It2Me host.

Cleanup threading in PolicyWatcher.

Previously PolicyWatcher allowed initialization on a thread
different from the one it runs on. It's no longer necessary.
Also for It2Me it was initialized on UI thread, but was given
task runner for the network thread, which causes the DCHECK in the
linked bug.

Now PolicyWatcher is marked as NonThreadSafe. It's initialized on
UI thread for It2Me host and on NetworkThread for the me2me host.
Also the file thread is used for blocking operation when reading
policies.

BUG=453615
Committed: https://crrev.com/b2ae7e31aeeaa8f6dcffc4d2c859beea16ae97c2
Cr-Commit-Position: refs/heads/master@{#314026}

[Sergey Ulanov, 2015-01-30 00:20:55]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-01-30 00:22:54]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2015-01-30 00:23:52]

sergeyu@chromium.org changed reviewers:
+ kelvinp@chromium.org, lukasza@chromium.org

[Łukasz Anforowicz, 2015-01-30 05:35:55]

Thanks for catching and addressing the threading problem I must have introduced
in the 2nd changelist for the policy_hack refactoring.  At least the problem was
caught by a DCHECK I've added in the very same changelist...  

I had a few code review comments.  They are probably more substantial/important
than "nits", but I am not sure if they should block a commit.  LGTM.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_host.cc (left):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.cc:362:
host_context_->network_task_runner()->PostTask(
After your changes, OnPolicyUpdate will always be called on the ui_task_runner,
and therefore we can remove this if/PostTask statement, right?
(i.e. this if statement can be replaced with a DCHECK, right?)

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.cc:539: context->network_task_runner());
I wonder if we should be using file_task_runner here (instead of
network_task_runner) - this thread will end up being used for reading policy
contents from json files / windows registry / mac preferences.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.cc (left):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.cc:286: content::BrowserThread::UI),
This call to GetMessageLoopProxyForThread should probably be replaced with
current_task_runner?  Or maybe with a DCHECK that these two values are equal?

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.cc:283:
DCHECK(current_task_runner->BelongsToCurrentThread());
It might be possible to avoid explicitly passing current_task_runner, by getting
it from MessageLoopProxy::current().  I am not sure if this is desirable
though...

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (left):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:84: // When |policy_service| is
specified then |task_runner| argument is ignored
Need to update the name of the parameter in the comment below:
When |policy_service| is specified then |*io*_task_runner| argument is ignored.

Also - we probably want to document in the comments that when specifying
|policy_service|, it has to be safe to use |policy_service| from
|current_task_runner|.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:80: // PolicyWatcher must only be
used on the same thread it's created and it call
Non-native speaker here, but I think it should be "... it's created *on* and it
*will* call ... ".

https://codereview.chromium.org/886913002/diff/60001/remoting/host/remoting_m...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:559: context_->network_task_runner());
I wonder if we should be using file_task_runner as the last argument
(io_task_runner) here - this thread will end up being used for reading policy
contents from json files / windows registry / mac preferences.

[Łukasz Anforowicz, 2015-01-30 17:16:00]

Still LGTM, but I've added a comment about opportunity to simplify StartWatching
and StopWatching methods after your changes.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:80: // PolicyWatcher must only be
used on the same thread it's created and it call
RE: PolicyWatcher must only be used on the same thread

This is definitely true for how it2me and me2me call StartWatching and
StopWatching and I think it means that these 2 methods can be simplified:

*) PolicyWatcher::StartWatching does not have to check if
(!OnPolicyServiceThread()) { ... policy_service_task_runner->PostTask ... } but
can simply do DCHECK(OnPolicyServiceThread()).  

*) StopWatching doesn't need to PostTask and doesn't need |stopped_callback| to
communicate asynchronous completion.

[Sergey Ulanov, 2015-01-30 19:33:46]

PTAL. Simplified PolicyWatcher.
Kelvin, I sill need your review (Lucazs is not a committer yet).

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_host.cc (left):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.cc:362:
host_context_->network_task_runner()->PostTask(
On 2015/01/30 05:35:55, lukasza wrote:
> After your changes, OnPolicyUpdate will always be called on the
ui_task_runner,
> and therefore we can remove this if/PostTask statement, right?
> (i.e. this if statement can be replaced with a DCHECK, right?)

This callback is called on UI thread, but we still want to process it on the
network thread. This PostTask() jumps from UI to Network thread.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.cc:539: context->network_task_runner());
On 2015/01/30 05:35:55, lukasza wrote:
> I wonder if we should be using file_task_runner here (instead of
> network_task_runner) - this thread will end up being used for reading policy
> contents from json files / windows registry / mac preferences.

Yes, you are right. From the AsyncPolicyLoader name I assumed that it doesn't
block the "background thread", but it does. Fixed now.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.cc (left):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.cc:286: content::BrowserThread::UI),
On 2015/01/30 05:35:55, lukasza wrote:
> This call to GetMessageLoopProxyForThread should probably be replaced with
> current_task_runner?  Or maybe with a DCHECK that these two values are equal?

removed current_task_runner

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.cc:283:
DCHECK(current_task_runner->BelongsToCurrentThread());
On 2015/01/30 05:35:55, lukasza wrote:
> It might be possible to avoid explicitly passing current_task_runner, by
getting
> it from MessageLoopProxy::current().  I am not sure if this is desirable
> though...

base::ThreadTaskRunnerHandle::Get() is the preferred way to get task runner for
current thread. But in remoting host we use AutoThreadTaskRunner and so it needs
to be passed explicitly. Anyway, this parameter is not needed anymore. Removed
it.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (left):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:84: // When |policy_service| is
specified then |task_runner| argument is ignored
On 2015/01/30 05:35:55, lukasza wrote:
> Need to update the name of the parameter in the comment below:
> When |policy_service| is specified then |*io*_task_runner| argument is
ignored.
> 
> Also - we probably want to document in the comments that when specifying
> |policy_service|, it has to be safe to use |policy_service| from
> |current_task_runner|.
> 

Done.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:80: // PolicyWatcher must only be
used on the same thread it's created and it call
On 2015/01/30 05:35:55, lukasza wrote:
> Non-native speaker here, but I think it should be "... it's created *on* and
it
> *will* call ... ".

Done.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:80: // PolicyWatcher must only be
used on the same thread it's created and it call
On 2015/01/30 17:16:00, lukasza wrote:
> RE: PolicyWatcher must only be used on the same thread
> 
> This is definitely true for how it2me and me2me call StartWatching and
> StopWatching and I think it means that these 2 methods can be simplified:
> 
> *) PolicyWatcher::StartWatching does not have to check if
> (!OnPolicyServiceThread()) { ... policy_service_task_runner->PostTask ... }
but
> can simply do DCHECK(OnPolicyServiceThread()).  
> 
> *) StopWatching doesn't need to PostTask and doesn't need |stopped_callback|
to
> communicate asynchronous completion.

Yes, Thank you. Actually marked PolicyWatcher as NonThreadSafe and removed
policy_service_task_runner.
Also removed StopWatchign() - we don't need it anymore because shutdown can be
synchronous now, yay!

https://codereview.chromium.org/886913002/diff/60001/remoting/host/remoting_m...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:559: context_->network_task_runner());
On 2015/01/30 05:35:55, lukasza wrote:
> I wonder if we should be using file_task_runner as the last argument
> (io_task_runner) here - this thread will end up being used for reading policy
> contents from json files / windows registry / mac preferences.

Yes. Fixed.

[Sergey Ulanov, 2015-01-30 19:37:39]

Patchset #4 (id:100001) has been deleted

[Łukasz Anforowicz, 2015-01-30 19:50:59]

Still LTGM, but I added some comments you might want to take a look at.

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_host.cc (left):

https://codereview.chromium.org/886913002/diff/60001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.cc:362:
host_context_->network_task_runner()->PostTask(
On 2015/01/30 19:33:45, Sergey Ulanov wrote:
> On 2015/01/30 05:35:55, lukasza wrote:
> > After your changes, OnPolicyUpdate will always be called on the
> ui_task_runner,
> > and therefore we can remove this if/PostTask statement, right?
> > (i.e. this if statement can be replaced with a DCHECK, right?)
> 
> This callback is called on UI thread, but we still want to process it on the
> network thread. This PostTask() jumps from UI to Network thread.

Acknowledged.

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.cc:100:
DCHECK(!policy_error_callback.is_null());
nit: I wonder if we should/could attempt to catch and prevent calling
StartWatching twice, by doing DCHECK(policy_updated_callback_.is_null()) here.

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (left):

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:98: bool OnPolicyServiceThread()
const;
OnPolicyServiceThread method can probably go away, because now we can just use
base::NonThreadSafe for DCHECKs.

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:69: // PolicyService (on other
OS-es). PolicyWatcher must be used on th thread on
nit: typo ("th thread")

[Sergey Ulanov, 2015-01-30 20:06:07]

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.cc:100:
DCHECK(!policy_error_callback.is_null());
On 2015/01/30 19:50:59, lukasza wrote:
> nit: I wonder if we should/could attempt to catch and prevent calling
> StartWatching twice, by doing DCHECK(policy_updated_callback_.is_null()) here.

Done.

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (left):

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:98: bool OnPolicyServiceThread()
const;
On 2015/01/30 19:50:59, lukasza wrote:
> OnPolicyServiceThread method can probably go away, because now we can just use
> base::NonThreadSafe for DCHECKs.

Done.

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/886913002/diff/80001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:69: // PolicyService (on other
OS-es). PolicyWatcher must be used on th thread on
On 2015/01/30 19:50:59, lukasza wrote:
> nit: typo ("th thread")

Done.

[kelvinp, 2015-01-30 20:45:40]

lgtm

[Sergey Ulanov, 2015-01-30 20:58:27]

Patchset #5 (id:140001) has been deleted

[Sergey Ulanov, 2015-01-30 20:58:47]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-01-30 20:59:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886913002/160001

[commit-bot: I haz the power, 2015-01-30 21:11:12]

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

[commit-bot: I haz the power, 2015-01-30 21:11:13]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2015-01-30 22:54:13]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-01-30 22:54:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886913002/180001

[commit-bot: I haz the power, 2015-01-30 23:33:56]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2015-01-30 23:34:56]

Patchset 6 (id:??) landed as
https://crrev.com/b2ae7e31aeeaa8f6dcffc4d2c859beea16ae97c2
Cr-Commit-Position: refs/heads/master@{#314026}