Issue 356320: Pinless entry not working on Windows.

Issue 356320: Pinless entry not working on Windows.

The problem is that we initialize pairing_registry_delegate_ in both OnInitializePairingRegistry and CreateAuthenticatorFactory (from StartHost and OnPolicyUpdate). The former runs on the UI thread and the latter runs on the network thread. The feature works only if OnInitializePairingRegistry is invoked first.

I fixed the race condition on pairing_registry_delegate_ initialization in HostProcess by:
  1. store the PairingRegistry object instead of the delegate object, and
  2. recreate the authenticator factory object when the network process receives the reg keys for pinless auth from the daemon process, and
  3. always create the PairingRegistry object and the authenticator factory on the network thread.

Now everything runs on the network thread. If the network process receives the pinless auth reg key handles from the daemon process after StartHost has completed, we will simply recreate the authenticator factory.

BUG=356320
Committed: https://crrev.com/fec693f0b30ce3c21a0df420ee8e68cc190ad318
Cr-Commit-Position: refs/heads/master@{#310559}

[weitao, 2015-01-07 19:58:49]

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

[Wez, 2015-01-07 20:13:39]

nit: Please replace the CL title with something that describes the change you're
making, rather than just the name of the bug.

[Sergey Ulanov, 2015-01-07 20:26:43]

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

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:629: scoped_refptr<PairingRegistry>
pairing_registry = NULL;
don't need to set it to NULL explicitly. Also use nullptr instead of NULL
please.

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:635: if (unprivileged_key_) {
On windows there is no point creating PairingRegistry if we don't have
unprivileged_key_. So you shouldn't be using CreatePairingRegistryDelegate() on
Windows.

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:637:
reinterpret_cast<PairingRegistryDelegateWin*>(delegate.get());
Ouch, reinterpret_cast<>! You can avoid it, see my comment below.

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:642: pairing_registry = new
PairingRegistry(context_->file_task_runner(),
We don't really need a new instance of PairingRegistry every time Authenticator
is reinitialized. I suggest replacing |unprivileged_key_| and |privileged_key_|
with |pairing_registry_| and then lazily initializing it here for Linux and Mac
or in InitializePairingRegistry() for Windows.

[weitao, 2015-01-08 00:13:36]

PTAL

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

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:629: scoped_refptr<PairingRegistry>
pairing_registry = NULL;
On 2015/01/07 20:26:43, Sergey Ulanov wrote:
> don't need to set it to NULL explicitly. Also use nullptr instead of NULL
> please.

Done.

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:635: if (unprivileged_key_) {
On 2015/01/07 20:26:43, Sergey Ulanov wrote:
> On windows there is no point creating PairingRegistry if we don't have
> unprivileged_key_. So you shouldn't be using CreatePairingRegistryDelegate()
on
> Windows.

Done.

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:637:
reinterpret_cast<PairingRegistryDelegateWin*>(delegate.get());
On 2015/01/07 20:26:43, Sergey Ulanov wrote:
> Ouch, reinterpret_cast<>! You can avoid it, see my comment below.

Done.

https://codereview.chromium.org/834673007/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:642: pairing_registry = new
PairingRegistry(context_->file_task_runner(),
On 2015/01/07 20:26:43, Sergey Ulanov wrote:
> We don't really need a new instance of PairingRegistry every time
Authenticator
> is reinitialized. I suggest replacing |unprivileged_key_| and
|privileged_key_|
> with |pairing_registry_| and then lazily initializing it here for Linux and
Mac
> or in InitializePairingRegistry() for Windows.

Done.

[Sergey Ulanov, 2015-01-08 17:34:02]

LGTM

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

https://codereview.chromium.org/834673007/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:630: CreatePairingRegistryDelegate());
nit: this would be more readable with assignment:
  scoped_ptr<PairingRegistry::Delegate> delegate =
      CreatePairingRegistryDelegate();

Or just call inline when creating PairingRegistry below.

https://codereview.chromium.org/834673007/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:633: delegate.Pass());
nit: indentation.

[weitao, 2015-01-08 17:56:29]

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

https://codereview.chromium.org/834673007/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:630: CreatePairingRegistryDelegate());
On 2015/01/08 17:34:02, Sergey Ulanov wrote:
> nit: this would be more readable with assignment:
>   scoped_ptr<PairingRegistry::Delegate> delegate =
>       CreatePairingRegistryDelegate();
> 
> Or just call inline when creating PairingRegistry below.

Done.

https://codereview.chromium.org/834673007/diff/60001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:633: delegate.Pass());
On 2015/01/08 17:34:02, Sergey Ulanov wrote:
> nit: indentation.

Done.

[weitao, 2015-01-08 18:39:35]

The CQ bit was checked by weitaosu@chromium.org

[commit-bot: I haz the power, 2015-01-08 18:40:37]

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

[commit-bot: I haz the power, 2015-01-08 19:26:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-08 19:28:10]

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