Create an experiment to study whether too many bindings cause NAT failure

content/renderer/media/webrtc/stun_field_trial.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 "content/renderer/media/webrtc/stun_field_trial.h"
 
 #include <math.h>
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "third_party/webrtc/base/asyncpacketsocket.h"
 #include "third_party/webrtc/base/asyncresolverinterface.h"
 #include "third_party/webrtc/base/ipaddress.h"
 #include "third_party/webrtc/base/network.h"
 #include "third_party/webrtc/base/socketaddress.h"
 #include "third_party/webrtc/base/thread.h"
 #include "third_party/webrtc/p2p/base/packetsocketfactory.h"
-#include "third_party/webrtc/p2p/stunprober/stunprober.h"
 
 using stunprober::StunProber;
 
 namespace content {
 
 namespace {
 
 // This needs to be the same as NatTypeCounters in histograms.xml.
 enum NatType {
   NAT_TYPE_NONE,
@@ skipping 23 matching lines @@
 }
 
 // Below 50ms, we are doing experiments at each 5ms interval. Beyond 50ms, only
 // one experiment of 100ms.
 int ClampProbingInterval(int interval_ms) {
   return interval_ms > 50 ? 100 : interval_ms;
 }
 
 std::string HistogramName(const std::string& prefix,
                           NatType nat_type,
-                          int interval_ms) {
-  return base::StringPrintf("WebRTC.Stun.%s.%s.%dms", prefix.c_str(),
-                            NatTypeNames[nat_type], interval_ms);
-}
-
-void SaveHistogramData(StunProber* prober) {
-  StunProber::Stats stats;
-  if (!prober->GetStats(&stats))
-    return;
-
-  NatType nat_type = GetNatType(stats.nat_type);
-
-  // Use the real probe interval for reporting, converting from nanosecond to
-  // millisecond at 5ms boundary.
-  int interval_ms =
-      round(static_cast<float>(stats.actual_request_interval_ns) / 5000) * 5;
-
-  interval_ms = ClampProbingInterval(interval_ms);
-
-  UMA_HISTOGRAM_ENUMERATION("WebRTC.NAT.Metrics", nat_type, NAT_TYPE_MAX);
-
-  std::string histogram_name =
-      HistogramName("SuccessPercent", nat_type, interval_ms);
-
-  // Mimic the same behavior as UMA_HISTOGRAM_PERCENTAGE. We can't use that
-  // macro as the histogram name is determined dynamically.
-  base::HistogramBase* histogram = base::Histogram::FactoryGet(
-      histogram_name, 1, 101, 102, base::Histogram::kUmaTargetedHistogramFlag);
-  histogram->Add(stats.success_percent);
-
-  DVLOG(1) << "Histogram '" << histogram_name.c_str()
-           << "' = " << stats.success_percent;
-
-  histogram_name = HistogramName("ResponseLatency", nat_type, interval_ms);
-
-  histogram = base::Histogram::FactoryTimeGet(
-      histogram_name, base::TimeDelta::FromMilliseconds(1),
-      base::TimeDelta::FromSeconds(10), 50,
-      base::Histogram::kUmaTargetedHistogramFlag);
-  histogram->AddTime(base::TimeDelta::FromMilliseconds(stats.average_rtt_ms));
-
-  DVLOG(1) << "Histogram '" << histogram_name.c_str()
-           << "' = " << stats.average_rtt_ms << " ms";
-
-  DVLOG(1) << "Shared Socket Mode: " << stats.shared_socket_mode;
-  DVLOG(1) << "Requests sent: " << stats.num_request_sent;
-  DVLOG(1) << "Responses received: " << stats.num_response_received;
-  DVLOG(1) << "Target interval (ns): " << stats.target_request_interval_ns;
-  DVLOG(1) << "Actual interval (ns): " << stats.actual_request_interval_ns;
-  DVLOG(1) << "NAT Type: " << NatTypeNames[nat_type];
-  DVLOG(1) << "Host IP: " << stats.host_ip;
-  DVLOG(1) << "Server-reflexive ips: ";
-  for (const auto& ip : stats.srflx_addrs)
-    DVLOG(1) << "\t" << ip;
-}
-
-void OnStunProbeTrialFinished(StunProber* prober, int result) {
-  if (result == StunProber::SUCCESS)
-    SaveHistogramData(prober);
+                          int interval_ms,
+                          int batch_index) {
+  return base::StringPrintf("WebRTC.Stun.%s.%s.%dms.%d.%d", prefix.c_str(),
+                            NatTypeNames[nat_type], interval_ms, batch_index);
 }
 
 }  // namespace
 
-bool ParseStunProbeParameters(const std::string& params,
-                              int* requests_per_ip,
-                              int* interval_ms,
-                              int* shared_socket_mode,
-                              std::vector<rtc::SocketAddress>* servers) {
+StunProberWithWeakPtr::StunProberWithWeakPtr(StunProber* prober)
+    : prober_(prober), weak_factory_(this) {}
+
+void StunProberWithWeakPtr::set_next_prober(
+    StunProberWithWeakPtr* next_prober) {
+  DCHECK(!next_prober_);
+  next_prober_ = next_prober;
+}
+
+void StunProberWithWeakPtr::Start(StunProber::Observer* observer) {
+  if (next_prober_) {
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE, base::Bind(&StunProberWithWeakPtr::Start,
+                              next_prober_->GetWeakPtr(), observer),

[Sergey Ulanov, 2015/10/30 18:27:49]

So if I understand correctly the reason you need weak pointers is so that you
can schedule sequence of the probers to run in sequence and then cancel the
process by destroying the chain. I don't think you really need weak pointers in
this case. E.g. you could use base::OneShotTimer here to schedule execution of
the next task. Timers don't call tasks after the timer object is destroyed.

I suggest getting rid of StunProberWithWeakPtr entirely and storing list of
pending probers in a list and then run them one by one from a timer task
scheduled in StunProberTrial. I think that would make this code simpler and
easier to understand.

[guoweis_left_chromium, 2015/10/30 21:47:13]

Done.

+        base::TimeDelta::FromMilliseconds(prober_->estimated_execution_time()));
+  }
+  prober_->Start(observer);
+}
+
+StunProberWithWeakPtr::~StunProberWithWeakPtr() {
+  if (next_prober_) {

[Alexei Svitkine (slow), 2015/10/30 17:32:18]

Nit: No {}'s

+    base::MessageLoop::current()->DeleteSoon(FROM_HERE, next_prober_);

[Alexei Svitkine (slow), 2015/10/30 17:32:18]

Can you add a comment why DeleteSoon is being used? vs. just e.g. deleting it?

[Sergey Ulanov, 2015/10/30 18:27:49]

So if you have a chain of 5 probers it would take 5 tasks to delete them all. Is
it necessary?

[Sergey Ulanov, 2015/10/30 18:27:49]

next_proper_ is effectively owned by StunProberWithWeakPtr, so in that case it's
better to use scoped_ptr<> for next_prober_ to make ownership clear in the class
definition. And then here you will need to call release() and pass the pointer
to DeleteSoon

+  }
+}
+
+StunProberTrial::StunProberTrial(rtc::NetworkManager* network_manager,
+                                 const std::string& params,
+                                 rtc::PacketSocketFactory* factory)
+    : network_manager_(network_manager), params_(params), factory_(factory) {
+  // We have to connect to the signal to avoid a race condition if network
+  // manager hasn't received the network update when we start, the StunProber
+  // will just fail.
+  network_manager_->SignalNetworksChanged.connect(
+      this, &StunProberTrial::OnNetworksChanged);
+  network_manager_->StartUpdating();
+}
+
+StunProberTrial::~StunProberTrial() {
+  DCHECK(thread_checker_.CalledOnValidThread());

[Sergey Ulanov, 2015/10/30 18:27:49]

Don't need this. thread_checker_ verifies that it's deleted on the right thread.

[guoweis_left_chromium, 2015/10/30 21:47:13]

Done.

+}
+
+void StunProberTrial::SaveHistogramData() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  NatType nat_type = NAT_TYPE_MAX;
+  int interval_ms = 0;
+  int count = 0;
+  int total_sent = 0;
+  int total_recv = 0;
+  for (StunProberWithWeakPtr* prober = prober_head_.get(); prober != nullptr;
+       prober = prober->GetNextProber()) {
+    ++count;
+
+    // Get the stats.
+    StunProber::Stats stats;
+    if (!prober->GetStats(&stats))
+      return;
+
+    // Check if the NAT type is consistent.
+    if (nat_type == NAT_TYPE_MAX) {
+      nat_type = GetNatType(stats.nat_type);
+      // If we can't figure out the nattype at the beginning, just return.
+      if (nat_type == NAT_TYPE_UNKNOWN)
+        return;
+    }
+    // For subsequent probers, we might get unknown as nattype if all the
+    // bindings fail, but it's ok.
+    else if (nat_type != GetNatType(stats.nat_type) &&

[Alexei Svitkine (slow), 2015/10/30 17:32:18]

Nit: Put this on the same line as }. Maybe move the comment inside the else if?

+             nat_type != NAT_TYPE_UNKNOWN)

[Alexei Svitkine (slow), 2015/10/30 17:32:19]

Nit: {}'s

+      return;
+
+    // Check that the interval is consistent.
+    // Use the real probe interval for reporting, converting from nanosecond to
+    // millisecond at 5ms boundary.
+    int new_interval_ms = ClampProbingInterval(
+        round(static_cast<float>(stats.actual_request_interval_ns) / 5000) * 5);
+    if (interval_ms == 0) {
+      interval_ms = new_interval_ms;
+    } else if (interval_ms != new_interval_ms)

[Alexei Svitkine (slow), 2015/10/30 17:32:19]

Nit: {}'s to be consistent with if

+      return;
+
+    // Sum up the total sent and recv packets.
+    total_sent += stats.num_request_sent;
+    total_recv += stats.num_response_received;
+
+    if (count % batch_size_ > 0)
+      continue;
+
+    if (total_sent == 0) {
+      total_recv = 0;
+      continue;
+    }
+
+    int success_rate = total_recv * 100 / total_sent;
+    std::string histogram_name = HistogramName(
+        "SuccessPercent", nat_type, interval_ms, count / batch_size_);
+
+    // Mimic the same behavior as UMA_HISTOGRAM_PERCENTAGE. We can't use
+    // that macro as the histogram name is determined dynamically.
+    base::HistogramBase* histogram =
+        base::Histogram::FactoryGet(histogram_name, 1, 101, 102,
+                                    base::Histogram::kUmaTargetedHistogramFlag);
+    histogram->Add(success_rate);
+
+    DVLOG(1) << "Histogram '" << histogram_name.c_str()
+             << "' = " << stats.success_percent;
+
+    DVLOG(1) << "Shared Socket Mode: " << stats.shared_socket_mode;
+    DVLOG(1) << "Requests sent: " << total_sent;
+    DVLOG(1) << "Responses received: " << total_recv;
+    DVLOG(1) << "Target interval (ns): " << stats.target_request_interval_ns;
+    DVLOG(1) << "Actual interval (ns): " << stats.actual_request_interval_ns;
+    DVLOG(1) << "NAT Type: " << NatTypeNames[nat_type];
+    DVLOG(1) << "Host IP: " << stats.host_ip;
+
+    total_sent = 0;
+    total_recv = 0;
+  }
+}
+
+bool StunProberTrial::ParseParameters(
+    const std::string& params,
+    int* requests_per_ip,
+    int* interval_ms,
+    int* shared_socket_mode,
+    int* batch_size,
+    int* total_batches,
+    std::vector<rtc::SocketAddress>* servers) {

[Alexei Svitkine (slow), 2015/10/30 17:32:18]

Nit: Suggest making a struct to hold the different param fields. Will make the
code more concise and harder to make mistakes (i.e. pass the wrong int as the
wrong param number).

   std::vector<std::string> stun_params = base::SplitString(
       params, "/", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
 
-  if (stun_params.size() < 4) {
+  if (stun_params.size() < 5) {
     DLOG(ERROR) << "Not enough parameters specified in StartStunProbeTrial";
     return false;
   }
   auto param = stun_params.begin();
 
   if (param->empty()) {
     *requests_per_ip = 10;
   } else if (!base::StringToInt(*param, requests_per_ip)) {
     DLOG(ERROR) << "Failed to parse request_per_ip in StartStunProbeTrial";
     return false;
@@ skipping 11 matching lines @@
   param++;
 
   if ((*param).empty()) {
     *shared_socket_mode = base::RandInt(0, 1);
   } else if (!base::StringToInt(*param, shared_socket_mode)) {
     DLOG(ERROR) << "Failed to parse shared_socket_mode in StartStunProbeTrial";
     return false;
   }
   param++;
 
+  if (param->empty()) {
+    *batch_size = 5;
+  } else if (!base::StringToInt(*param, batch_size)) {
+    DLOG(ERROR) << "Failed to parse batch_size in StartStunProbeTrial";
+    return false;
+  }
+  param++;
+
+  if (param->empty()) {
+    *total_batches = 5;
+  } else if (!base::StringToInt(*param, total_batches)) {
+    DLOG(ERROR) << "Failed to parse total_batches in StartStunProbeTrial";
+    return false;
+  }
+  param++;
+
   while (param != stun_params.end() && !param->empty()) {
     rtc::SocketAddress server;
     if (!server.FromString(*param)) {
       DLOG(ERROR) << "Failed to parse address in StartStunProbeTrial";
       return false;
     }
     servers->push_back(server);
     param++;
   }
 
   return !servers->empty();
 }
 
-scoped_ptr<stunprober::StunProber> StartStunProbeTrial(
-    const rtc::NetworkManager::NetworkList& networks,
-    const std::string& params,
-    rtc::PacketSocketFactory* factory) {
-  DVLOG(1) << "Starting stun trial with params: " << params;
+void StunProberTrial::OnNetworksChanged() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(1) << "Starting stun trial with params: " << params_;
+  rtc::NetworkManager::NetworkList networks;
+  network_manager_->GetNetworks(&networks);
 
   // If we don't have local addresses, we won't be able to determine whether
   // we're behind NAT or not.
   if (networks.empty()) {
     DLOG(ERROR) << "No networks specified in StartStunProbeTrial";
-    return nullptr;
+    return;
   }
 
+  network_manager_->StopUpdating();
+  network_manager_->SignalNetworksChanged.disconnect(this);
+
   int requests_per_ip;
+  int shared_socket_mode;
   int interval_ms;
-  int shared_socket_mode;
+  int total_batches;
   std::vector<rtc::SocketAddress> servers;
 
-  if (!ParseStunProbeParameters(params, &requests_per_ip, &interval_ms,
-                                &shared_socket_mode, &servers)) {
-    return nullptr;
+  if (!ParseParameters(params_, &requests_per_ip, &interval_ms,
+                       &shared_socket_mode, &batch_size_, &total_batches,
+                       &servers)) {
+    return;
   }
 
-  scoped_ptr<StunProber> prober(
-      new StunProber(factory, rtc::Thread::Current(), networks));
+  total_probers_ = total_batches * batch_size_;
 
-  if (!prober->Start(
-          servers, (shared_socket_mode != 0), interval_ms, requests_per_ip,
-          1000,
-          rtc::Callback2<void, StunProber*, int>(&OnStunProbeTrialFinished))) {
-    DLOG(ERROR) << "Failed to Start in StartStunProbeTrial";
-    OnStunProbeTrialFinished(prober.get(), StunProber::GENERIC_FAILURE);
-    return nullptr;
+  StunProberWithWeakPtr* prev_prober = nullptr;
+
+  for (int i = 0; i < total_probers_; i++) {
+    stunprober::StunProber* prober =
+        new StunProber(factory_, rtc::Thread::Current(), networks);
+    scoped_ptr<StunProberWithWeakPtr> prober_wp(
+        new StunProberWithWeakPtr(prober));
+    if (!prober->Prepare(servers, (shared_socket_mode != 0), interval_ms,
+                         requests_per_ip, 1000, this)) {
+      DLOG(ERROR) << "Failed to Prepare in StartStunProbeTrial";
+      return;
+    }
+
+    if (prev_prober)
+      prev_prober->set_next_prober(prober_wp.get());
+    else
+      prober_head_.reset(prober_wp.get());
+
+    prev_prober = prober_wp.release();
   }
-
-  return prober;
 }
 
+void StunProberTrial::OnFinished(StunProber* prober,
+                                 StunProber::Status result) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (result == StunProber::SUCCESS)
+    ++finished_probers_;
+
+  if (finished_probers_ == total_probers_)
+    SaveHistogramData();
+}
+
+void StunProberTrial::OnPrepared(StunProber* prober,
+                                 StunProber::Status result) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (result == StunProber::SUCCESS)
+    ++ready_probers_;
+
+  if (ready_probers_ == total_probers_) {
+    DCHECK(prober_head_);

[Alexei Svitkine (slow), 2015/10/30 17:32:19]

Nit: I think we usually omit such DCHECKs since the line below will cause a
crash and it will already be obvious what's wrong. Also, when you remove it,
also remove the {} since the body will be one line.

+    prober_head_->Start(this);
+  }
+}
 }  // namespace content