IpcNetworkManager in renderer/p2p didn't specify the right network prefix,

which prevent grouping mechanism in MergeNetworkList from working.

The end result is that each Network after MergeNetworkList contains only 1 ip address. With this change, the grouping will happen correctly. WebRTC has logic to only retrieve single IP from each network for connectivity check already.

This CL is a dup of https://codereview.chromium.org/536133003/ which was created under the wrong identity.
BUG=

Review URL: https://codereview.chromium.org/565773003

Cr-Commit-Position: refs/heads/master@{#294912}

content/common/p2p_messages.h

@@ -28,6 +28,7 @@ IPC_STRUCT_TRAITS_BEGIN(net::NetworkInterface)
   IPC_STRUCT_TRAITS_MEMBER(name)
   IPC_STRUCT_TRAITS_MEMBER(type)
   IPC_STRUCT_TRAITS_MEMBER(address)
+  IPC_STRUCT_TRAITS_MEMBER(network_prefix)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(rtc::PacketTimeUpdateParams)

content/content_tests.gypi

@@ -709,6 +709,7 @@
         'renderer/media/video_capture_message_filter_unittest.cc',
         'renderer/media/webrtc/video_destination_handler_unittest.cc',
         'renderer/npapi/webplugin_impl_unittest.cc',
+        'renderer/p2p/ipc_network_manager_unittest.cc',
         'renderer/pepper/event_conversion_unittest.cc',
         'renderer/pepper/host_var_tracker_unittest.cc',
         'renderer/pepper/mock_resource.h',

content/renderer/p2p/ipc_network_manager.cc

@@ -3,7 +3,7 @@
 // found in the LICENSE file.
 
 #include "content/renderer/p2p/ipc_network_manager.h"
-
+#include <string>
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
@@ -35,17 +35,17 @@ rtc::AdapterType ConvertConnectionTypeToAdapterType(
 
 }  // namespace
 
-IpcNetworkManager::IpcNetworkManager(P2PSocketDispatcher* socket_dispatcher)
-    : socket_dispatcher_(socket_dispatcher),
+IpcNetworkManager::IpcNetworkManager(NetworkListManager* network_list_manager)
+    : network_list_manager_(network_list_manager),
       start_count_(0),
       network_list_received_(false),
       weak_factory_(this) {
-  socket_dispatcher_->AddNetworkListObserver(this);
+  network_list_manager_->AddNetworkListObserver(this);
 }
 
 IpcNetworkManager::~IpcNetworkManager() {
   DCHECK(!start_count_);
-  socket_dispatcher_->RemoveNetworkListObserver(this);
+  network_list_manager_->RemoveNetworkListObserver(this);
 }
 
 void IpcNetworkManager::StartUpdating() {
@@ -71,8 +71,6 @@ void IpcNetworkManager::OnNetworkListChanged(
   if (!network_list_received_)
     network_list_received_ = true;
 
-  // Note: 32 and 64 are the arbitrary(kind of) prefix length used to
-  // differentiate IPv4 and IPv6 addresses.
   // rtc::Network uses these prefix_length to compare network
   // interfaces discovered.
   std::vector<rtc::Network*> networks;
@@ -84,9 +82,14 @@ void IpcNetworkManager::OnNetworkListChanged(
       uint32 address;
       memcpy(&address, &it->address[0], sizeof(uint32));
       address = rtc::NetworkToHost32(address);
-      rtc::Network* network = new rtc::Network(
-          it->name, it->name, rtc::IPAddress(address), 32,
-          ConvertConnectionTypeToAdapterType(it->type));
+      rtc::IPAddress prefix =
+          rtc::TruncateIP(rtc::IPAddress(address), it->network_prefix);
+      rtc::Network* network =
+          new rtc::Network(it->name,
+                           it->name,
+                           prefix,
+                           it->network_prefix,
+                           ConvertConnectionTypeToAdapterType(it->type));
       network->AddIP(rtc::IPAddress(address));
       networks.push_back(network);
       ++ipv4_interfaces;
@@ -95,9 +98,14 @@ void IpcNetworkManager::OnNetworkListChanged(
       memcpy(&address, &it->address[0], sizeof(in6_addr));
       rtc::IPAddress ip6_addr(address);
       if (!rtc::IPIsPrivate(ip6_addr)) {
-        rtc::Network* network = new rtc::Network(
-            it->name, it->name, ip6_addr, 64,
-            ConvertConnectionTypeToAdapterType(it->type));
+        rtc::IPAddress prefix =
+            rtc::TruncateIP(rtc::IPAddress(ip6_addr), it->network_prefix);
+        rtc::Network* network =
+            new rtc::Network(it->name,
+                             it->name,
+                             prefix,
+                             it->network_prefix,
+                             ConvertConnectionTypeToAdapterType(it->type));
         network->AddIP(ip6_addr);
         networks.push_back(network);
         ++ipv6_interfaces;

content/renderer/p2p/ipc_network_manager.h

@@ -10,8 +10,8 @@
 #include "base/compiler_specific.h"
 #include "base/memory/weak_ptr.h"
 #include "content/common/content_export.h"
+#include "content/renderer/p2p/network_list_manager.h"
 #include "content/renderer/p2p/network_list_observer.h"
-#include "content/renderer/p2p/socket_dispatcher.h"
 #include "third_party/webrtc/base/network.h"
 
 namespace content {
@@ -21,8 +21,8 @@ namespace content {
 class IpcNetworkManager : public rtc::NetworkManagerBase,
                           public NetworkListObserver {
  public:
-  // Constructor doesn't take ownership of the |socket_dispatcher|.
-  CONTENT_EXPORT IpcNetworkManager(P2PSocketDispatcher* socket_dispatcher);
+  // Constructor doesn't take ownership of the |network_list_manager|.
+  CONTENT_EXPORT IpcNetworkManager(NetworkListManager* network_list_manager);
   virtual ~IpcNetworkManager();
 
   virtual void StartUpdating() OVERRIDE;
@@ -35,7 +35,7 @@ class IpcNetworkManager : public rtc::NetworkManagerBase,
  private:
   void SendNetworksChangedSignal();
 
-  P2PSocketDispatcher* socket_dispatcher_;
+  NetworkListManager* network_list_manager_;
   int start_count_;
   bool network_list_received_;
 

content/renderer/p2p/ipc_network_manager_unittest.cc

+// Copyright 2014 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 "base/memory/scoped_ptr.h"
+#include "content/renderer/p2p/ipc_network_manager.h"
+#include "content/renderer/p2p/network_list_manager.h"
+#include "net/base/net_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+namespace {
+
+class MockP2PSocketDispatcher : public NetworkListManager {
+ public:
+  virtual void AddNetworkListObserver(
+      NetworkListObserver* network_list_observer) OVERRIDE {}
+
+  virtual void RemoveNetworkListObserver(
+      NetworkListObserver* network_list_observer) OVERRIDE {}
+
+  virtual ~MockP2PSocketDispatcher() {}
+};
+
+}  // namespace
+
+// 2 IPv6 addresses with only last digit different.
+static const char kIPv6PublicAddrString1[] =
+    "2401:fa00:4:1000:be30:5bff:fee5:c3";
+static const char kIPv6PublicAddrString2[] =
+    "2401:fa00:4:1000:be30:5bff:fee5:c4";
+
+class IpcNetworkManagerTest : public testing::Test {
+ public:
+  IpcNetworkManagerTest()
+      : network_list_manager_(new MockP2PSocketDispatcher()),
+        network_manager_(new IpcNetworkManager(network_list_manager_.get())) {}
+
+ protected:
+  scoped_ptr<MockP2PSocketDispatcher> network_list_manager_;
+  scoped_ptr<IpcNetworkManager> network_manager_;
+};
+
+// Test overall logic of IpcNetworkManager on OnNetworkListChanged
+// that it should group addresses with the same network key under
+// single Network class. This also tests the logic inside
+// IpcNetworkManager in addition to MergeNetworkList.
+// TODO(guoweis): disable this test case for now until fix for webrtc
+// issue 19249005 integrated into chromium
+TEST_F(IpcNetworkManagerTest, DISABLED_TestMergeNetworkList) {
+  net::NetworkInterfaceList list;
+  net::IPAddressNumber ip_number;
+  std::vector<rtc::Network*> networks;
+  rtc::IPAddress ip_address;
+
+  // Add 2 networks with the same prefix and prefix length.
+  EXPECT_TRUE(net::ParseIPLiteralToNumber(kIPv6PublicAddrString1, &ip_number));
+  list.push_back(
+      net::NetworkInterface("em1",
+                            "em1",
+                            0,
+                            net::NetworkChangeNotifier::CONNECTION_UNKNOWN,
+                            ip_number,
+                            64));
+
+  EXPECT_TRUE(net::ParseIPLiteralToNumber(kIPv6PublicAddrString2, &ip_number));
+  list.push_back(
+      net::NetworkInterface("em1",
+                            "em1",
+                            0,
+                            net::NetworkChangeNotifier::CONNECTION_UNKNOWN,
+                            ip_number,
+                            64));
+
+  network_manager_->OnNetworkListChanged(list);
+  network_manager_->GetNetworks(&networks);
+  EXPECT_EQ(1uL, networks.size());
+  EXPECT_EQ(2uL, networks[0]->GetIPs().size());
+
+  // Add another network with different prefix length, should result in
+  // a different network.
+  networks.clear();
+  list.push_back(
+      net::NetworkInterface("em1",
+                            "em1",
+                            0,
+                            net::NetworkChangeNotifier::CONNECTION_UNKNOWN,
+                            ip_number,
+                            48));
+
+  network_manager_->OnNetworkListChanged(list);
+
+  network_manager_->GetNetworks(&networks);
+
+  // Verify we have 2 networks now.
+  EXPECT_EQ(2uL, networks.size());
+  // Verify the network with prefix length of 64 has 2 IP addresses.
+  EXPECT_EQ(64, networks[1]->prefix_length());
+  EXPECT_EQ(2uL, networks[1]->GetIPs().size());
+  EXPECT_TRUE(rtc::IPFromString(kIPv6PublicAddrString1, &ip_address));
+  EXPECT_EQ(networks[1]->GetIPs()[0], ip_address);
+  EXPECT_TRUE(rtc::IPFromString(kIPv6PublicAddrString2, &ip_address));
+  EXPECT_EQ(networks[1]->GetIPs()[1], ip_address);
+  // Verify the network with prefix length of 48 has 2 IP addresses.
+  EXPECT_EQ(48, networks[0]->prefix_length());
+  EXPECT_EQ(1uL, networks[0]->GetIPs().size());
+  EXPECT_TRUE(rtc::IPFromString(kIPv6PublicAddrString2, &ip_address));
+  EXPECT_EQ(networks[0]->GetIPs()[0], ip_address);
+}
+
+}  // namespace content

content/renderer/p2p/network_list_manager.h

+// Copyright 2014 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.
+
+// NetworkListManager interface is introduced to enable unit test on
+// IpcNetworkManager such that it doesn't depend on implementation of
+// P2PSocketDispatcher.
+
+#ifndef CONTENT_RENDERER_P2P_NETWORK_LIST_MANAGER_H_
+#define CONTENT_RENDERER_P2P_NETWORK_LIST_MANAGER_H_
+
+namespace content {
+
+class NetworkListObserver;
+
+class CONTENT_EXPORT NetworkListManager {
+ public:
+  // Add a new network list observer. Each observer is called
+  // immidiately after it is registered and then later whenever
+  // network configuration changes. Can be called on any thread. The
+  // observer is always called on the thread it was added.
+  virtual void AddNetworkListObserver(
+      NetworkListObserver* network_list_observer) = 0;
+
+  // Removes network list observer. Must be called on the thread on
+  // which the observer was added.
+  virtual void RemoveNetworkListObserver(
+      NetworkListObserver* network_list_observer) = 0;
+
+ protected:
+  // Marked as protected to prevent explicit deletion, as
+  // P2PSocketDispatcher is not owned by IpcNetworkManager.
+  virtual ~NetworkListManager() {}
+};
+
+}  // namespace content
+
+#endif  // CONTENT_RENDERER_P2P_NETWORK_LIST_MANAGER_H_

content/renderer/p2p/socket_dispatcher.h

@@ -30,6 +30,7 @@
 #include "base/synchronization/lock.h"
 #include "content/common/content_export.h"
 #include "content/common/p2p_socket_type.h"
+#include "content/renderer/p2p/network_list_manager.h"
 #include "ipc/message_filter.h"
 #include "net/base/net_util.h"
 
@@ -48,19 +49,16 @@ class P2PAsyncAddressResolver;
 class P2PSocketClientImpl;
 class RenderViewImpl;
 
-class CONTENT_EXPORT P2PSocketDispatcher : public IPC::MessageFilter {
+class CONTENT_EXPORT P2PSocketDispatcher : public IPC::MessageFilter,
+                                           public NetworkListManager {
  public:
   explicit P2PSocketDispatcher(base::MessageLoopProxy* ipc_message_loop);
 
-  // Add a new network list observer. Each observer is called
-  // immidiately after it is registered and then later whenever
-  // network configuration changes. Can be called on any thread. The
-  // observer is always called on the thread it was added.
-  void AddNetworkListObserver(NetworkListObserver* network_list_observer);
-
-  // Removes network list observer. Must be called on the thread on
-  // which the observer was added.
-  void RemoveNetworkListObserver(NetworkListObserver* network_list_observer);
+  // NetworkListManager interface:
+  virtual void AddNetworkListObserver(
+      NetworkListObserver* network_list_observer) OVERRIDE;
+  virtual void RemoveNetworkListObserver(
+      NetworkListObserver* network_list_observer) OVERRIDE;
 
  protected:
   virtual ~P2PSocketDispatcher();

net/base/net_util.cc

@@ -1004,7 +1004,7 @@ NetworkInterface::NetworkInterface(const std::string& name,
                                    uint32 interface_index,
                                    NetworkChangeNotifier::ConnectionType type,
                                    const IPAddressNumber& address,
-                                   size_t network_prefix)
+                                   uint32 network_prefix)
     : name(name),
       friendly_name(friendly_name),
       interface_index(interface_index),

net/base/net_util.h

@@ -447,7 +447,7 @@ struct NET_EXPORT NetworkInterface {
                    uint32 interface_index,
                    NetworkChangeNotifier::ConnectionType type,
                    const IPAddressNumber& address,
-                   size_t network_prefix);
+                   uint32 network_prefix);
   ~NetworkInterface();
 
   std::string name;
@@ -455,7 +455,7 @@ struct NET_EXPORT NetworkInterface {
   uint32 interface_index;  // Always 0 on Android.
   NetworkChangeNotifier::ConnectionType type;
   IPAddressNumber address;
-  size_t network_prefix;
+  uint32 network_prefix;
 };
 
 typedef std::vector<NetworkInterface> NetworkInterfaceList;