Limit number of service instances passed to onServiceList listeners.

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

Index: chrome/browser/extensions/api/mdns/mdns_api.cc
diff --git a/chrome/browser/extensions/api/mdns/mdns_api.cc b/chrome/browser/extensions/api/mdns/mdns_api.cc
index 2a7c41d210931d1540e61b8cb89fe402a203e1e8..2658b34c568cb9c08229730ea5e12b4e0a2f9d58 100644
--- a/chrome/browser/extensions/api/mdns/mdns_api.cc
+++ b/chrome/browser/extensions/api/mdns/mdns_api.cc
@@ -9,7 +9,11 @@
 #include "base/lazy_instance.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/common/extensions/api/mdns.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
+#include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension_messages.h"
 
 namespace extensions {
 
@@ -84,35 +88,7 @@ void MDnsAPI::OnListenerRemoved(const EventListenerInfo& details) {
 
 void MDnsAPI::UpdateMDnsListeners(const EventListenerInfo& details) {
   std::set<std::string> new_service_types;
-
-  // Check all listeners for service type filters.
-  const EventListenerMap::ListenerList& listeners =
-      extensions::EventRouter::Get(browser_context_)
-          ->listeners()
-          .GetEventListenersByName(details.event_name);
-  for (EventListenerMap::ListenerList::const_iterator it = listeners.begin();
-       it != listeners.end(); ++it) {
-    base::DictionaryValue* filter = ((*it)->filter());
-
-    std::string filter_value;
-    filter->GetStringASCII(kEventFilterServiceTypeKey, &filter_value);
-    if (filter_value.empty())
-      continue;
-
-    const Extension* extension = ExtensionRegistry::Get(browser_context_)->
-        enabled_extensions().GetByID((*it)->extension_id());
-    // Don't listen for services associated only with disabled extensions.
-    if (!extension)
-      continue;
-
-    // Platform apps may query for all services; other types of extensions are
-    // restricted to a whitelist.
-    if (!extension->is_platform_app() &&
-        !IsServiceTypeWhitelisted(filter_value))
-      continue;
-
-    new_service_types.insert(filter_value);
-  }
+  GetValidOnServiceListListeners(nullptr, &new_service_types);

[mark a. foltz, 2015/04/03 23:26:40]

What is this nullptr argument?  Optional arguments are discouraged.

Can you have the function new up a set<> and return it instead?

[Red Daly, 2015/04/04 00:56:15]

Done.  I did it the other way to reduce code duplication (one caller needed the
service types but not the listeners).

 
   // Find all the added and removed service types since last update.
   std::set<std::string> added_service_types =
@@ -140,6 +116,16 @@ void MDnsAPI::OnDnsSdEvent(const std::string& service_type,
   std::vector<linked_ptr<mdns::MDnsService> > args;
   for (DnsSdRegistry::DnsSdServiceList::const_iterator it = services.begin();
        it != services.end(); ++it) {
+    if (static_cast<long>(args.size()) ==
+        api::mdns::MAX_SERVICE_INSTANCES_PER_EVENT) {

[mark a. foltz, 2015/04/03 23:26:40]

Can this constant be declared as size_t?

[Red Daly, 2015/04/04 00:56:15]

I don't think there's currently support for that in the IDL parser:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i...

+      WriteToConsole(service_type,
+                     content::CONSOLE_MESSAGE_LEVEL_WARNING,
+                     base::StringPrintf(
+                         "Truncating number of service instances in "

[mark a. foltz, 2015/04/03 23:26:40]

Is there a prefix logged for this?  If not, prepend [chrome.mdns]

[Red Daly, 2015/04/04 00:56:15]

Done.

+                         "onServiceList to maximum allowed: %d",
+                         api::mdns::MAX_SERVICE_INSTANCES_PER_EVENT));

[mark a. foltz, 2015/04/03 23:26:39]

This is not the most meaningful way of notifying the application that something
bad happened.  It will go to the user's console (which most users don't look at)
and the developer will be none the wiser.

Instead, changing the event to pass the number of discovered instances would
allow the caller to know when the list is truncated and tell the user something
meaningful in the extension/app.

I guess this is okay for now (especially since the practical circumstances when
this would be hit are few and far between).

[Red Daly, 2015/04/04 00:56:15]

I agree, added a TODO for the future enhancement.

+      break;
+    }
     linked_ptr<mdns::MDnsService> mdns_service =
         make_linked_ptr(new mdns::MDnsService);
     mdns_service->service_name = (*it).service_name;
@@ -162,4 +148,65 @@ void MDnsAPI::OnDnsSdEvent(const std::string& service_type,
   extensions::EventRouter::Get(browser_context_)->BroadcastEvent(event.Pass());
 }
 
+void MDnsAPI::GetValidOnServiceListListeners(
+    std::set<const EventListener*>* listeners,
+    std::set<std::string>* service_types) {
+ for (const auto& listener :
+          extensions::EventRouter::Get(browser_context_)->listeners()
+          .GetEventListenersByName(mdns::OnServiceList::kEventName)) {
+   base::DictionaryValue* filter = (listener->filter());
+
+   std::string service_type;
+   filter->GetStringASCII(kEventFilterServiceTypeKey, &service_type);
+   if (service_type.empty())
+     continue;
+
+   const Extension* extension = ExtensionRegistry::Get(browser_context_)->
+       enabled_extensions().GetByID(listener->extension_id());
+   // Don't listen for services associated only with disabled extensions.
+   if (!extension)
+     continue;
+
+   // Platform apps may query for all services; other types of extensions are
+   // restricted to a whitelist.
+   if (!extension->is_platform_app() &&
+       !IsServiceTypeWhitelisted(service_type))
+     continue;
+
+   if (listeners)
+     listeners->insert(listener.get());
+   if (service_types)
+     service_types->insert(service_type);
+ }
+}
+
+void MDnsAPI::WriteToConsole(const std::string& service_type,
+                             content::ConsoleMessageLevel level,
+                             const std::string& message) {
+  // Get all the extensions with an onServiceList listener for a particular
+  // service type.
+  std::set<const EventListener*> listeners;
+  GetValidOnServiceListListeners(&listeners, nullptr);
+  std::set<std::string> extension_ids;
+  for (const EventListener* listener : listeners) {
+    extension_ids.insert(listener->extension_id());
+  }
+
+  // Log to the consoles of the background pages for those extensions.
+  for (const std::string& extension_id : extension_ids) {
+    extensions::ExtensionHost* host =
+        extensions::ProcessManager::Get(browser_context_)
+        ->GetBackgroundHostForExtension(extension_id);
+    if (!host)
+      continue;
+    content::RenderViewHost* rvh = host->render_view_host();
+    if (!rvh)
+      continue;
+    rvh->Send(new ExtensionMsg_AddMessageToConsole(
+        rvh->GetRoutingID(),
+        level,
+        message));
+  }
+}
+
 }  // namespace extensions