Add blocking IO restriction on the network thread.

Add blocking IO restriction on the network thread.

Now ChromotingHostContext disallows blocking operations on then
network thread. This will help to avoid issues such as the one
that was fixed in https://codereview.chromium.org/886913002/
(Network thread was used to load policies from disk).

Also made the following changes to avoid blocking operation on
the network thread:
 - Moved PolicyWatcher to the main thread in the Me2Me host.
   PolicyLoader uses blocking IO for initialization. Also this is
   more consistent with the It2Me host where PolicyWatcher also
   runs on the UI thread.
 - Moved NetworkChangeNotifier initialization out of
   HostSignalingManager. NetworkChangeNotifier creates a thread
   internally and therefore should be destroyed on the main thread.
 - Added a reference to URLRequestContextGetter in HostProcessMain
   to make sure it's destroyed on the main thread. This is necessary
   because BasicURLRequestContext owns threads and needs to be
   deleted on the main thread.

Committed: https://crrev.com/c099813e194006d7ba33b71d7f62cc782a36d6a6
Cr-Commit-Position: refs/heads/master@{#314165}

[Sergey Ulanov, 2015-01-30 20:59:22]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-01-30 23:01:29]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2015-01-30 23:04:03]

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

[Łukasz Anforowicz, 2015-01-31 00:02:34]

LGTM, except I don't understand what the scoped_ptr<net::NetworkChangeNotifier>
does.

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

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:553: if (!policies_loaded_)
The early return is not needed - this can be made more similar to
OnPolicyUpdated which does a similar "if" before calling StartHost:

if (state_ == HOST_INITIALIZING) {
  if (policies_loaded_) {
    StartHost();
  }
} else if (state == HOST_STARTED) {
  DCHECK(policies_loaded_);
  ...
}

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:557: StartHost();
It seems that StartHost can only proceed if
1) policies_loaded_
and
2) !serialized_config_.empty()
Would it make sense to check these 2 conditions *inside* StartHost method,
rather than in OnConfigUpdated and OnPolicyUpdated methods?

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:560: ApplyHostDomainPolicy();
Would it make sense to add DCHECK(policy_loaded_) inside ApplyHostDomainPolicy
method (and other ApplyXxx methods)?

If not, then maybe a DCHECK here would help understand the code (because when
state_ == HOST_STARTED, then we have to have policies_loaded_, right?).

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:983: policies_loaded_ = true;
If adding DCHECK (as I suggested elsewhere) to ApplyXxx methods, then
policies_loaded_ = true would have to be set slightly earlier, before calling
any of OnXxx from this method.

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:1509:
scoped_ptr<net::NetworkChangeNotifier> network_change_notifier;
I don't understand what this does - this will always be wrapping a nullptr /
this local variable is never assigned to.  Did you want to drop scoped_ptr and
just have a net::NetworkChangeNotifier value here?

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:1520: // dereferences on UI threads. Store
the reference to the URLRequestGetter to
I don't understand :-(  I would understand if you said that threads managed by
BasicURLRequestContext need to be destroyed as late as possible / from UI
thread.  I don't understand why holding references to threads would mean having
to be dereferenced from UI threads.

[Sergey Ulanov, 2015-01-31 00:34:26]

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

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:553: if (!policies_loaded_)
On 2015/01/31 00:02:34, lukasza wrote:
> The early return is not needed - this can be made more similar to
> OnPolicyUpdated which does a similar "if" before calling StartHost:
> 
> if (state_ == HOST_INITIALIZING) {
>   if (policies_loaded_) {
>     StartHost();
>   }
> } else if (state == HOST_STARTED) {
>   DCHECK(policies_loaded_);
>   ...
> }

Done.

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:557: StartHost();
On 2015/01/31 00:02:34, lukasza wrote:
> It seems that StartHost can only proceed if
> 1) policies_loaded_
> and
> 2) !serialized_config_.empty()
> Would it make sense to check these 2 conditions *inside* StartHost method,
> rather than in OnConfigUpdated and OnPolicyUpdated methods?

Added StartHostIfReady() that checks these two conditions and call StartHost();

StartHost() is called in other places, particularly when restarting.

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:560: ApplyHostDomainPolicy();
On 2015/01/31 00:02:34, lukasza wrote:
> Would it make sense to add DCHECK(policy_loaded_) inside ApplyHostDomainPolicy
> method (and other ApplyXxx methods)?
> 
> If not, then maybe a DCHECK here would help understand the code (because when
> state_ == HOST_STARTED, then we have to have policies_loaded_, right?).

Added DCHECK here. I don't think we need a DCHECK in Apply*().

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:983: policies_loaded_ = true;
On 2015/01/31 00:02:34, lukasza wrote:
> If adding DCHECK (as I suggested elsewhere) to ApplyXxx methods, then
> policies_loaded_ = true would have to be set slightly earlier, before calling
> any of OnXxx from this method.

I'd like to avoid too many changes in how policies are handled in this CL.

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:1509:
scoped_ptr<net::NetworkChangeNotifier> network_change_notifier;
On 2015/01/31 00:02:34, lukasza wrote:
> I don't understand what this does - this will always be wrapping a nullptr /
> this local variable is never assigned to.  Did you want to drop scoped_ptr and
> just have a net::NetworkChangeNotifier value here?

Yes. Thank you.

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:1520: // dereferences on UI threads. Store
the reference to the URLRequestGetter to
On 2015/01/31 00:02:34, lukasza wrote:
> I don't understand :-(  I would understand if you said that threads managed by
> BasicURLRequestContext need to be destroyed as late as possible / from UI
> thread.  I don't understand why holding references to threads would mean
having
> to be dereferenced from UI threads.

Thread::Stop() is a blocking operation (because it joins the thread). So it now
DCHECKs if it runs on the network thread.

[Łukasz Anforowicz, 2015-01-31 00:41:35]

lgtm

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

https://codereview.chromium.org/891663005/diff/40001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:1520: // dereferences on UI threads. Store
the reference to the URLRequestGetter to
On 2015/01/31 00:34:26, Sergey Ulanov wrote:
> On 2015/01/31 00:02:34, lukasza wrote:
> > I don't understand :-(  I would understand if you said that threads managed
by
> > BasicURLRequestContext need to be destroyed as late as possible / from UI
> > thread.  I don't understand why holding references to threads would mean
> having
> > to be dereferenced from UI threads.
> 
> Thread::Stop() is a blocking operation (because it joins the thread). So it
now
> DCHECKs if it runs on the network thread.

Acknowledged.

[Sergey Ulanov, 2015-01-31 01:08:32]

Renato, PTAL.

[rmsousa, 2015-01-31 01:34:34]

lgtm

(see comment about networkchangenotifier - it doesn't seem to be doing anything,
but, either way, we're not _changing_ any behavior there)

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

https://codereview.chromium.org/891663005/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:1517:
scoped_ptr<net::NetworkChangeNotifier> network_change_notifier(
I don't see any object registering observers for this, and this variable isn't
passed anywhere... is this doing anything?

(the old code also doesn't seem to do anything with this object)

[Sergey Ulanov, 2015-01-31 01:38:39]

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

https://codereview.chromium.org/891663005/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:1517:
scoped_ptr<net::NetworkChangeNotifier> network_change_notifier(
On 2015/01/31 01:34:34, rmsousa wrote:
> I don't see any object registering observers for this, and this variable isn't
> passed anywhere... is this doing anything?
> 
> (the old code also doesn't seem to do anything with this object)

This is a singleton that is used by code that needs to be notified about network
changes. See
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...

[Sergey Ulanov, 2015-01-31 01:38:43]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-01-31 01:39:50]

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

[commit-bot: I haz the power, 2015-01-31 07:41:25]

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

[commit-bot: I haz the power, 2015-01-31 07:41:26]

Try jobs failed on following builders:
  win8_chromium_gn_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)

[Sergey Ulanov, 2015-02-02 17:59:58]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-02-02 18:00:55]

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

[commit-bot: I haz the power, 2015-02-02 18:02:00]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-02-02 18:02:49]

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