Issue 1085703003: Use standard ICE in Chromoting.

Issue 1085703003: Use standard ICE in Chromoting. (Closed)

Created:
5 years, 8 months ago by Sergey Ulanov

Modified:
5 years, 8 months ago

Reviewers:
rmsousa, Wez, dcaiafa

CC:
chromium-reviews, chromoting-reviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Use standard ICE in Chromoting. Previously we were using legacy, non-standard version of ICE. This change adds ICE version negotiation and enabled standard ICE by default, when both peers support it. BUG=473758 Committed: https://crrev.com/5a5854ee3e1c5760b422f26d31909bfb5dca631f Cr-Commit-Position: refs/heads/master@{#326560} Committed: https://crrev.com/b4b0927a2e02d6357f019aeb78576e7a8d84da47 Cr-Commit-Position: refs/heads/master@{#326600}

Patch Set 1 : #

Total comments: 1

Patch Set 2 : #

Patch Set 3 : #

Total comments: 44

Patch Set 4 : #

Total comments: 4

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : fixes for gcc #

Patch Set 8 : fix crash on memory bots #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+744 lines, -328 lines)			Patch
M	remoting/client/jni/chromoting_jni_instance.cc	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -1 line	0 comments	Download
M	remoting/client/plugin/chromoting_instance.cc	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	remoting/host/chromoting_host.cc	View	1 2 3	1 chunk	+5 lines, -3 lines	0 comments	Download
M	remoting/host/chromoting_host_unittest.cc	View	1 2 3	7 chunks	+8 lines, -12 lines	0 comments	Download
M	remoting/host/client_session_unittest.cc	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	remoting/host/session_manager_factory.cc	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	remoting/protocol/content_description.h	View		2 chunks	+2 lines, -5 lines	0 comments	Download
M	remoting/protocol/content_description.cc	View	1 2 3 4 5 6	5 chunks	+19 lines, -23 lines	0 comments	Download
M	remoting/protocol/fake_connection_to_host.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/protocol/fake_connection_to_host.cc	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/protocol/fake_session.h	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	remoting/protocol/fake_session.cc	View	1 2 3	1 chunk	+3 lines, -3 lines	0 comments	Download
M	remoting/protocol/jingle_messages.h	View	1 2 3	5 chunks	+17 lines, -8 lines	0 comments	Download
M	remoting/protocol/jingle_messages.cc	View	1 2 3	8 chunks	+206 lines, -42 lines	0 comments	Download
M	remoting/protocol/jingle_messages_unittest.cc	View	1 2 3 4	3 chunks	+105 lines, -14 lines	0 comments	Download
M	remoting/protocol/jingle_session.h	View	1 2 3	6 chunks	+17 lines, -8 lines	0 comments	Download
M	remoting/protocol/jingle_session.cc	View	1 2 3 4	12 chunks	+88 lines, -42 lines	0 comments	Download
M	remoting/protocol/jingle_session_unittest.cc	View	1 2 3	6 chunks	+24 lines, -4 lines	0 comments	Download
M	remoting/protocol/libjingle_transport_factory.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	remoting/protocol/libjingle_transport_factory.cc	View	1 2 3 4 5 6 7	13 chunks	+66 lines, -17 lines	0 comments	Download
M	remoting/protocol/protocol_mock_objects.h	View	1 2 3	1 chunk	+4 lines, -1 line	0 comments	Download
M	remoting/protocol/session.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/protocol/session_config.h	View	1 2 3	5 chunks	+35 lines, -40 lines	0 comments	Download
M	remoting/protocol/session_config.cc	View	1 2 3	7 chunks	+101 lines, -86 lines	0 comments	Download
M	remoting/protocol/transport.h	View	1 2 3	4 chunks	+21 lines, -0 lines	0 comments	Download
M	remoting/test/protocol_perftest.cc	View	1 2 3 4 5	2 chunks	+4 lines, -6 lines	0 comments	Download
M	remoting/test/test_chromoting_client.cc	View		1 chunk	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 41 (16 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Sergey Ulanov

https://codereview.chromium.org/1085703003/diff/20001/remoting/protocol/libjingle_transport_factory.cc File remoting/protocol/libjingle_transport_factory.cc (right): https://codereview.chromium.org/1085703003/diff/20001/remoting/protocol/libjingle_transport_factory.cc#newcode220 remoting/protocol/libjingle_transport_factory.cc:220: base::ThreadTaskRunnerHandle::Get()->PostTask( Previously callback_ was called only after the channel ...

5 years, 8 months ago (2015-04-13 23:43:57 UTC) #3

Wez

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/content_description.cc File remoting/protocol/content_description.cc (right): https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/content_description.cc#newcode234 remoting/protocol/content_description.cc:234: } How long will we have two ICE impls ...

5 years, 8 months ago (2015-04-17 20:33:11 UTC) #8

rmsousa

Confirming: Has this been tested with old client vs. new host and new client vs. ...

5 years, 8 months ago (2015-04-17 23:36:42 UTC) #9

Sergey Ulanov

> Has this been tested with old client vs. new host and new client vs. ...

5 years, 8 months ago (2015-04-22 00:08:40 UTC) #10

> Has this been tested with old client vs. new host and new client vs. old host?

Yes.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/conte...
File remoting/protocol/content_description.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/conte...
remoting/protocol/content_description.cc:226: LOG(ERROR) << "Unknown
ice-version: " << child->BodyText();
On 2015/04/17 23:36:41, rmsousa wrote:
> Should we fail if _one_ of the ice versions is not recognized? That makes this
> very non-future-proof. Wouldn't it be better to just ignore (with warning?)
> unknown ice versions, and only fail if the XML (1) did provide Ice Versions
> (i.e. not old chromoting), and (2) all Ice Versions are unsupported.?

Good catch. But I removed this code now. It's replaced with a single tag that
indicates ICE support.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/conte...
remoting/protocol/content_description.cc:234: }
On 2015/04/17 20:33:10, Wez wrote:
> How long will we have two ICE impls for?

Not longer than a release or two.

> Could we make the presence of the XML
> attribute a boolean "on switch" for standard ICE, and default to Google-ICE
> otherwise?

Yes, good idea. I don't think we will need to support more than one ICE version
for very long. Done.


> i.e. once we switch to standard ICE, will we later remove this
> "version" tag again?

I think we will want to keep it for a while, even after GICE support is removed,
just so that we can detect older clients and hosts, to make sure the client will
show appropriate error message.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/ice_v...
File remoting/protocol/ice_version.h (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/ice_v...
remoting/protocol/ice_version.h:14: enum class IceVersion {
On 2015/04/17 20:33:10, Wez wrote:
> nit: Consider calling this IceProto and then the enums STANDARD & GOOGLE.

Removed this now - replaced with a simple bool in SessionConfig.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
File remoting/protocol/jingle_messages.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:54: JingleMessage::IceCredentials*
credentials) {
On 2015/04/17 20:33:10, Wez wrote:
> nit: Would it be cleaner to have ParseIceCredentials return a
> scoped_ptr<IceCredentials>?

The caller stores result in a list<> so the value would need to be copied out of
scoped_ptr<> later if it's returned by pointer here. Also passing pointer to the
output is more consistent with ParseIceTransportInfo() that I added now.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:75: JingleMessage::NamedCandidate*
candidate) {
On 2015/04/17 20:33:10, Wez wrote:
> nit: See above re return value.

same here

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:223: }
On 2015/04/17 20:33:10, Wez wrote:
> nit: Can't you define this via = default in the header?

Done.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:245: reason(UNKNOWN_REASON) {
On 2015/04/17 20:33:10, Wez wrote:
> nit: Redefine this as a delegated ctor?

Done.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:251: const std::string& sid_value)
On 2015/04/17 20:33:10, Wez wrote:
> Neither this nor the default ctor initialize ice_version

Done.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:251: const std::string& sid_value)
On 2015/04/17 23:36:42, rmsousa wrote:
> On 2015/04/17 20:33:10, Wez wrote:
> > Neither this nor the default ctor initialize ice_version
> 
> Furthermore, I'd recommend setting the 0-value of the enum explicitly to
> UNKNOWN, and initializing with that, to protect against unintended "default"
> behavior.

Removed IceVersion enum now.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:356: *error = "ICE and GICE transport
information is found in the same message";
On 2015/04/17 23:36:42, rmsousa wrote:
> shouldn't a new client be sending both sets of ICE transports (since it
doesn't
> know beforehand if it'll be talking to a new or old peer?)

No. Transport info is sent only after the connection is accepted by the host.
Once session-accepted message is received both host and client know which
version they are going to use.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:358: } else if (ice_transport_tag) {
On 2015/04/17 20:33:10, Wez wrote:
> Suggest breaking these out into ParseIceTransportInfo() and
> ParseGiceTransportInfo() helper methods, for readability.

Done.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:456: if (ice_version ==
IceVersion::STANDARD) {
On 2015/04/17 20:33:10, Wez wrote:
> Consider using switch here, to guard against new IceVersion values being added
> later.

Removed IceVersion enum now.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
File remoting/protocol/jingle_messages_unittest.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_messages_unittest.cc:108:
"<ice-version>rfc5245</ice-version>"
On 2015/04/17 23:36:42, rmsousa wrote:
> Please add tests that messages from old peers (no ice-version) are parsed
> correctly.

Done.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
File remoting/protocol/jingle_session.cc (left):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_session.cc:466: // In case there is transport
information in the accept message.
On 2015/04/17 23:36:42, rmsousa wrote:
> Do we know that even old peers will never send transport info in the accept
> message?

Some very old versions may include transport info in session-accept, but  not
after we switched to the current auth protocol. Support for the old auth
protocol was dropped long time ago.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_session.cc:40: const int kTransportInfoSendDelayMs =
20;
On 2015/04/17 23:36:42, rmsousa wrote:
> what happens if the delay is not enough? do we just send separate
transport-info
> messages as they become ready?

Yes.

> does our code handle those gracefully?

Yes.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_session.cc:247:
std::list<JingleMessage::IceCredentials>::iterator credentials =
On 2015/04/17 23:36:42, rmsousa wrote:
> Should this and jingleMessage::ParseXML be consistent regarding  what happens
if
> both types of credentials being present?

Not sure what you mean.

 - When using standard ICE the credentials are received separately in
JingleMessage::ice_credentials. So we need to pass these credentials to
transport by calling SetRemoteCandidate(). In this case JingleMessage guarantees
that JingleMessage::candidates doesn't include credentials.
 - When using legacy GICE we will be receiving TransportInfo messages with list
of candidates that include ICE credentials. In that case
JingleMessage::ice_credentials is empty. JingleMessage guarantees that.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_session.cc:530: LOG(ERROR) << "Received transport-info
message with invalid ICE version.";
On 2015/04/17 20:33:10, Wez wrote:
> invalid -> mismatched?

Changed to "Received transport-info message in format different from
negotiated."

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/libji...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/libji...
remoting/protocol/libjingle_transport_factory.cc:243: // FIXME: what if channel
is empty;
On 2015/04/17 23:36:42, rmsousa wrote:
> //FIXME

Done.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/sessi...
File remoting/protocol/session_config.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/sessi...
remoting/protocol/session_config.cc:23: // Usually each of these vectors will
contain just several elements,
On 2015/04/17 20:33:10, Wez wrote:
> nit: vectors -> lists
> nit: "just several" -> "only a few"

Done.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/sessi...
remoting/protocol/session_config.cc:63: SessionConfig::SessionConfig() :
ice_version_(IceVersion::STANDARD) {
On 2015/04/17 23:36:42, rmsousa wrote:
> Do we want STANDARD to be the default? If this is intended to always be
> overwritten later, it should be initialized to an explicit unknown value.

I've refactored SessionConfig to guarantee that all instances of SessionConfig
are negotiated, i.e. there is no way to create SessionConfig with the default
value. Does this address your concern?

rmsousa

lgtm https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingle_session.cc File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingle_session.cc#newcode40 remoting/protocol/jingle_session.cc:40: const int kTransportInfoSendDelayMs = 20; On 2015/04/22 00:08:40, ...

5 years, 8 months ago (2015-04-22 00:32:51 UTC) #11

lgtm

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_session.cc:40: const int kTransportInfoSendDelayMs =
20;
On 2015/04/22 00:08:40, Sergey Ulanov wrote:
> On 2015/04/17 23:36:42, rmsousa wrote:
> > what happens if the delay is not enough? do we just send separate
> transport-info
> > messages as they become ready?
> 
> Yes.

Nit: Could you document that more explicitly in the comment?

> 
> > does our code handle those gracefully?
> 
> Yes.
>

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingl...
remoting/protocol/jingle_session.cc:247:
std::list<JingleMessage::IceCredentials>::iterator credentials =
On 2015/04/22 00:08:40, Sergey Ulanov wrote:
> On 2015/04/17 23:36:42, rmsousa wrote:
> > Should this and jingleMessage::ParseXML be consistent regarding  what
happens
> if
> > both types of credentials being present?
> 
> Not sure what you mean.
> 
>  - When using standard ICE the credentials are received separately in
> JingleMessage::ice_credentials. So we need to pass these credentials to
> transport by calling SetRemoteCandidate(). In this case JingleMessage
guarantees
> that JingleMessage::candidates doesn't include credentials.
>  - When using legacy GICE we will be receiving TransportInfo messages with
list
> of candidates that include ICE credentials. In that case
> JingleMessage::ice_credentials is empty. JingleMessage guarantees that.

Nevermind, I think I misunderstood how the credentials/candidates lists worked.

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/sessi...
File remoting/protocol/session_config.cc (right):

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/sessi...
remoting/protocol/session_config.cc:63: SessionConfig::SessionConfig() :
ice_version_(IceVersion::STANDARD) {
On 2015/04/22 00:08:40, Sergey Ulanov wrote:
> On 2015/04/17 23:36:42, rmsousa wrote:
> > Do we want STANDARD to be the default? If this is intended to always be
> > overwritten later, it should be initialized to an explicit unknown value.
> 
> I've refactored SessionConfig to guarantee that all instances of SessionConfig
> are negotiated, i.e. there is no way to create SessionConfig with the default
> value. Does this address your concern?

Yup. thanks

dcaiafa

Driving by :) https://codereview.chromium.org/1085703003/diff/100001/remoting/protocol/content_description.cc File remoting/protocol/content_description.cc (right): https://codereview.chromium.org/1085703003/diff/100001/remoting/protocol/content_description.cc#newcode148 remoting/protocol/content_description.cc:148: for (ChannelConfig config : config()->control_configs()) { ...

5 years, 8 months ago (2015-04-22 17:42:45 UTC) #13

Sergey Ulanov

https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingle_session.cc File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/1085703003/diff/60001/remoting/protocol/jingle_session.cc#newcode40 remoting/protocol/jingle_session.cc:40: const int kTransportInfoSendDelayMs = 20; On 2015/04/22 00:32:50, rmsousa ...

5 years, 8 months ago (2015-04-22 19:14:13 UTC) #14

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-22 19:22:04 UTC) #17

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_dbg/builds/64357)

5 years, 8 months ago (2015-04-22 19:44:12 UTC) #19

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-22 20:33:03 UTC) #22

commit-bot: I haz the power

Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile_dbg/builds/8931)

5 years, 8 months ago (2015-04-22 22:19:32 UTC) #24

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-23 00:55:57 UTC) #27

commit-bot: I haz the power

Patchset 7 (id:??) landed as https://crrev.com/4b35571a3085aa960dcf5533fb2277d5dcaaf11f Cr-Commit-Position: refs/heads/master@{#326435}

5 years, 8 months ago (2015-04-23 02:54:59 UTC) #29

horo

A revert of this CL (patchset #7 id:160001) has been created in https://codereview.chromium.org/1099203005/ by horo@chromium.org. ...

5 years, 8 months ago (2015-04-23 05:03:10 UTC) #30

Sergey Ulanov

On 2015/04/23 05:03:10, horo wrote: > A revert of this CL (patchset #7 id:160001) has ...

5 years, 8 months ago (2015-04-23 17:17:43 UTC) #31

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-23 17:18:48 UTC) #33

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-23 17:22:34 UTC) #35

Sergey Ulanov

A revert of this CL (patchset #7 id:160001) has been created in https://codereview.chromium.org/1089253005/ by sergeyu@chromium.org. ...

5 years, 8 months ago (2015-04-23 17:41:22 UTC) #36

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-23 17:48:47 UTC) #39

commit-bot: I haz the power

5 years, 8 months ago (2015-04-23 19:00:52 UTC) #41

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages