Refactor of Authenticator to allow it to ProcessMessage asynchronously and then call a callback

Refactor of Authenticator to allow it to ProcessMessage asynchronously and then call a callback

This is so that ProcessMessage can potentially require user input (or use the result of a network operation) without blocking the network thread on JingleSession.

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

[rmsousa, 2013-02-28 04:42:47]

Hi Sergey,

This is the authenticator refactor that wez suggested in
https://codereview.chromium.org/12326090/

The existing authenticators are all still synchronous underneath, so this
shouldn't affect them.

This will be used to remove the WAITING_EXTERNAL/PerformExternalAction stuff
from the token authentication protocol.

[Sergey Ulanov, 2013-02-28 08:05:15]

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:156: void
JingleSession::ContinueAcceptIncomingConnection() {
This change introduces another state that wasn't possible before, and we keep
state_ set to INITIALIZING before this method is called. We need to make sure
that we handle the case when another message is received while in that state. I
suggest adding another state value in Session::State enum and then look through
this class to make sure it's handled properly everywhere. Specifically I see
that OnTerminate() needs to be updated, but there may be some other places.

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.h:92: void ContinueAcceptIncomingConnection();
The comment above doesn't apply to this method, so maybe add an empty line and a
comment above it.

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/negotiating...
File remoting/protocol/negotiating_authenticator.cc (right):

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/negotiating...
remoting/protocol/negotiating_authenticator.cc:162: // This object is guaranteed
to outlive the underlying authenticator.
nit: This is a good comment to have, but it's annoying to have it everywhere,
perhaps it's best to put use it only once per file. Also, we don't usually
separate parameter lists with long comments, so it may be better to move it
above ProcessMessage().

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/negotiating...
remoting/protocol/negotiating_authenticator.cc:164: resume_callback), message);
nit: I suggest formatting this code as follows:

current_authenticator_->ProcessMessage(
    base::Bind(&NegotiatingAuthenticator::UpdateState,
               base::Unretained(this), resume_callback),
    message);

It's more readable this way. Particularly the message parameter is not as easy
to miss.

[rmsousa, 2013-02-28 23:29:23]

Thanks for the review, ptal

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:156: void
JingleSession::ContinueAcceptIncomingConnection() {
On 2013/02/28 08:05:16, sergeyu wrote:
> This change introduces another state that wasn't possible before, and we keep
> state_ set to INITIALIZING before this method is called. We need to make sure
> that we handle the case when another message is received while in that state.
I
> suggest adding another state value in Session::State enum and then look
through
> this class to make sure it's handled properly everywhere. Specifically I see
> that OnTerminate() needs to be updated, but there may be some other places.

AcceptIncomingConnection is called after InitializeIncomingConnection, so it's
already in the CONNECTING stage.

Looking through the code, we seem to mostly already do the right thing in a
"CONNECTING" state: New SessionInfo messages for that session are rejected with
UNEXPECTED_REQUEST, CloseInternal sends a TERMINATE message, and OnTerminate
closes the session.

The only problem is that CONNECTING doesn't differentiate whether this is the
initiator or acceptor, so we could indeed get some interesting interactions
where we get confused: e.g. If we get an ACCEPT or TERMINATE message while we're
still processing the INITIATE, the acceptor suddenly thinks it was the one
trying to connect, and assumes the session was ACCEPTED/REJECTED.

I separated the states in CONNECTING and ACCEPTING, to avoid that kind of issue.
The Session doesn't need an explicit PROCESSING_MESSAGE state per se (receiving
a message while the authenticator is processing another is perfectly valid, as
long it's not another authentication message), that state must live in the
authenticator.

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.h:92: void ContinueAcceptIncomingConnection();
On 2013/02/28 08:05:16, sergeyu wrote:
> The comment above doesn't apply to this method, so maybe add an empty line and
a
> comment above it.

Yeah, this actually belongs together with ProcessAuthenticationStep.

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/negotiating...
File remoting/protocol/negotiating_authenticator.cc (right):

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/negotiating...
remoting/protocol/negotiating_authenticator.cc:162: // This object is guaranteed
to outlive the underlying authenticator.
On 2013/02/28 08:05:16, sergeyu wrote:
> nit: This is a good comment to have, but it's annoying to have it everywhere,
> perhaps it's best to put use it only once per file. Also, we don't usually
> separate parameter lists with long comments, so it may be better to move it
> above ProcessMessage().

Done.

https://codereview.chromium.org/12389010/diff/1/remoting/protocol/negotiating...
remoting/protocol/negotiating_authenticator.cc:164: resume_callback), message);
On 2013/02/28 08:05:16, sergeyu wrote:
> nit: I suggest formatting this code as follows:
> 
> current_authenticator_->ProcessMessage(
>     base::Bind(&NegotiatingAuthenticator::UpdateState,
>                base::Unretained(this), resume_callback),
>     message);
> 
> It's more readable this way. Particularly the message parameter is not as easy
> to miss.

Done.

https://codereview.chromium.org/12389010/diff/4001/remoting/protocol/authenti...
File remoting/protocol/authenticator.h (right):

https://codereview.chromium.org/12389010/diff/4001/remoting/protocol/authenti...
remoting/protocol/authenticator.h:60: PROCESSING_MESSAGE,
The current code always processes messages synchronously, so it never actually
uses this state.

https://codereview.chromium.org/12389010/diff/4001/remoting/protocol/session.h
File remoting/protocol/session.h (left):

https://codereview.chromium.org/12389010/diff/4001/remoting/protocol/session....
remoting/protocol/session.h:34: // TODO(sergeyu): Do we really need this state?
This state was indeed fairly redundant before this CL (it was always an
immediate, unconditional transition from INITIALIZING).
Now it is useful because we need to differentiate between CONNECTING and
ACCEPTING. INITIALIZING is still useless in practice (it always transitions
immediately to CONNECTING or ACCEPTING), but I guess it's a neutral placeholder
to use in the constructor.

[Sergey Ulanov, 2013-03-01 01:28:30]

LGTM provided my nits are addressed.

https://codereview.chromium.org/12389010/diff/4001/remoting/protocol/session.h
File remoting/protocol/session.h (left):

https://codereview.chromium.org/12389010/diff/4001/remoting/protocol/session....
remoting/protocol/session.h:34: // TODO(sergeyu): Do we really need this state?
On 2013/02/28 23:29:23, rmsousa wrote:
> This state was indeed fairly redundant before this CL (it was always an
> immediate, unconditional transition from INITIALIZING).
> Now it is useful because we need to differentiate between CONNECTING and
> ACCEPTING. INITIALIZING is still useless in practice (it always transitions
> immediately to CONNECTING or ACCEPTING), but I guess it's a neutral
placeholder
> to use in the constructor.

Agree. Thanks for removing TODO.

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
File remoting/protocol/authenticator.h (right):

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator.h:42: //    WAITING_MESSAGE ->
PROCESSING_MESSAGE -> MESSAGE_READY
Move the second transition to a separate line and add a comment that it happens
asynchronously later (not in ProcessMessage())

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator.h:93: virtual void ProcessMessage(const
base::Closure& resume_callback,
It would be more natural to have message argument first and the callback second
- usually completion callback goes last in list of arguments.

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator_test_base.cc:62:
RunAuthExchangeStep(client_.get(), host_.get());
This changes behavior of this function. Should it have Step suffix? And then
maybe rename the second function to RunAuthExchangeStepWith()?

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator_test_base.cc:65: void
AuthenticatorTestBase::RunAuthExchangeStep(Authenticator* sender,
Can this be static?

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/negotiat...
File remoting/protocol/negotiating_authenticator.cc (right):

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/negotiat...
remoting/protocol/negotiating_authenticator.cc:160: // |this| outlives
current_authenticator_, so using Unretained is safe here.
nit: I suggest rephrasing: |current_authenticator| is owned, so Unretained() is
safe here.

[rmsousa, 2013-03-01 02:22:11]

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
File remoting/protocol/authenticator.h (right):

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator.h:42: //    WAITING_MESSAGE ->
PROCESSING_MESSAGE -> MESSAGE_READY
On 2013/03/01 01:28:30, sergeyu wrote:
> Move the second transition to a separate line and add a comment that it
happens
> asynchronously later (not in ProcessMessage())

Done.

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator.h:93: virtual void ProcessMessage(const
base::Closure& resume_callback,
On 2013/03/01 01:28:30, sergeyu wrote:
> It would be more natural to have message argument first and the callback
second
> - usually completion callback goes last in list of arguments.

Done.

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator_test_base.cc:62:
RunAuthExchangeStep(client_.get(), host_.get());
On 2013/03/01 01:28:30, sergeyu wrote:
> This changes behavior of this function. Should it have Step suffix? And then
> maybe rename the second function to RunAuthExchangeStepWith()?

It's the other one that's named wrongly - running it will always run the
authentication until it finishes.
I renamed it ContinueAuthExchangeWith().

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/authenti...
remoting/protocol/authenticator_test_base.cc:65: void
AuthenticatorTestBase::RunAuthExchangeStep(Authenticator* sender,
On 2013/03/01 01:28:30, sergeyu wrote:
> Can this be static?

Yes.

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/negotiat...
File remoting/protocol/negotiating_authenticator.cc (right):

https://codereview.chromium.org/12389010/diff/7001/remoting/protocol/negotiat...
remoting/protocol/negotiating_authenticator.cc:160: // |this| outlives
current_authenticator_, so using Unretained is safe here.
On 2013/03/01 01:28:30, sergeyu wrote:
> nit: I suggest rephrasing: |current_authenticator| is owned, so Unretained()
is
> safe here. 

Done.

[commit-bot: I haz the power, 2013-03-01 02:24:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12389010/13001

[commit-bot: I haz the power, 2013-03-01 02:24:56]

Failed to apply patch for remoting/protocol/negotiating_authenticator.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file remoting/protocol/negotiating_authenticator.h
  Hunk #1 succeeded at 40 (offset -2 lines).
  Hunk #2 FAILED at 53.
  1 out of 2 hunks FAILED -- saving rejects to file
remoting/protocol/negotiating_authenticator.h.rej

Patch:       remoting/protocol/negotiating_authenticator.h
Index: remoting/protocol/negotiating_authenticator.h
diff --git a/remoting/protocol/negotiating_authenticator.h
b/remoting/protocol/negotiating_authenticator.h
index
eded6220fe47df051aec1bf95faebe7e67767eeb..7be0ff4d486a8ce3deca3d368d1ca211e8800dd3
100644
--- a/remoting/protocol/negotiating_authenticator.h
+++ b/remoting/protocol/negotiating_authenticator.h
@@ -42,7 +42,8 @@ class NegotiatingAuthenticator : public Authenticator {
   // Authenticator interface.
   virtual State state() const OVERRIDE;
   virtual RejectionReason rejection_reason() const OVERRIDE;
-  virtual void ProcessMessage(const buzz::XmlElement* message) OVERRIDE;
+  virtual void ProcessMessage(const buzz::XmlElement* message,
+                              const base::Closure& resume_callback) OVERRIDE;
   virtual scoped_ptr<buzz::XmlElement> GetNextMessage() OVERRIDE;
   virtual scoped_ptr<ChannelAuthenticator>
       CreateChannelAuthenticator() const OVERRIDE;
@@ -52,6 +53,7 @@ class NegotiatingAuthenticator : public Authenticator {
 
   void AddMethod(const AuthenticationMethod& method);
   void CreateAuthenticator(State initial_state);
+  void UpdateState(const base::Closure& resume_callback);
 
   bool is_host_side() const;

[commit-bot: I haz the power, 2013-03-01 02:39:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12389010/7002

[commit-bot: I haz the power, 2013-03-01 03:03:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12389010/6005

[commit-bot: I haz the power, 2013-03-01 07:59:42]

Change committed as 185492