Hide requests in an extension from other extensions

extensions/browser/api/web_request/web_request_api.cc

 // 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.
 
 #include "extensions/browser/api/web_request/web_request_api.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 398 matching lines @@
   // <webview> events will be removed in RemoveWebViewEventListeners. Ideally,
   // this code should be decoupled from extensions, we should use the host ID
   // instead, and not have two different code paths. This is a huge undertaking
   // unfortunately, so we'll resort to two code paths for now.
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
                           base::Bind(&RemoveEventListenerOnIOThread,
                                      details.browser_context,
                                      details.extension_id,
                                      details.event_name,
-                                     0 /* embedder_process_id */,
+                                     0 /* embedder_process_id (ignored) */,
                                      0 /* web_view_instance_id */));
 }
 
 // Represents a single unique listener to an event, along with whatever filter
 // parameters and extra_info_spec were specified at the time the listener was
 // added.
 // NOTE(benjhayden) New APIs should not use this sub_event_name trick! It does
 // not play well with event pages. See downloads.onDeterminingFilename and
 // ExtensionDownloadsEventRouter for an alternative approach.
 struct ExtensionWebRequestEventRouter::EventListener {
   std::string extension_id;
   std::string extension_name;
   std::string sub_event_name;
   RequestFilter filter;
   int extra_info_spec;
   int embedder_process_id;
   int web_view_instance_id;
   base::WeakPtr<IPC::Sender> ipc_sender;
   mutable std::set<uint64> blocked_requests;
 
   // Comparator to work with std::set.
   bool operator<(const EventListener& that) const {
     if (extension_id != that.extension_id)
       return extension_id < that.extension_id;
 
     if (sub_event_name != that.sub_event_name)
       return sub_event_name < that.sub_event_name;
 
+    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;
 
-    if (web_view_instance_id != that.web_view_instance_id)
-      return web_view_instance_id < that.web_view_instance_id;
-
     return false;
   }
 
   EventListener() :
       extra_info_spec(0),
       embedder_process_id(0),
       web_view_instance_id(0) {}
 };
 
 // Contains info about requests that are blocked waiting for a response from
@@ skipping 767 matching lines @@
   return false;
 }
 
 void ExtensionWebRequestEventRouter::OnEventHandled(
     void* browser_context,
     const std::string& extension_id,
     const std::string& event_name,
     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;
 
   // The listener may have been removed (e.g. due to the process going away)
   // before we got here.
   std::set<EventListener>::iterator found =
       listeners_[browser_context][event_name].find(listener);
   if (found != listeners_[browser_context][event_name].end())
     found->blocked_requests.erase(request_id);
@@ skipping 212 matching lines @@
       // The IPC sender has been deleted. This listener will be removed soon
       // via a call to RemoveEventListener. For now, just skip it.
       continue;
     }
 
     if (is_web_view_guest &&
         (it->embedder_process_id != web_view_info.embedder_process_id ||
          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_ &&
         web_request_event_router_delegate_->OnGetMatchingListenersImplCheck(
             it->filter.tab_id, it->filter.window_id, request))
       continue;
     if (!it->filter.types.empty() &&
         std::find(it->filter.types.begin(), it->filter.types.end(),
                   resource_type) == it->filter.types.end())
       continue;
@@ skipping 700 matching lines @@
   std::string event_name;
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(3, &event_name));
 
   std::string sub_event_name;
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(4, &sub_event_name));
 
   int web_view_instance_id = 0;
   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());
   std::string extension_name =
       extension ? extension->name() : extension_id_safe();
 
   if (!web_view_instance_id) {
     // We check automatically whether the extension has the 'webRequest'
     // permission. For blocking calls we require the additional permission
     // 'webRequestBlocking'.
@@ skipping 232 matching lines @@
   // Continue gracefully.
   RunSync();
 }
 
 bool WebRequestHandlerBehaviorChangedFunction::RunSync() {
   helpers::ClearCacheOnNavigation();
   return true;
 }
 
 }  // namespace extensions