Fix semantics of ExtensionWebRequestEventRouter::EventListeners.

extensions/browser/api/web_request/web_request_api.h

 // Copyright (c) 2012 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.
 
 #ifndef EXTENSIONS_BROWSER_API_WEB_REQUEST_WEB_REQUEST_API_H_
 #define EXTENSIONS_BROWSER_API_WEB_REQUEST_WEB_REQUEST_API_H_
 
 #include <stdint.h>
 
 #include <list>
 #include <map>
 #include <set>
 #include <string>
 #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"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "content/public/common/resource_type.h"
 #include "extensions/browser/api/declarative/rules_registry.h"
 #include "extensions/browser/api/declarative_webrequest/request_stage.h"
 #include "extensions/browser/api/web_request/web_request_api_helpers.h"
 #include "extensions/browser/api/web_request/web_request_permissions.h"
@@ skipping 259 matching lines @@
                         const std::string& extension_name,
                         events::HistogramValue histogram_value,
                         const std::string& event_name,
                         const std::string& sub_event_name,
                         const RequestFilter& filter,
                         int extra_info_spec,
                         int embedder_process_id,
                         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,
       int embedder_process_id,
       int web_view_instance_id);
 
   // Called when an incognito browser_context is created or destroyed.
   void OnOTRBrowserContextCreated(void* original_browser_context,
                                   void* otr_browser_context);
   void OnOTRBrowserContextDestroyed(void* original_browser_context,
                                     void* otr_browser_context);
 
   // Registers a |callback| that is executed when the next page load happens.
   // The callback is then deleted.
   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
   using SignaledRequestMap = std::map<uint64_t, int>;
   // For each browser_context: a bool indicating whether it is an incognito
   // browser_context, and a pointer to the corresponding (non-)incognito
   // browser_context.
   using CrossBrowserContextMap = std::map<void*, std::pair<bool, void*>>;
   using CallbacksForPageLoad = std::list<base::Closure>;
 
   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,
       const net::URLRequest* request,
       int* extra_info_spec);
 
   // Helper for the above functions. This is called twice: once for the
   // 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
   // method requested by the extension with the highest precedence. Precedence
   // is decided by extension install time. If |response| is non-NULL, this
   // method assumes ownership.
   void DecrementBlockCount(void* browser_context,
                            const std::string& extension_id,
                            const std::string& event_name,
                            uint64_t request_id,
@@ skipping 58 matching lines @@
   void* GetCrossBrowserContext(void* browser_context) const;
 
   // Determines whether the specified browser_context is an incognito
   // browser_context (based on the contents of the cross-browser_context table
   // and without dereferencing the browser_context pointer).
   bool IsIncognitoBrowserContext(void* browser_context) const;
 
   // 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_;
 
   // A map of network requests that are waiting for at least one event handler
   // to respond.
   BlockedRequestMap blocked_requests_;
 
   // A map of request ids to a bitvector indicating which events have been
   // signaled and should not be sent again.
@@ skipping 83 matching lines @@
       extensions::QuotaLimitHeuristics* heuristics) const override;
   // Handle quota exceeded gracefully: Only warn the user but still execute the
   // function.
   void OnQuotaExceeded(const std::string& error) override;
   ResponseAction Run() override;
 };
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_BROWSER_API_WEB_REQUEST_WEB_REQUEST_API_H_