network_diagnostics: Make UdpProber class abstract

To facillate testing in classes that use the UdpProber, create an
abstract UdpProber. Tests can derive from this abstract UdpProber class.

BUG=b/172994051
TEST=unit_tests --gtest_filter=UdpProberWithFakeNetworkContextTest.*

Change-Id: Ibdfba9690e996e211f1db85888f66fe15f24c75e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545047
Commit-Queue: Kartik Hegde <khegde@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828923}

chrome/browser/chromeos/net/network_diagnostics/udp_prober.cc

@@ -4,24 +4,124 @@
 
 #include "chrome/browser/chromeos/net/network_diagnostics/udp_prober.h"
 
+#include <cstdint>
+#include <memory>
 #include <utility>
 
+#include "base/callback.h"
+#include "base/containers/span.h"
 #include "base/logging.h"
+#include "base/memory/weak_ptr.h"
+#include "base/optional.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "chrome/browser/chromeos/net/network_diagnostics/host_resolver.h"
+#include "chrome/browser/chromeos/net/network_diagnostics/udp_prober.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
 #include "net/base/address_list.h"
 #include "net/base/host_port_pair.h"
+#include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
 #include "net/dns/public/resolve_error_info.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/network/public/mojom/network_context.mojom.h"
+#include "services/network/public/mojom/udp_socket.mojom.h"
+#include "url/gurl.h"
 
 namespace chromeos {
 namespace network_diagnostics {
 
-UdpProber::UdpProber(NetworkContextGetter network_context_getter,
+// Implements the UdpProber class.
+class UdpProberImpl final : public network::mojom::UDPSocketListener,
+                            public UdpProber {
+ public:
+  using ConnectCallback = base::OnceCallback<
+      void(int result, const base::Optional<net::IPEndPoint>& local_addr_out)>;
+  using SendCallback = base::OnceCallback<void(int result)>;
+
+  // Establishes a UDP connection and sends |data| to |host_port_pair|. The
+  // traffic sent by the prober is described by |tag|. Since there is no
+  // guarantee the host specified by |host_port_pair| will respond to a UDP
+  // request, the prober will timeout with a failure after
+  // |timeout_after_host_resolution|. As such, the prober will run no longer
+  // than the time it takes to perform host resolution +
+  // |timeout_after_host_resolution|. Note that the constructor will not invoke
+  // |callback|, which is passed into UdpProberImpl during construction. This
+  // ensures the UdpProberImpl instance is constructed before |callback| is
+  // invoked.  The UdpProberImpl must be created on the UI thread and will
+  // invoke |callback| on the UI thread.  |network_context_getter| will be
+  // invoked on the UI thread.
+  UdpProberImpl(NetworkContextGetter network_context_getter,
+                net::HostPortPair host_port_pair,
+                base::span<const uint8_t> data,
+                net::NetworkTrafficAnnotationTag tag,
+                base::TimeDelta timeout_after_host_resolution,
+                UdpProbeCompleteCallback callback);
+  UdpProberImpl(const UdpProberImpl&) = delete;
+  UdpProberImpl& operator=(const UdpProberImpl&) = delete;
+  ~UdpProberImpl() override;
+
+ private:
+  // Processes the results of the DNS resolution done by |host_resolver_|.
+  void OnHostResolutionComplete(
+      HostResolver::ResolutionResult& resolution_result);
+
+  // On success, the UDP socket is connected to the destination and is ready to
+  // send data. On failure, the UdpProberImpl exits with a failure.
+  void OnConnectComplete(int result,
+                         const base::Optional<net::IPEndPoint>& local_addr_out);
+
+  // On success, the UDP socket is ready to receive data. So long as the
+  // received data is not empty, it is considered valid. The content itself is
+  // not verified.
+  void OnSendComplete(int result);
+
+  // network::mojom::UDPSocketListener:
+  void OnReceived(int32_t result,
+                  const base::Optional<net::IPEndPoint>& src_ip,
+                  base::Optional<base::span<const uint8_t>> data) override;
+
+  // Signals the end of the probe. Manages the clean up and returns a response
+  // to the caller.
+  void OnDone(int result, ProbeExitEnum probe_exit_enum);
+
+  // Handles disconnects on the UDPSocket remote and UDPSocketListener receiver.
+  void OnDisconnect();
+
+  // Gets the active profile-specific network context.
+  NetworkContextGetter network_context_getter_;
+  // Contains the hostname and port.
+  net::HostPortPair host_port_pair_;
+  // Data to be sent to the destination.
+  base::span<const uint8_t> data_;
+  // Network annotation tag describing the socket traffic.
+  net::NetworkTrafficAnnotationTag tag_;
+  // Represents the time after host resolution.
+  base::TimeDelta timeout_after_host_resolution_;
+  // Times the prober.
+  base::OneShotTimer timer_;
+  // Host resolver used for DNS lookup.
+  std::unique_ptr<HostResolver> host_resolver_;
+  // Stores the callback invoked once probe is complete or interrupted.
+  UdpProbeCompleteCallback callback_;
+  // Holds the UDPSocket remote.
+  mojo::Remote<network::mojom::UDPSocket> udp_socket_remote_;
+  // Listens to the response from hostname specified by |url_|.
+  mojo::Receiver<network::mojom::UDPSocketListener>
+      udp_socket_listener_receiver_{this};
+
+  // Must be the last member so that any callbacks taking a weak pointer to this
+  // instance are invalidated first during object destruction.
+  base::WeakPtrFactory<UdpProberImpl> weak_factory_{this};
+};
+
+UdpProberImpl::UdpProberImpl(NetworkContextGetter network_context_getter,
                              net::HostPortPair host_port_pair,
                              base::span<const uint8_t> data,
                              net::NetworkTrafficAnnotationTag tag,
@@ -44,13 +144,13 @@ UdpProber::UdpProber(NetworkContextGetter network_context_getter,
 
   host_resolver_ = std::make_unique<HostResolver>(
       host_port_pair_, network_context,
-      base::BindOnce(&UdpProber::OnHostResolutionComplete,
+      base::BindOnce(&UdpProberImpl::OnHostResolutionComplete,
                      weak_factory_.GetWeakPtr()));
 }
 
-UdpProber::~UdpProber() = default;
+UdpProberImpl::~UdpProberImpl() = default;
 
-void UdpProber::OnHostResolutionComplete(
+void UdpProberImpl::OnHostResolutionComplete(
     HostResolver::ResolutionResult& resolution_result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -68,26 +168,27 @@ void UdpProber::OnHostResolutionComplete(
 
   auto pending_receiver = udp_socket_remote_.BindNewPipeAndPassReceiver();
   udp_socket_remote_.set_disconnect_handler(
-      base::BindOnce(&UdpProber::OnDisconnect, weak_factory_.GetWeakPtr()));
+      base::BindOnce(&UdpProberImpl::OnDisconnect, weak_factory_.GetWeakPtr()));
   DCHECK(udp_socket_remote_.is_bound());
 
   auto pending_remote =
       udp_socket_listener_receiver_.BindNewPipeAndPassRemote();
   udp_socket_listener_receiver_.set_disconnect_handler(
-      base::BindOnce(&UdpProber::OnDisconnect, weak_factory_.GetWeakPtr()));
+      base::BindOnce(&UdpProberImpl::OnDisconnect, weak_factory_.GetWeakPtr()));
 
   network_context->CreateUDPSocket(std::move(pending_receiver),
                                    std::move(pending_remote));
-  timer_.Start(FROM_HERE, timeout_after_host_resolution_,
-               base::BindOnce(&UdpProber::OnDone, weak_factory_.GetWeakPtr(),
+  timer_.Start(
+      FROM_HERE, timeout_after_host_resolution_,
+      base::BindOnce(&UdpProberImpl::OnDone, weak_factory_.GetWeakPtr(),
                      net::ERR_TIMED_OUT, ProbeExitEnum::kTimeout));
   udp_socket_remote_->Connect(
       resolution_result.resolved_addresses.value().front(), nullptr,
-      base::BindOnce(&UdpProber::OnConnectComplete,
+      base::BindOnce(&UdpProberImpl::OnConnectComplete,
                      weak_factory_.GetWeakPtr()));
 }
 
-void UdpProber::OnConnectComplete(
+void UdpProberImpl::OnConnectComplete(
     int result,
     const base::Optional<net::IPEndPoint>& local_addr_out) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -95,12 +196,13 @@ void UdpProber::OnConnectComplete(
     OnDone(result, ProbeExitEnum::kConnectFailure);
     return;
   }
-  udp_socket_remote_->Send(
-      std::move(data_), net::MutableNetworkTrafficAnnotationTag(tag_),
-      base::BindOnce(&UdpProber::OnSendComplete, weak_factory_.GetWeakPtr()));
+  udp_socket_remote_->Send(std::move(data_),
+                           net::MutableNetworkTrafficAnnotationTag(tag_),
+                           base::BindOnce(&UdpProberImpl::OnSendComplete,
+                                          weak_factory_.GetWeakPtr()));
 }
 
-void UdpProber::OnSendComplete(int result) {
+void UdpProberImpl::OnSendComplete(int result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (result != net::OK) {
@@ -110,7 +212,7 @@ void UdpProber::OnSendComplete(int result) {
   udp_socket_remote_->ReceiveMore(/*num_additional_datagrams=*/1);
 }
 
-void UdpProber::OnReceived(int32_t result,
+void UdpProberImpl::OnReceived(int32_t result,
                                const base::Optional<net::IPEndPoint>& src_ip,
                                base::Optional<base::span<const uint8_t>> data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -120,7 +222,7 @@ void UdpProber::OnReceived(int32_t result,
     return;
   }
 
-  // The UdpProber instance is only interested in validating whether
+  // The UdpProberImpl instance is only interested in validating whether
   // data can be received from the destination host.
   if (!data.has_value() || data.value().empty()) {
     // Note that net::ERR_FAILED is reported even if |result| is net::OK
@@ -131,7 +233,7 @@ void UdpProber::OnReceived(int32_t result,
   OnDone(net::OK, ProbeExitEnum::kSuccess);
 }
 
-void UdpProber::OnDone(int result, ProbeExitEnum probe_exit_enum) {
+void UdpProberImpl::OnDone(int result, ProbeExitEnum probe_exit_enum) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Invalidate pending callbacks.
@@ -145,11 +247,24 @@ void UdpProber::OnDone(int result, ProbeExitEnum probe_exit_enum) {
   std::move(callback_).Run(result, probe_exit_enum);
 }
 
-void UdpProber::OnDisconnect() {
+void UdpProberImpl::OnDisconnect() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   OnDone(net::ERR_FAILED, ProbeExitEnum::kMojoDisconnectFailure);
 }
 
+// static
+std::unique_ptr<UdpProber> UdpProber::Start(
+    NetworkContextGetter network_context_getter,
+    net::HostPortPair host_port_pair,
+    base::span<const uint8_t> data,
+    net::NetworkTrafficAnnotationTag tag,
+    base::TimeDelta timeout_after_host_resolution,
+    UdpProbeCompleteCallback callback) {
+  return std::make_unique<UdpProberImpl>(
+      std::move(network_context_getter), host_port_pair, std::move(data), tag,
+      timeout_after_host_resolution, std::move(callback));
+}
+
 }  // namespace network_diagnostics
 }  // namespace chromeos

chrome/browser/chromeos/net/network_diagnostics/udp_prober.h

@@ -6,23 +6,14 @@
 #define CHROME_BROWSER_CHROMEOS_NET_NETWORK_DIAGNOSTICS_UDP_PROBER_H_
 
 #include <cstdint>
+#include <memory>
 
 #include "base/callback.h"
 #include "base/containers/span.h"
-#include "base/memory/weak_ptr.h"
-#include "base/optional.h"
 #include "base/time/time.h"
-#include "base/timer/timer.h"
-#include "chrome/browser/chromeos/net/network_diagnostics/host_resolver.h"
-#include "mojo/public/cpp/bindings/receiver.h"
-#include "mojo/public/cpp/bindings/remote.h"
-#include "net/base/address_list.h"
-#include "net/base/ip_endpoint.h"
-#include "net/dns/public/resolve_error_info.h"
+#include "net/base/host_port_pair.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/mojom/network_context.mojom.h"
-#include "services/network/public/mojom/udp_socket.mojom.h"
-#include "url/gurl.h"
 
 namespace chromeos {
 namespace network_diagnostics {
@@ -31,7 +22,7 @@ namespace network_diagnostics {
 // the prober listens for received data. It confirms that data was received but
 // does not validate the content, hence no data is parsed. Used by network
 // diagnostic routines.
-class UdpProber : public network::mojom::UDPSocketListener {
+class UdpProber {
  public:
   // Lists the ways a prober may end. The callback passed into the prober's
   // constructor is invoked while exiting.
@@ -48,86 +39,20 @@ class UdpProber : public network::mojom::UDPSocketListener {
 
   using NetworkContextGetter =
       base::RepeatingCallback<network::mojom::NetworkContext*()>;
-  using ConnectCallback = base::OnceCallback<
-      void(int result, const base::Optional<net::IPEndPoint>& local_addr_out)>;
-  using SendCallback = base::OnceCallback<void(int result)>;
   using UdpProbeCompleteCallback =
       base::OnceCallback<void(int result, ProbeExitEnum probe_exit_enum)>;
 
-  // Establishes a UDP connection and sends |data| to |host_port_pair|. The
-  // traffic sent by the prober is described by |tag|. Since there is no
-  // guarantee the host specified by |host_port_pair| will respond to a UDP
-  // request, the prober will timeout with a failure after
-  // |timeout_after_host_resolution|. As such, the prober will run no longer
-  // than the time it takes to perform host resolution +
-  // |timeout_after_host_resolution|. Note that the constructor will not invoke
-  // |callback|, which is passed into UdpProber during construction. This
-  // ensures the UdpProber instance is constructed before |callback| is invoked.
-  // The UdpProber must be created on the UI thread and will invoke |callback|
-  // on the UI thread.  |network_context_getter| will be invoked on the UI
-  // thread.
-  UdpProber(NetworkContextGetter network_context_getter,
+  virtual ~UdpProber() = default;
+
+  // Creates a UdpProber instance which resolves |host_port_pair| and starts the
+  // UDP probe.  See implementation for more details.
+  static std::unique_ptr<UdpProber> Start(
+      NetworkContextGetter network_context_getter,
       net::HostPortPair host_port_pair,
       base::span<const uint8_t> data,
       net::NetworkTrafficAnnotationTag tag,
       base::TimeDelta timeout_after_host_resolution,
       UdpProbeCompleteCallback callback);
-  UdpProber(const UdpProber&) = delete;
-  UdpProber& operator=(const UdpProber&) = delete;
-  ~UdpProber() override;
-
- private:
-  // Processes the results of the DNS resolution done by |host_resolver_|.
-  void OnHostResolutionComplete(
-      HostResolver::ResolutionResult& resolution_result);
-
-  // On success, the UDP socket is connected to the destination and is ready to
-  // send data. On failure, the UdpProber exits with a failure.
-  void OnConnectComplete(int result,
-                         const base::Optional<net::IPEndPoint>& local_addr_out);
-
-  // On success, the UDP socket is ready to receive data. So long as the
-  // received data is not empty, it is considered valid. The content itself is
-  // not verified.
-  void OnSendComplete(int result);
-
-  // network::mojom::UDPSocketListener:
-  void OnReceived(int32_t result,
-                  const base::Optional<net::IPEndPoint>& src_ip,
-                  base::Optional<base::span<const uint8_t>> data) override;
-
-  // Signals the end of the probe. Manages the clean up and returns a response
-  // to the caller.
-  void OnDone(int result, ProbeExitEnum probe_exit_enum);
-
-  // Handles disconnects on the UDPSocket remote and UDPSocketListener receiver.
-  void OnDisconnect();
-
-  // Gets the active profile-specific network context.
-  NetworkContextGetter network_context_getter_;
-  // Contains the hostname and port.
-  net::HostPortPair host_port_pair_;
-  // Data to be sent to the destination.
-  base::span<const uint8_t> data_;
-  // Network annotation tag describing the socket traffic.
-  net::NetworkTrafficAnnotationTag tag_;
-  // Represents the time after host resolution.
-  base::TimeDelta timeout_after_host_resolution_;
-  // Times the prober.
-  base::OneShotTimer timer_;
-  // Host resolver used for DNS lookup.
-  std::unique_ptr<HostResolver> host_resolver_;
-  // Stores the callback invoked once probe is complete or interrupted.
-  UdpProbeCompleteCallback callback_;
-  // Holds the UDPSocket remote.
-  mojo::Remote<network::mojom::UDPSocket> udp_socket_remote_;
-  // Listens to the response from hostname specified by |url_|.
-  mojo::Receiver<network::mojom::UDPSocketListener>
-      udp_socket_listener_receiver_{this};
-
-  // Must be the last member so that any callbacks taking a weak pointer to this
-  // instance are invalidated first during object destruction.
-  base::WeakPtrFactory<UdpProber> weak_factory_{this};
 };
 
 }  // namespace network_diagnostics

chrome/browser/chromeos/net/network_diagnostics/udp_prober_unittest.cc

@@ -89,7 +89,7 @@ class UdpProberWithFakeNetworkContextTest : public ::testing::Test {
   void CreateAndExecuteUdpProber(base::span<const uint8_t> data,
                                  UdpProber::UdpProbeCompleteCallback callback) {
     ASSERT_TRUE(fake_network_context_);
-    udp_prober_ = std::make_unique<UdpProber>(
+    udp_prober_ = UdpProber::Start(
         base::BindRepeating(
             [](network::mojom::NetworkContext* network_context) {
               return network_context;