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..26f5ec5796c33eb807eaefd8541ea7ac1af706e3 100644
--- a/content/renderer/media/webrtc/stun_field_trial.h
+++ b/content/renderer/media/webrtc/stun_field_trial.h
@@ -7,42 +7,100 @@
 
 #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;

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

style guide doesn't allow 'using' in headers

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

Done.

+
+// 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 weak pointer to trigger next prober's Start(), in case if the
+// owning PeerConnectionDependencyFactory is deleted in between.
+class StunProberWithWeakPtr {

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

It looks to me that this is not just StunPropber with weak pointer. It's
actually linked list of StunProber objects with the elements supporting weak
pointers. Do you really need to reimplement linked list this way? Why not just
use std::list<>.

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

Done.

+ public:
+  explicit StunProberWithWeakPtr(StunProber* prober);
+  virtual ~StunProberWithWeakPtr();
+
+  void set_next_prober(StunProberWithWeakPtr* prober);
+  void Start(StunProber::Observer* observer);
+  base::WeakPtr<StunProberWithWeakPtr> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+  bool GetStats(StunProber::Stats* stats) { return prober_->GetStats(stats); }
+  StunProberWithWeakPtr* GetNextProber() { return next_prober_; }

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

Nit: This code doesn't seem to match the style guide / conventions.

Usually, trivial methods should be inlined and named with hacker_style, while
non-trivial ones should be out of line and named with CamelCase. But here, the
name for a trivial method uses CamelCase which isn't correct and it's setter,
set_next_prober, is not inlined even though its named as a trivial method.
Please make consistent. Also, un-inline non-trivial ones like GetStats(),
GetWeakPtr(), etc.

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

Done.

+
+ 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:
+  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& params,
+      int* requests_per_ip,
+      int* interval_ms,
+      int* shared_socket_mode,
+      int* batch_size,
+      int* total_batches,
+      std::vector<rtc::SocketAddress>* servers);
+
+  // stunprober::StunProber::Observer
+  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 params_;
+  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