Hide requests in an extension from other extensions

extensions/browser/api/web_request/web_request_api.cc

Index: extensions/browser/api/web_request/web_request_api.cc
diff --git a/extensions/browser/api/web_request/web_request_api.cc b/extensions/browser/api/web_request/web_request_api.cc
index 801e58dfe539abb0035c42ec723b76cb05d80043..1b962c5ac89332aad2ab9675b4df088ae31896db 100644
--- a/extensions/browser/api/web_request/web_request_api.cc
+++ b/extensions/browser/api/web_request/web_request_api.cc
@@ -416,7 +416,7 @@ void WebRequestAPI::OnListenerRemoved(const EventListenerInfo& details) {
                                      details.browser_context,
                                      details.extension_id,
                                      details.event_name,
-                                     0 /* embedder_process_id */,
+                                     0 /* embedder_process_id (ignored) */,
                                      0 /* web_view_instance_id */));
 }
 
@@ -445,12 +445,20 @@ struct ExtensionWebRequestEventRouter::EventListener {
     if (sub_event_name != that.sub_event_name)
       return sub_event_name < that.sub_event_name;
 
-    if (embedder_process_id != that.embedder_process_id)
-      return embedder_process_id < that.embedder_process_id;
-
     if (web_view_instance_id != that.web_view_instance_id)
       return web_view_instance_id < that.web_view_instance_id;
 
+    if (web_view_instance_id == 0) {
+      // For non-webviews, ignore the process ID because the extension id and
+      // subevent name already identifies a listener. This allows for the use
+      // of find() without specifying the process ID of the extension.

[not at google - send to devlin, 2015/08/04 20:27:02]

Huh, funky. Where does this actually make a difference? I *would* rather we be
principled and set the process ID everywhere, unless there is a good reason not
to.

[robwu, 2015/08/04 21:57:24]

WebRequestAPI::OnListenerRemoved doesn't provide any information about the
reason for event removal. I don't want to rely on the process ID for removal, to
avoid getting bit by the following scenario:
1. Extension unloads.
2. OnListenerRemoved is called.
3. Meanwhile the extension process is gone.
4. The browser process tries to look up the process ID for the given extension
ID, but doesn't find any.

[not at google - send to devlin, 2015/08/04 23:12:02]

Makes sense that you're guarding against a bug here. The way the comment is
phrased makes it sound like an optimisation. Perhaps fix it up a bit.

[robwu, 2015/08/05 16:38:08]

Done.

+      DCHECK(embedder_process_id == 0 || that.embedder_process_id == 0);
+      return false;
+    }
+
+    if (embedder_process_id != that.embedder_process_id)
+      return embedder_process_id < that.embedder_process_id;
+
     return false;
   }
 
@@ -1238,6 +1246,8 @@ void ExtensionWebRequestEventRouter::OnEventHandled(
     const std::string& sub_event_name,
     uint64 request_id,
     EventResponse* response) {
+  // TODO(robwu): Does this also work with webviews? operator< (used by find)
+  // takes the webview ID into account, which is not set on |listener|.

[robwu, 2015/08/04 16:18:15]

Fady / Istiaque: This new TODO item is completely unrelated to the patch itself.
I added you to this ticket because I was wondering whether 1) this item is in
fact a real problem, and 2) if it is, whether you'll fix the issue.

In retrospect I should have created a new ticket to avoid confusion, but
hindsight is 20/20.

   EventListener listener;
   listener.extension_id = extension_id;
   listener.sub_event_name = sub_event_name;
@@ -1470,6 +1480,10 @@ void ExtensionWebRequestEventRouter::GetMatchingListenersImpl(
          it->web_view_instance_id != web_view_info.instance_id))
       continue;
 
+    if (is_request_from_extension &&

[not at google - send to devlin, 2015/08/04 20:27:02]

Comment this ("allow requests from the extension"), because it's subtle.

Also note that this won't work for iframes right now, but once site isolation is
in place, it will. I am fine with this behavior, but consider testing that.

[robwu, 2015/08/04 21:57:24]

Done.

+        it->embedder_process_id != render_process_host_id)
+      continue;
+
     if (!it->filter.urls.is_empty() && !it->filter.urls.MatchesURL(url))
       continue;
     if (web_request_event_router_delegate_ &&
@@ -2190,9 +2204,7 @@ bool WebRequestInternalAddEventListenerFunction::RunSync() {
   EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(5, &web_view_instance_id));
 
   base::WeakPtr<IOThreadExtensionMessageFilter> ipc_sender = ipc_sender_weak();
-  int embedder_process_id =
-      ipc_sender.get() && web_view_instance_id > 0 ?
-          ipc_sender->render_process_id() : 0;
+  int embedder_process_id = ipc_sender ? ipc_sender->render_process_id() : 0;
 
   const Extension* extension =
       extension_info_map()->extensions().GetByID(extension_id_safe());