Add TransportContext class.

remoting/protocol/transport_context.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/protocol/transport_context.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "remoting/protocol/port_allocator_factory.h"
+#include "third_party/webrtc/p2p/client/httpportallocator.h"
+
+namespace remoting {
+namespace protocol {
+
+// Get fresh STUN/Relay configuration every hour.
+static const int kJingleInfoUpdatePeriodSeconds = 3600;
+
+TransportContext::TransportContext(
+    SignalStrategy* signal_strategy,
+    scoped_ptr<PortAllocatorFactory> port_allocator_factory,
+    const NetworkSettings& network_settings,
+    TransportRole role)
+    : signal_strategy_(signal_strategy),
+      port_allocator_factory_(port_allocator_factory.Pass()),
+      network_settings_(network_settings),
+      role_(role) {}
+
+TransportContext::~TransportContext() {}
+
+void TransportContext::Prepare() {
+  EnsureFreshJingleInfo();
+}
+
+void TransportContext::CreatePortAllocator(
+    const CreatePortAllocatorCallback& callback) {
+  EnsureFreshJingleInfo();
+
+  // If there is a pending |jingle_info_request_| delay starting the new
+  // transport until the request is finished.
+  if (jingle_info_request_) {
+    pending_port_allocator_requests_.push_back(callback);
+  } else {
+    callback.Run(CreatePortAllocatorInternal());

[Jamie, 2015/12/14 23:29:05]

In our JS code, we try to make sure that, even in the synchronous case, the
callback is not invoked until after the method responsible for calling it has
returned. Having the order non-deterministic can lead to hard-to-reproduce bugs
if one of the scenarios is rather rare and the caller doesn't handle it
properly. Could you explicitly post the task to the current run loop, or are
there already so many instances of this pattern that it's not worth fixing?

[Sergey Ulanov, 2015/12/15 18:52:15]

We use the same pattern for many other asynchronous operations and I don't think
it causes any problems (e.g. see StreamChannelFactory::CreateChannel()).

+  }
+}
+
+void TransportContext::EnsureFreshJingleInfo() {
+  uint32 stun_or_relay_flags = NetworkSettings::NAT_TRAVERSAL_STUN |
+      NetworkSettings::NAT_TRAVERSAL_RELAY;
+  if (!(network_settings_.flags & stun_or_relay_flags) ||

[Jamie, 2015/12/14 23:29:05]

I think an explicit ==0 would be clearer than a ! here, especially since the
conditional has an ||.

[Sergey Ulanov, 2015/12/15 18:52:15]

Done.

+      jingle_info_request_) {
+    return;
+  }
+
+  if (last_jingle_info_update_time_.is_null() ||
+      base::TimeTicks::Now() - last_jingle_info_update_time_ >
+          base::TimeDelta::FromSeconds(kJingleInfoUpdatePeriodSeconds)) {
+    jingle_info_request_.reset(new JingleInfoRequest(signal_strategy_));
+    jingle_info_request_->Send(base::Bind(
+        &TransportContext::OnJingleInfo, base::Unretained(this)));
+  }
+}
+
+void TransportContext::OnJingleInfo(
+    const std::string& relay_token,
+    const std::vector<std::string>& relay_hosts,
+    const std::vector<rtc::SocketAddress>& stun_hosts) {
+  relay_token_ = relay_token;
+  relay_hosts_ = relay_hosts;
+  stun_hosts_ = stun_hosts;
+
+  jingle_info_request_.reset();
+  if ((!relay_token.empty() && !relay_hosts.empty()) || !stun_hosts.empty())

[Jamie, 2015/12/14 23:29:05]

If neither relay nor stun hosts were received, should we log an error? 
Similarly, does should we log an error if we receive relay hosts but no token,
or vice versa? Does it make sense to call the port allocator callbacks in that
case?

[Sergey Ulanov, 2015/12/15 18:52:15]

This check is here to handle the case when the request has failed. In that case
JingleInfoRequest logs a warning and calls this callback with empty parameters.
PortAllocator is still created because the session may still connect
successfully without STUN and Relay.

If the server returned a response that had STUN servers, but no relay, or vise
versa then the response is still considered valid. I don't think it makes sense
to retry the request in that case. Also client already log results of jingleinfo
request so it's not worth logging a warning in that case.

There is also the case when we get a valid but empty response - it's handled the
same way as the error case here. This is not ideal, but I don't think it's worth
fixing now.

BTW we will need switch to standard TURN soon and will probably remove
JingleInfoRequest or replace it with something else.
Also note that this is not new logic - I'm just moving this code from
IceTransportFactory.

+    last_jingle_info_update_time_ = base::TimeTicks::Now();

[Jamie, 2015/12/14 23:29:05]

Doesn't this belong in EnsureFreshJingleInfo (or maybe in both places)?
Otherwise, multiple rapid calls to the function will result in multiple requests
being sent.

[Sergey Ulanov, 2015/12/15 18:52:15]

EnsureFreshJingleInfo() makes a request only if it's not already pending, so I
don't think this is a problem. I've updated EnsureFreshJingleInfo() to make it
clearer.

+
+  while (!pending_port_allocator_requests_.empty()) {
+    pending_port_allocator_requests_.begin()->Run(
+        CreatePortAllocatorInternal());
+    pending_port_allocator_requests_.pop_front();
+  }
+}
+
+scoped_ptr<cricket::PortAllocator>
+TransportContext::CreatePortAllocatorInternal() {
+  scoped_ptr<cricket::HttpPortAllocatorBase> result =
+      port_allocator_factory_->CreatePortAllocator();
+
+  LOG(ERROR) << relay_hosts_.size();

[Jamie, 2015/12/14 23:29:05]

Remove (or lower verbosity level).

[Sergey Ulanov, 2015/12/15 18:52:15]

Sorry, I thought I removed these.

+  if (!relay_token_.empty() && !relay_hosts_.empty()) {
+    result->SetRelayHosts(relay_hosts_);
+    result->SetRelayToken(relay_token_);
+  }
+  LOG(ERROR) << stun_hosts_.size();

[Jamie, 2015/12/14 23:29:05]

Ditto

[Sergey Ulanov, 2015/12/15 18:52:15]

Done.

+  if (!stun_hosts_.empty()) {
+    result->SetStunHosts(stun_hosts_);
+  }
+
+  // We always use PseudoTcp to provide a reliable channel. It provides poor
+  // performance when combined with TCP-based transport, so we have to disable
+  // TCP ports. ENABLE_SHARED_UFRAG flag is specified so that the same username
+  // fragment is shared between all candidates.
+  int flags = cricket::PORTALLOCATOR_DISABLE_TCP |
+              cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+              cricket::PORTALLOCATOR_ENABLE_IPV6;
+
+ if (!(network_settings_.flags & NetworkSettings::NAT_TRAVERSAL_STUN))
+    flags |= cricket::PORTALLOCATOR_DISABLE_STUN;
+
+  if (!(network_settings_.flags & NetworkSettings::NAT_TRAVERSAL_RELAY))
+    flags |= cricket::PORTALLOCATOR_DISABLE_RELAY;
+
+  result->set_flags(flags);
+  result->SetPortRange(network_settings_.port_range.min_port,
+                       network_settings_.port_range.max_port);
+
+  return result.Pass();
+}
+
+}  // namespace protocol
+}  // namespace remoting