Allow transport candidates to arrive after the authentication accepted message.

Allow transport candidates to arrive before the authentication accepted message.

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

[rmsousa, 2013-07-12 22:14:56]

I tested this locally, and it seems to work. Looking through the code, it seems
safe to pass around NamedCandidate/cricket::Candidate objects this way. If we
get candidates that end up being never used, they'll be cleaned up once the
jingle_session is ended (jingle_session only lives for a single connection).

[Wez, 2013-07-12 22:43:24]

LGTM w/ nits!

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

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:240: // Check if any remote candidates were
already received for this channel.
nit: Blank line before this comment.

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:241:
std::list<JingleMessage::NamedCandidate>::iterator it =
nit: This code fragment is identical to the one in CreateDatagramChannel(), so
factor it out into a static helper function.

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:264: channel->Connect(callback);
nit: Blank line before this comment.

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:281: if (it != channels_.end() &&
!it->second->is_connected()) {
nit: Clear any candidates received for cancelled channels here?

[rmsousa, 2013-07-13 00:11:47]

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

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:240: // Check if any remote candidates were
already received for this channel.
On 2013/07/12 22:43:24, Wez wrote:
> nit: Blank line before this comment.

Moved to the header for the new function.

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:241:
std::list<JingleMessage::NamedCandidate>::iterator it =
On 2013/07/12 22:43:24, Wez wrote:
> nit: This code fragment is identical to the one in CreateDatagramChannel(), so
> factor it out into a static helper function.

Done.

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:264: channel->Connect(callback);
On 2013/07/12 22:43:24, Wez wrote:
> nit: Blank line before this comment.
Moved to the header for the new function.

https://codereview.chromium.org/18295009/diff/1/remoting/protocol/jingle_sess...
remoting/protocol/jingle_session.cc:281: if (it != channels_.end() &&
!it->second->is_connected()) {
On 2013/07/12 22:43:24, Wez wrote:
> nit: Clear any candidates received for cancelled channels here?

The candidates are removed from the list and added to the channel at the moment
we create it. At this point they're already gone from the pending list.

[commit-bot: I haz the power, 2013-07-13 03:59:40]

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

[Wez, 2013-07-13 03:59:55]

Looks great - CQing. Let's try to back-merge first thing next week.

[commit-bot: I haz the power, 2013-07-13 09:44:59]

Change committed as 211558

[Sergey Ulanov, 2013-07-15 17:25:52]

I'm not arguing that this is a right fix, but I think it should be framed as a
workaround for a bug in WCS. XMPP requires that messages are delivered in the
order in which they are sent ( http://xmpp.org/rfcs/rfc6120.html#rules-order ).
If that doesn't happen with WCS it's clearly a bug.

[Wez, 2013-07-15 18:17:51]

On 2013/07/15 17:25:52, Sergey Ulanov wrote:
> I'm not arguing that this is a right fix, but I think it should be framed as a
> workaround for a bug in WCS. XMPP requires that messages are delivered in the
> order in which they are sent ( http://xmpp.org/rfcs/rfc6120.html#rules-order
).
> If that doesn't happen with WCS it's clearly a bug.

Definitely worth raising w/ WCS folks, and perhaps adding a comment to this code
w/ a reference to the rules-order link.