Fix semantics of ExtensionWebRequestEventRouter::EventListeners.

extensions/browser/api/web_request/web_request_api.h

Index: extensions/browser/api/web_request/web_request_api.h
diff --git a/extensions/browser/api/web_request/web_request_api.h b/extensions/browser/api/web_request/web_request_api.h
index 796d75fe914c714f50405204f8ff54a4af462949..75c772402cbcec990fd975ae68be3cb6f0b4ec0a 100644
--- a/extensions/browser/api/web_request/web_request_api.h
+++ b/extensions/browser/api/web_request/web_request_api.h
@@ -14,6 +14,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "base/memory/weak_ptr.h"
@@ -293,14 +294,6 @@ class ExtensionWebRequestEventRouter
                         int web_view_instance_id,
                         base::WeakPtr<IPC::Sender> ipc_sender);
 
-  // Removes the listener for the given sub-event.
-  void RemoveEventListener(
-      void* browser_context,
-      const std::string& extension_id,
-      const std::string& sub_event_name,
-      int embedder_process_id,
-      int web_view_instance_id);
-
   // Removes the listeners for a given <webview>.
   void RemoveWebViewEventListeners(
       void* browser_context,
@@ -318,12 +311,87 @@ class ExtensionWebRequestEventRouter
   void AddCallbackForPageLoad(const base::Closure& callback);
 
  private:
+  friend class WebRequestAPI;
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest,
+                           BlockingEventPrecedenceRedirect);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest,
+                           BlockingEventPrecedenceCancel);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest,
+                           SimulateChancelWhileBlocked);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest, AccessRequestBodyData);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest,
+                           MinimalAccessRequestBodyData);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest, NoAccessRequestBodyData);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest, AddAndRemoveListeners);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestTest, BlockedRequestsAreRemoved);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionWebRequestHeaderModificationTest,
+                           TestModifications);
+
+  struct EventListener {
+    // An EventListener is uniquely defined by five properties.

[Devlin, 2016/09/07 19:18:00]

This isn't strictly true, right?  We potentially have multiple listeners with
the same id.  At least, we don't enforce we don't, and you have a test that
does...

And the more I look at it, the more that scares me.  *Can* we enforce that
there's only one listener with a given id?

[erikchen, 2016/09/07 22:07:43]

Two responses:
1) This comment was based on my interpretation of the code in
ExtensionWebRequestEventRouter::AddEventListener:
"""
if (base::ContainsKey(listeners_[browser_context][event_name], listener)) {
 // This is likely an abuse of the API by a malicious extension.
  return false;
}

2) I don't know anything about the semantics of WebRequest Event Listeners. If
it should be possible to have multiple listeners with the same "id", then the
previous code was wrong and should be fixed in a followup CL.

[Devlin, 2016/09/08 17:33:12]

Yep, that and others are why I was scared. :)
Can we add a TODO and avoid using multiple listeners with the same id in new
tests?

[erikchen, 2016/09/08 18:34:41]

I added a long TODO & comment. We still need to use the "same id" for non-strict
removal of event listeners (embedder_process_id = 0). Note that that test was
added in https://codereview.chromium.org/2121873002. I'd be willing to believe
that for non web view event listeners, the tuple of (browser_context,
extension_id, sub_event_name) already uniquely specifies an extension, in which
case the test would be incorrect and misleading, but that's not immediately
clear to me.

+    struct Identifier {
+      Identifier(void* browser_context,
+                 const std::string& extension_id,
+                 const std::string& sub_event_name,
+                 int embedder_process_id,
+                 int web_view_instance_id)
+          : browser_context(browser_context),

[Devlin, 2016/09/07 19:18:00]

nit: these methods should probably be defined in the .cc.

[erikchen, 2016/09/07 22:07:43]

Done.

+            extension_id(extension_id),
+            sub_event_name(sub_event_name),
+            embedder_process_id(embedder_process_id),
+            web_view_instance_id(web_view_instance_id) {}
+
+      // If web_view_instance_id is 0, then ignore embedder_process_id.
+      bool LooselyMatches(const Identifier& that) const {

[Devlin, 2016/09/07 19:18:00]

Is there a time we don't want to do this?  i.e., why not just have this be the
operator== implementation?

[erikchen, 2016/09/07 22:07:43]

We can't use operator== everywhere because we aren't guaranteed to have the
right embedder_process_id at certain points in time (see the reverted CL). We
can't use LooselyMatches since it's possible for multiple IDs to "loosely match"
when a web_view_instance_id exists.

[Devlin, 2016/09/08 17:33:12]

Ah, I see.  This is annoying.  Can we add another TODO to remove this, so that
the world is slightly more sane? :)

[erikchen, 2016/09/08 18:34:42]

Added a TODO.

+        if (web_view_instance_id == 0 && that.web_view_instance_id == 0) {
+          // Since EventListeners are segmented by browser_context, check that
+          // last, as it is exceedingly unlikely to be different.
+          return extension_id == that.extension_id &&
+                 sub_event_name == that.sub_event_name &&
+                 browser_context == that.browser_context;
+        }
+
+        return *this == that;
+      }
+
+      bool operator==(const Identifier& that) const {
+        // Since EventListeners are segmented by browser_context, check that
+        // last, as it is exceedingly unlikely to be different.
+        return extension_id == that.extension_id &&
+               sub_event_name == that.sub_event_name &&
+               web_view_instance_id == that.web_view_instance_id &&
+               embedder_process_id == that.embedder_process_id &&
+               browser_context == that.browser_context;
+      }
+
+      void* browser_context;
+      std::string extension_id;
+      std::string sub_event_name;
+      int embedder_process_id;
+      int web_view_instance_id;
+    };
+
+    EventListener(Identifier id);
+    ~EventListener();
+
+    const Identifier id;
+    std::string extension_name;
+    events::HistogramValue histogram_value = events::UNKNOWN;
+    RequestFilter filter;
+    int extra_info_spec = 0;
+    base::WeakPtr<IPC::Sender> ipc_sender;
+    std::unordered_set<uint64_t> blocked_requests;
+
+   private:
+    friend class WebRequestAPI;

[Devlin, 2016/09/07 19:18:00]

Why does the WebRequestAPI need to be able to copy these?

[erikchen, 2016/09/07 22:07:43]

Removed.

+    DISALLOW_COPY_AND_ASSIGN(EventListener);
+  };
+
   friend struct base::DefaultSingletonTraits<ExtensionWebRequestEventRouter>;
 
-  struct EventListener;
-  using EventListeners = std::vector<const EventListener*>;
-  using ListenerMapForBrowserContext =
-      std::map<std::string, std::set<EventListener>>;
+  using ListenerIdentifiers = std::vector<EventListener::Identifier>;
+  using Listeners = std::vector<std::unique_ptr<EventListener>>;
+  using ListenerMapForBrowserContext = std::map<std::string, Listeners>;
   using ListenerMap = std::map<void*, ListenerMapForBrowserContext>;
   using BlockedRequestMap = std::map<uint64_t, BlockedRequest>;
   // Map of request_id -> bit vector of EventTypes already signaled
@@ -337,24 +405,32 @@ class ExtensionWebRequestEventRouter
   ExtensionWebRequestEventRouter();
   ~ExtensionWebRequestEventRouter();
 
+  // Returns the EventListener with the given |id|, or nullptr. Must be called
+  // from the IO thread.
+  EventListener* FindEventListener(const EventListener::Identifier& id);
+
+  // Removes the listener for the given sub-event. Must be called from the IO
+  // thread.
+  void RemoveEventListener(const EventListener::Identifier& id);
+
   // Ensures that future callbacks for |request| are ignored so that it can be
   // destroyed safely.
   void ClearPendingCallbacks(const net::URLRequest* request);
 
   bool DispatchEvent(void* browser_context,
                      net::URLRequest* request,
-                     const std::vector<const EventListener*>& listeners,
+                     const ListenerIdentifiers& listener_ids,
                      std::unique_ptr<WebRequestEventDetails> event_details);
 
   void DispatchEventToListeners(
       void* browser_context,
-      std::unique_ptr<std::vector<EventListener>> listeners,
+      const ListenerIdentifiers& listener_ids,
       std::unique_ptr<WebRequestEventDetails> event_details);
 
   // Returns a list of event listeners that care about the given event, based
   // on their filter parameters. |extra_info_spec| will contain the combined
   // set of extra_info_spec flags that every matching listener asked for.
-  std::vector<const EventListener*> GetMatchingListeners(
+  ListenerIdentifiers GetMatchingListeners(
       void* browser_context,
       const extensions::InfoMap* extension_info_map,
       const std::string& event_name,
@@ -365,20 +441,19 @@ class ExtensionWebRequestEventRouter
   // browser_context of the event, the next time for the "cross" browser_context
   // (i.e. the incognito browser_context if the event is originally for the
   // normal browser_context, or vice versa).
-  void GetMatchingListenersImpl(
-      void* browser_context,
-      const net::URLRequest* request,
-      const extensions::InfoMap* extension_info_map,
-      bool crosses_incognito,
-      const std::string& event_name,
-      const GURL& url,
-      int render_process_host_id,
-      int routing_id,
-      content::ResourceType resource_type,
-      bool is_async_request,
-      bool is_request_from_extension,
-      int* extra_info_spec,
-      std::vector<const EventListener*>* matching_listeners);
+  void GetMatchingListenersImpl(void* browser_context,
+                                const net::URLRequest* request,
+                                const extensions::InfoMap* extension_info_map,
+                                bool crosses_incognito,
+                                const std::string& event_name,
+                                const GURL& url,
+                                int render_process_host_id,
+                                int routing_id,
+                                content::ResourceType resource_type,
+                                bool is_async_request,
+                                bool is_request_from_extension,
+                                int* extra_info_spec,
+                                ListenerIdentifiers* matching_listeners);
 
   // Decrements the count of event handlers blocking the given request. When the
   // count reaches 0, we stop blocking the request and proceed it using the
@@ -457,6 +532,10 @@ class ExtensionWebRequestEventRouter
   // Returns true if |request| was already signaled to some event handlers.
   bool WasSignaled(const net::URLRequest& request) const;
 
+  // Get the number of listeners - for testing only.
+  size_t GetListenerCountForTesting(void* browser_context,
+                                    const std::string& event_name);
+
   // A map for each browser_context that maps an event name to a set of
   // extensions that are listening to that event.
   ListenerMap listeners_;