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

content/renderer/media/webrtc/stun_field_trial.h

Index: content/renderer/media/webrtc/stun_field_trial.h
diff --git a/content/renderer/media/webrtc/stun_field_trial.h b/content/renderer/media/webrtc/stun_field_trial.h
index 4095bede6812c01ab81f8b934d2f642a2731e52f..830fd7285642774ad821574612427ab56c2f6457 100644
--- a/content/renderer/media/webrtc/stun_field_trial.h
+++ b/content/renderer/media/webrtc/stun_field_trial.h
@@ -7,42 +7,113 @@
 
 #include <string>
 
+#include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "content/common/content_export.h"
+#include "content/renderer/p2p/network_list_manager.h"
+#include "content/renderer/p2p/network_list_observer.h"
 #include "third_party/webrtc/base/network.h"
+#include "third_party/webrtc/base/sigslot.h"
+#include "third_party/webrtc/p2p/stunprober/stunprober.h"
 
 namespace rtc {
 class PacketSocketFactory;
 class SocketAddress;
 }  // namespace rtc
 
-namespace stunprober {
-class StunProber;
-}  // namespace stunprober
-
 namespace content {
 
+using StunProber = stunprober::StunProber;
+
+// Helper class to link the StunProbers together such that the first STUN ping
+// in the next prober could be sent at the same interval as inside a
+// prober. This is to maintain the consistency of intervals across the whole
+// trial. It uses a weak pointer to trigger next prober's Start(), in case if
+// the owning PeerConnectionDependencyFactory is deleted in between.
+class StunProberWithWeakPtr {
+ public:
+  explicit StunProberWithWeakPtr(StunProber* prober);
+  virtual ~StunProberWithWeakPtr();
+
+  void SetNextProber(StunProberWithWeakPtr* prober);
+  void Start(StunProber::Observer* observer);
+  base::WeakPtr<StunProberWithWeakPtr> GetWeakPtr();
+  bool GetStats(StunProber::Stats* stats);
+  StunProberWithWeakPtr* next_prober() { return next_prober_; }
+
+ private:
+  scoped_ptr<StunProber> prober_;
+  StunProberWithWeakPtr* next_prober_ = nullptr;
+  base::WeakPtrFactory<StunProberWithWeakPtr> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(StunProberWithWeakPtr);
+};
+
 // Wait for 30 seconds to avoid high CPU usage during browser start-up which
 // might affect the accuracy of the trial. The trial wakes up the browser every
 // 1 ms for no more than 3 seconds to see if time has passed for sending next
 // stun probe.
 static const int kExperimentStartDelayMs = 30000;
 
-// This will use |factory| to create sockets, send stun binding requests with
-// various intervals as determined by |params|, observed the success rate and
-// latency of the stun responses and report through UMA.
-scoped_ptr<stunprober::StunProber> StartStunProbeTrial(
-    const rtc::NetworkManager::NetworkList& networks,
-    const std::string& params,
-    rtc::PacketSocketFactory* factory);
-
-// Parsing function to decode the '/' separated parameter string |params|.
-CONTENT_EXPORT bool ParseStunProbeParameters(
-    const std::string& params,
-    int* requests_per_ip,
-    int* interval_ms,
-    int* shared_socket_mode,
-    std::vector<rtc::SocketAddress>* servers);
+class StunProberTrial : public StunProber::Observer,
+                        public sigslot::has_slots<> {
+ public:
+  struct Param {
+    Param(int* requests_per_ip,
+          int* interval_ms,
+          int* shared_socket_mode,
+          int* batch_size,
+          int* total_batches,
+          std::vector<rtc::SocketAddress>* servers)
+        : requests_per_ip(requests_per_ip),
+          interval_ms(interval_ms),
+          shared_socket_mode(shared_socket_mode),
+          batch_size(batch_size),
+          total_batches(total_batches),
+          servers(servers) {}

[Alexei Svitkine (slow), 2015/10/30 18:39:47]

Nit: No need for ctor.

+    int* requests_per_ip;

[Alexei Svitkine (slow), 2015/10/30 18:39:47]

Sorry, this is not quite what I meant.

I meant having the struct have actual fields, rather than pointers:

struct Params {
  int interval_ms = 0;
  int shared_socket_mode = 0;
  ...
  std::vector<rtc::SocketAddress> servers;
};

Then, in StunProberTrial::OnNetworksChanged(), you don't need to have separate
ints on the stack. See my other comments about that.

+    int* interval_ms;
+    int* shared_socket_mode;
+    int* batch_size;
+    int* total_batches;
+    std::vector<rtc::SocketAddress>* servers;
+  };
+
+  StunProberTrial(rtc::NetworkManager* network_manager,
+                  const std::string& params,
+                  rtc::PacketSocketFactory* factory);
+  ~StunProberTrial() override;
+
+  // This will use |factory_| to create sockets, send stun binding requests with
+  // various intervals as determined by |params|, observed the success rate and
+  // latency of the stun responses and report through UMA.
+  void OnNetworksChanged();
+
+  // Parsing function to decode the '/' separated parameter string |params|.
+  static CONTENT_EXPORT bool ParseParameters(const std::string& param_line,
+                                             Param* params);
+
+  // stunprober::StunProber::Observer

[Alexei Svitkine (slow), 2015/10/30 18:39:47]

Nit: Add a : at the end.

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

Done.

+  void OnPrepared(StunProber* prober, StunProber::Status status) override;
+  void OnFinished(StunProber* prober, StunProber::Status status) override;
+
+ private:
+  void SaveHistogramData();
+
+  rtc::NetworkManager* network_manager_;
+  std::string param_line_;
+  rtc::PacketSocketFactory* factory_ = nullptr;
+  int total_probers_ = 0;
+  int batch_size_ = 0;
+  int ready_probers_ = 0;
+  int finished_probers_ = 0;
+  scoped_ptr<StunProberWithWeakPtr> prober_head_;
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(StunProberTrial);
+};
 
 }  // namespace content