Limit number of service instances passed to onServiceList listeners.

chrome/browser/extensions/api/mdns/mdns_api_unittest.cc

Index: chrome/browser/extensions/api/mdns/mdns_api_unittest.cc
diff --git a/chrome/browser/extensions/api/mdns/mdns_api_unittest.cc b/chrome/browser/extensions/api/mdns/mdns_api_unittest.cc
index ede2a2b50d4a03491f54e9f4e8e34ff72d3e852d..6ad54dc541fe67b8e120f497b029213a5c21b48b 100644
--- a/chrome/browser/extensions/api/mdns/mdns_api_unittest.cc
+++ b/chrome/browser/extensions/api/mdns/mdns_api_unittest.cc
@@ -63,10 +63,7 @@ class MDnsAPITest : public extensions::ExtensionServiceTestBase {
         MDnsAPITestingFactoryFunction);
 
     // Create an event router and associate it with the context.
-    extensions::EventRouter* event_router = new extensions::EventRouter(
-        browser_context(),
-        ExtensionPrefsFactory::GetInstance()->GetForBrowserContext(
-            browser_context()));
+    extensions::EventRouter* event_router = CreateEventRouter_();
     static_cast<TestExtensionSystem*>(
         ExtensionSystem::Get(browser_context()))->SetEventRouter(
             scoped_ptr<extensions::EventRouter>(event_router));
@@ -133,6 +130,14 @@ class MDnsAPITest : public extensions::ExtensionServiceTestBase {
     return render_process_host_.get();
   }
 
+ protected:
+  virtual extensions::EventRouter* CreateEventRouter_() {
+    return new extensions::EventRouter(
+        browser_context(),
+        ExtensionPrefsFactory::GetInstance()->GetForBrowserContext(
+            browser_context()));
+  }
+
  private:
   // The registry is owned by MDnsAPI, but MDnsAPI does not have an accessor
   // for it, so use a private member.
@@ -212,6 +217,100 @@ TEST_F(MDnsAPITest, PlatformAppsNotSubjectToWhitelist) {
       filter, false);
 }
 
+class MockEventRouter : public EventRouter {
+ public:
+  explicit MockEventRouter(content::BrowserContext* browser_context,
+                           ExtensionPrefs* extension_prefs) :
+      EventRouter(browser_context, extension_prefs) {}
+  virtual ~MockEventRouter() {}
+
+  virtual void BroadcastEvent(scoped_ptr<Event> event) {
+    BroadcastEventPtr(event.get());
+  }
+  MOCK_METHOD1(BroadcastEventPtr, void(Event* event));
+};
+
+class MDnsAPIMaxServicesTest : public MDnsAPITest {
+ public:
+  MockEventRouter* event_router() {
+    return static_cast<MockEventRouter*>(EventRouter::Get(browser_context()));
+  }
+ protected:
+  virtual extensions::EventRouter* CreateEventRouter_() override {
+    return new MockEventRouter(
+        browser_context(),
+        ExtensionPrefsFactory::GetInstance()->GetForBrowserContext(
+            browser_context()));
+  }
+};
+
+class EventServiceListSizeMatcher :
+      public testing::MatcherInterface<const Event&> {
+ public:
+  explicit EventServiceListSizeMatcher(const testing::Matcher<size_t>& m)
+      : service_count_matcher_(m) {}
+
+  virtual bool MatchAndExplain(const Event& e,
+                               testing::MatchResultListener* listener) const {
+    if (e.event_args.get() == nullptr) {
+      *listener << "event.event_arg is null when it shouldn't be";
+      return false;
+    }
+    if (e.event_args->GetSize() != 1) {
+      *listener << "event.event_arg.GetSize() should be 1 but is "
+                << e.event_args->GetSize();
+      return false;
+    }
+    const base::ListValue* services = nullptr;
+    {
+      const base::Value* out;
+      e.event_args->Get(0, &out);
+      services = static_cast<const base::ListValue*>(out);
+    }
+    if (services == nullptr) {
+      *listener << "event's service list argument is not a ListValue";
+      return false;
+    }
+    *listener << "number of services is "
+              << services->GetSize()
+    return service_count_matcher_.MatchAndExplain(services->GetSize(),

[mark a. foltz, 2015/03/30 17:57:22]

It seems like all you really care about is whether the service list is equal to
the maximum allowed size.  I wonder if you need the custom matcher or can just
match on SizeIs(kMaxServices).

https://code.google.com/p/googlemock/wiki/V1_7_CheatSheet#Container_Matchers

 

[Red Daly, 2015/03/31 19:23:50]

I think SizeIs() only works with stl containers, not a ListValue.  Instead I got
rid of the custom matcher argument and just do a comparison using
testing::Eq(x).

+                                                  listener);
+  }
+
+  virtual void DescribeTo(::std::ostream* os) const {
+    *os << "is an onServiceList event where the number of services ";
+    service_count_matcher_.DescribeTo(os);
+  }
+
+  virtual void DescribeNegationTo(::std::ostream* os) const {
+    *os << "isn't an onServiceList event where the number of services ";
+    service_count_matcher_.DescribeTo(os);
+  }
+ private:
+  testing::Matcher<size_t> service_count_matcher_;
+};
+
+inline testing::Matcher<const Event&> EventServiceListSize(
+    const testing::Matcher<size_t>& m) {
+  return MakeMatcher(new EventServiceListSizeMatcher(m));
+}
+
+TEST_F(MDnsAPIMaxServicesTest, OnServiceListDoesNotExceedLimit) {
+  // Dispatch an mDNS event with more service instances than
+  // kMaxServicesPerOnServiceListEvent, and ensure that the list is truncated by
+  // inspecting the argument to MockEventRouter's BroadcastEvent method.
+  DnsSdRegistry::DnsSdServiceList services;
+  for (int i=0; i < 80; ++i) {

[not at google - send to devlin, 2015/03/30 17:08:50]

when it's a shared constant, use that rather than "80".

[Red Daly, 2015/03/31 19:23:50]

Done, except it is a new function rather than a shared constant.

+    services.push_back(DnsSdService());
+  }
+  EXPECT_CALL(
+      *event_router(),
+      BroadcastEventPtr(
+          testing::Pointee(EventServiceListSize(testing::Eq(size_t(64))))))
+      .Times(1);

[not at google - send to devlin, 2015/03/30 17:08:50]

I dunno - a mock here seems pretty complex and overkill. Can you test it in a
more principled way, like add the services, then get them back and assert that
they're limited? another thing to check, and think about, is how eviction should
work. Should earlier registrations be evicted, vs later registrations ignored?

[Red Daly, 2015/03/31 19:23:50]

The MDnsAPI object does not hold onto the event it generates, so it's not
straightforward to get the truncated service list back without a change in
interface.  Are you suggesting changing the interface?
There are arguments for both of those strategies.  One way or the other, the
extension will not see services that are actually available.
- Evicting older services will mean the API provides extensions with a less
stable list.
- Evicting older services allows an extension to see more of the available
services on a network, getting around the service limit.
- Suppressing new services prevents newly discovered services from appearing,
perhaps interesting ones like a just-plugged-in device.

I'm content to leave this as an optimization in case the 64-service limit is a
problem for someone in the future.

+  dns_sd_registry()->DispatchMDnsEvent("_testing._tcp.local", services);
+}
+
 } // empty namespace
 
 } // namespace extensions