Make the mapping from client id -> secret asynchronous.

Make the mapping from client id -> secret asynchronous.

In the original design, I intended the pairing registry delegate to maintain
an in-memory cache of client id -> secret, and for this to be invalidated by
a watcher when the on-disk version changes (for example, if the web-app is
used to revoke a pairing). I think an asynchronous model makes more sense,
and eliminates the need for the watcher.

In addition, this CL fixes a bug in the negotiating authenticator when dealing
with authenticators that process messages asychronously.

BUG=156182
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206347

[Jamie, 2013-06-12 23:27:34]

ptal

[rmsousa, 2013-06-13 04:26:18]

lgtm

I think AddPairing would be better asynchronous, but since it won't affect user
experience now, I'm fine with checking this in as it is and TODO'ing it.

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.cc:26: PairingRegistry::Pairing
PairingRegistry::CreatePairing(
Not for current CL:

This is now a very thin layer around the Delegate. The only piece of code in it
is actually a constructor for Pairing.

As it is now, it looks like this and Delegate could be combined in a single
interface. It also looks like implementations might be completely stateless,
eliminating the need for this to be a shared pointer, and letting each
authenticator/clientsession just have their own instance. But I think it's
better to wait until all the OS-specific implementations are done before
refactoring this, as it's possible we might find out we need an
intermediate/shared class after all.

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:48: // Add pairing information to
persistent storage. Must not block.
This interface isn't safe anymore, now that there isn't a local cache. Assuming
"must not block" means that it will post to an IO thread to do the actual write
to disk, it is possible that CreatePairing would succeed, and then an attempt to
connect with those credentials immediately afterwards would fail (either because
the disk write failed, or, depending on the IO threading setup, because
GetPairing returned before the write).

It seems that this should also be asynchronous. It would finish writing, and
*then* call a callback passed by clientSession that sends the new pairing. You'd
need to add a weakptrfactory to clientsession, though.

Of course, because right now the webapp doesn't really notify the user of the
pairing response, or show the user the saved pairings, the failure cases would
look exactly the same to the user either way - the user would've requested
pairing, and it wouldn't work in the next connection. So this is only relevant
once there is some client UI for showing pairings.

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:52: // invoke the callback with a callback
in which the client_id is empty.
nit: it says client_id is empty in the description, but the test in the
authenticator code is for paired_secret.

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/protocol_mo...
File remoting/protocol/protocol_mock_objects.cc (right):

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.cc:7: #include "base/message_loop.h"
Nit: is this needed?

[Jamie, 2013-06-13 17:22:15]

fyi

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.cc:26: PairingRegistry::Pairing
PairingRegistry::CreatePairing(
On 2013/06/13 04:26:19, rmsousa wrote:
> Not for current CL:
> 
> This is now a very thin layer around the Delegate. The only piece of code in
it
> is actually a constructor for Pairing.
> 
> As it is now, it looks like this and Delegate could be combined in a single
> interface. It also looks like implementations might be completely stateless,
> eliminating the need for this to be a shared pointer, and letting each
> authenticator/clientsession just have their own instance. But I think it's
> better to wait until all the OS-specific implementations are done before
> refactoring this, as it's possible we might find out we need an
> intermediate/shared class after all.

That's true. At one point there was some code here to check explicitly that
duplicate client ids are never generated, but since the probability of that was
so low, I removed it. I think you're right that the class isn't really needed
any more.

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:48: // Add pairing information to
persistent storage. Must not block.
On 2013/06/13 04:26:19, rmsousa wrote:
> This interface isn't safe anymore, now that there isn't a local cache.
Assuming
> "must not block" means that it will post to an IO thread to do the actual
write
> to disk, it is possible that CreatePairing would succeed, and then an attempt
to
> connect with those credentials immediately afterwards would fail (either
because
> the disk write failed, or, depending on the IO threading setup, because
> GetPairing returned before the write).
> 
> It seems that this should also be asynchronous. It would finish writing, and
> *then* call a callback passed by clientSession that sends the new pairing.
You'd
> need to add a weakptrfactory to clientsession, though.
> 
> Of course, because right now the webapp doesn't really notify the user of the
> pairing response, or show the user the saved pairings, the failure cases would
> look exactly the same to the user either way - the user would've requested
> pairing, and it wouldn't work in the next connection. So this is only relevant
> once there is some client UI for showing pairings.

The client will never show pairing information, only the host. So, the failure
mode you describe is always benign. However, I don't think adding an
asynchronous callback is particularly difficult, so it's probably worth doing it
as a follow-up, perhaps once all the platform specific delegates are
implemented.

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:52: // invoke the callback with a callback
in which the client_id is empty.
On 2013/06/13 04:26:19, rmsousa wrote:
> nit: it says client_id is empty in the description, but the test in the
> authenticator code is for paired_secret.

Done.

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/protocol_mo...
File remoting/protocol/protocol_mock_objects.cc (right):

https://codereview.chromium.org/16893002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.cc:7: #include "base/message_loop.h"
On 2013/06/13 04:26:19, rmsousa wrote:
> Nit: is this needed?

Not any more, and neither are the other includes, so I've removed them all.

[commit-bot: I haz the power, 2013-06-13 17:22:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/16893002/7001

[commit-bot: I haz the power, 2013-06-13 17:41:26]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-13 18:23:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/16893002/18002

[commit-bot: I haz the power, 2013-06-14 07:08:16]

Change committed as 206347