Use FrameTreeNode ID as frameId in extension APIs

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 e6c175cb1e354278a0de49c0c657657f49b8b62b..0db5f68b2aa82d324a76417a75db31ea416713f2 100644
--- a/extensions/browser/api/web_request/web_request_api.cc
+++ b/extensions/browser/api/web_request/web_request_api.cc
@@ -37,6 +37,7 @@
 #include "extensions/browser/api/web_request/web_request_event_router_delegate.h"
 #include "extensions/browser/api/web_request/web_request_time_tracker.h"
 #include "extensions/browser/event_router.h"
+#include "extensions/browser/extension_api_frame_id_map.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
@@ -128,10 +129,6 @@ const char* GetRequestStageAsString(
   return "Not reached";
 }
 
-int GetFrameId(bool is_main_frame, int frame_id) {
-  return is_main_frame ? 0 : frame_id;
-}
-
 bool IsWebRequestEvent(const std::string& event_name) {
   std::string web_request_event_name(event_name);
   if (base::StartsWith(web_request_event_name,
@@ -202,9 +199,7 @@ bool GetWebViewInfo(const net::URLRequest* request,
 
 void ExtractRequestInfoDetails(const net::URLRequest* request,
                                bool* is_main_frame,
-                               int* frame_id,
-                               bool* parent_is_main_frame,
-                               int* parent_frame_id,
+                               int* render_frame_id,
                                int* render_process_host_id,
                                int* routing_id,
                                ResourceType* resource_type) {
@@ -212,10 +207,8 @@ void ExtractRequestInfoDetails(const net::URLRequest* request,
   if (!info)
     return;
 
-  *frame_id = info->GetRenderFrameID();
+  *render_frame_id = info->GetRenderFrameID();
   *is_main_frame = info->IsMainFrame();
-  *parent_frame_id = info->GetParentRenderFrameID();
-  *parent_is_main_frame = info->ParentIsMainFrame();
   *render_process_host_id = info->GetChildID();
   *routing_id = info->GetRouteID();
 
@@ -226,6 +219,26 @@ void ExtractRequestInfoDetails(const net::URLRequest* request,
     *resource_type = content::RESOURCE_TYPE_LAST_TYPE;
 }
 
+void ExtractRenderFrameInfo(base::DictionaryValue* dict,
+                            int* render_process_id,
+                            int* render_frame_id) {
+  if (dict->GetInteger(keys::kFrameIdKey, render_frame_id) &&
+      dict->GetInteger(keys::kProcessIdKey, render_process_id)) {
+  } else {
+    *render_process_id = -1;
+    *render_frame_id = -1;
+  }
+  // ExtractFrameIdInfo will overwrite kFrameIdKey, so it is not removed here.

[battre, 2015/12/10 15:31:39]

nit: ExtractApiFrameFrameIdInfo?

[robwu, 2015/12/10 16:39:29]

Done (using 1x Frame though: ExtractApiFrameIdInfo)

+  dict->Remove(keys::kProcessIdKey, nullptr);

[battre, 2015/12/10 15:31:39]

It feels weird to me that the extract function has the side effect of modifying
dict.

[robwu, 2015/12/10 16:39:29]

Indeed. That's why I document that processId is modified. I'd like to submit
another CL that refactors this class (using a struct to hold the DictionaryValue
plus frame routing id / process routing id) instead of passing a ListValue
around that has only one DictionaryValue...

+}
+
+void ExtractApiFrameIdInfo(base::DictionaryValue* dict,
+                           const ExtensionApiFrameId& extension_api_frame_id) {

[battre, 2015/12/10 15:31:39]

nit: do you want to put the output parameter last?
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

[robwu, 2015/12/10 16:39:29]

Done.

+  dict->SetInteger(keys::kFrameIdKey, extension_api_frame_id.frame_id);
+  dict->SetInteger(keys::kParentFrameIdKey,
+                   extension_api_frame_id.parent_frame_id);
+}
+
 // Extracts the body from |request| and writes the data into |out|.
 void ExtractRequestInfoBody(const net::URLRequest* request,
                             base::DictionaryValue* out) {
@@ -358,6 +371,14 @@ void SendOnMessageEventOnUI(
     return;
 
   scoped_ptr<base::ListValue> event_args(new base::ListValue);
+  int render_process_host_id = -1;
+  int render_frame_id = -1;
+  ExtractRenderFrameInfo(event_argument.get(), &render_process_host_id,
+                         &render_frame_id);
+  const ExtensionApiFrameId& extension_api_frame_id =
+      ExtensionApiFrameIdMap::GetFrameId(render_process_host_id,
+                                         render_frame_id);
+  ExtractApiFrameIdInfo(event_argument.get(), extension_api_frame_id);
   event_args->Append(event_argument.release());
 
   EventRouter* event_router = EventRouter::Get(browser_context);
@@ -524,8 +545,9 @@ struct ExtensionWebRequestEventRouter::EventListener {
       // is also used to find and remove an event listener when an extension is
       // unloaded. At this point, the event listener cannot be mapped back to
       // the original process, so 0 is used instead of the actual process ID.
-      DCHECK(embedder_process_id == 0 || that.embedder_process_id == 0);
-      return false;
+      if (embedder_process_id == 0 || that.embedder_process_id == 0) {
+        return false;
+      }

[battre, 2015/12/10 15:31:39]

nit: no {}?

[robwu, 2015/12/10 16:39:29]

Done.

     }
 
     if (embedder_process_id != that.embedder_process_id)
@@ -741,28 +763,24 @@ void ExtensionWebRequestEventRouter::ExtractRequestInfo(
     const net::URLRequest* request,
     base::DictionaryValue* out) {
   bool is_main_frame = false;
-  int frame_id = -1;
-  bool parent_is_main_frame = false;
-  int parent_frame_id = -1;
-  int frame_id_for_extension = -1;
-  int parent_frame_id_for_extension = -1;
+  int render_frame_id = -1;
   int render_process_host_id = -1;
   int routing_id = -1;
   ResourceType resource_type = content::RESOURCE_TYPE_LAST_TYPE;
-  ExtractRequestInfoDetails(request, &is_main_frame, &frame_id,
-                            &parent_is_main_frame, &parent_frame_id,
+  ExtractRequestInfoDetails(request, &is_main_frame, &render_frame_id,
                             &render_process_host_id, &routing_id,
                             &resource_type);
-  frame_id_for_extension = GetFrameId(is_main_frame, frame_id);
-  parent_frame_id_for_extension = GetFrameId(parent_is_main_frame,
-                                             parent_frame_id);
 
   out->SetString(keys::kRequestIdKey,
                  base::Uint64ToString(request->identifier()));
   out->SetString(keys::kUrlKey, request->url().spec());
   out->SetString(keys::kMethodKey, request->method());
-  out->SetInteger(keys::kFrameIdKey, frame_id_for_extension);
-  out->SetInteger(keys::kParentFrameIdKey, parent_frame_id_for_extension);
+  // Note: This (frameId, processId) pair is removed by ExtractRenderFrameInfo,
+  // and then finally set by ExtractApiFrameIdInfo.
+  // TODO(robwu): This is ugly. Create a proper data structure to separate these
+  // two IDs from the dictionary, so that kFrameIdKey has only one meaning.
+  out->SetInteger(keys::kFrameIdKey, render_frame_id);
+  out->SetInteger(keys::kProcessIdKey, render_process_host_id);
   out->SetString(keys::kTypeKey, helpers::ResourceTypeToString(resource_type));
   out->SetDouble(keys::kTimeStampKey, base::Time::Now().ToDoubleT() * 1000);
   if (web_request_event_router_delegate_) {
@@ -1241,21 +1259,12 @@ bool ExtensionWebRequestEventRouter::DispatchEvent(
   // TODO(mpcomplete): Consider consolidating common (extension_id,json_args)
   // pairs into a single message sent to a list of sub_event_names.
   int num_handlers_blocking = 0;
-  for (const EventListener* listener : listeners) {
-    // Filter out the optional keys that this listener didn't request.
-    scoped_ptr<base::ListValue> args_filtered(args.DeepCopy());
-    base::DictionaryValue* dict = NULL;
-    CHECK(args_filtered->GetDictionary(0, &dict) && dict);
-    if (!(listener->extra_info_spec & ExtraInfoSpec::REQUEST_HEADERS))
-      dict->Remove(keys::kRequestHeadersKey, NULL);
-    if (!(listener->extra_info_spec & ExtraInfoSpec::RESPONSE_HEADERS))
-      dict->Remove(keys::kResponseHeadersKey, NULL);
 
-    EventRouter::DispatchEventToSender(
-        listener->ipc_sender.get(), browser_context, listener->extension_id,
-        listener->histogram_value, listener->sub_event_name,
-        args_filtered.Pass(), EventRouter::USER_GESTURE_UNKNOWN,
-        EventFilteringInfo());
+  scoped_ptr<std::vector<EventListener>> listeners_to_dispatch(
+      new std::vector<EventListener>());
+  listeners_to_dispatch->reserve(listeners.size());
+  for (const EventListener* listener : listeners) {
+    listeners_to_dispatch->push_back(*listener);
     if (listener->extra_info_spec &
         (ExtraInfoSpec::BLOCKING | ExtraInfoSpec::ASYNC_BLOCKING)) {
       listener->blocked_requests.insert(request->identifier());
@@ -1273,6 +1282,22 @@ bool ExtensionWebRequestEventRouter::DispatchEvent(
     }
   }
 
+  // TODO(robwu): Avoid unnecessary copy, by changing |args| to be a
+  // scoped_ptr<base::DictionaryValue> and transferring the ownership.
+  const base::DictionaryValue* dict = nullptr;
+  CHECK(args.GetDictionary(0, &dict) && dict);
+  base::DictionaryValue* args_copy = dict->DeepCopy();
+
+  int render_process_host_id = -1;
+  int render_frame_id = -1;
+  ExtractRenderFrameInfo(args_copy, &render_process_host_id, &render_frame_id);
+
+  ExtensionApiFrameIdMap::GetFrameIdOnIO(
+      render_process_host_id, render_frame_id,
+      base::Bind(&ExtensionWebRequestEventRouter::DispatchEventOnIO,
+                 AsWeakPtr(), browser_context,
+                 base::Passed(&listeners_to_dispatch), base::Owned(args_copy)));

[battre, 2015/12/10 15:31:39]

Just checking: Can this asynchronicity rearrange events in such a way that a
subresource is reported before its frame has been reported?

[robwu, 2015/12/10 16:39:29]

Good point. In theory, this is certainly possible because PostTask does not
guarantee the order in which the tasks are run.

My ExtensionApiFrameId class only guarantees that lookup requests for the same
ID as processed in order. If this theoretical possiblity turns out to be a real
issue, then I can send another update that synchronizes parent/child frame IDs,
e.g. queuing callbacks when the parent frame ID is not known, and replying with
the whole path to the root when a frame is looked up. This has some overhead, so
I'd like to wait and see whether it is a real problem.

+
   if (num_handlers_blocking > 0) {
     BlockedRequest& blocked_request = blocked_requests_[request->identifier()];
     blocked_request.request = request;
@@ -1285,6 +1310,57 @@ bool ExtensionWebRequestEventRouter::DispatchEvent(
   return false;
 }
 
+void ExtensionWebRequestEventRouter::DispatchEventOnIO(

[battre, 2015/12/10 15:31:39]

I think this naming is confusing because the other DispatchEvent is also
executed on the IO thread.

[robwu, 2015/12/10 16:39:29]

I've renamed it to DispatchEventToListeners.

+    void* browser_context,
+    scoped_ptr<std::vector<EventListener>> listeners,
+    base::DictionaryValue* dict,
+    const ExtensionApiFrameId& extension_api_frame_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(listeners.get());
+  DCHECK_GT(listeners->size(), 0UL);
+  DCHECK(dict);
+
+  ExtractApiFrameIdInfo(dict, extension_api_frame_id);
+
+  std::string event_name =
+      EventRouter::GetBaseEventName((*listeners)[0].sub_event_name);
+  DCHECK(IsWebRequestEvent(event_name));
+
+  const std::set<EventListener>& event_listeners =
+      listeners_[browser_context][event_name];
+  void* cross_browser_context = GetCrossBrowserContext(browser_context);
+  const std::set<EventListener>* cross_event_listeners =
+      cross_browser_context ? &listeners_[cross_browser_context][event_name]
+                            : nullptr;
+
+  for (const EventListener& target : *listeners) {
+    std::set<EventListener>::const_iterator listener =
+        event_listeners.find(target);
+    // Ignore listener if it was removed between the thread hops.
+    if (listener == event_listeners.end()) {
+      if (!cross_event_listeners)
+        continue;
+      listener = cross_event_listeners->find(target);
+      if (listener == cross_event_listeners->end())
+        continue;
+    }
+
+    // Filter out the optional keys that this listener didn't request.
+    scoped_ptr<base::ListValue> args_filtered(new base::ListValue);
+    args_filtered->Append(dict->DeepCopy());
+    if (!(listener->extra_info_spec & ExtraInfoSpec::REQUEST_HEADERS))
+      dict->Remove(keys::kRequestHeadersKey, nullptr);
+    if (!(listener->extra_info_spec & ExtraInfoSpec::RESPONSE_HEADERS))
+      dict->Remove(keys::kResponseHeadersKey, nullptr);
+
+    EventRouter::DispatchEventToSender(
+        listener->ipc_sender.get(), browser_context, listener->extension_id,
+        listener->histogram_value, listener->sub_event_name,
+        args_filtered.Pass(), EventRouter::USER_GESTURE_UNKNOWN,
+        EventFilteringInfo());
+  }
+}
+
 void ExtensionWebRequestEventRouter::OnEventHandled(
     void* browser_context,
     const std::string& extension_id,
@@ -1438,17 +1514,14 @@ void ExtensionWebRequestEventRouter::AddCallbackForPageLoad(
 bool ExtensionWebRequestEventRouter::IsPageLoad(
     const net::URLRequest* request) const {
   bool is_main_frame = false;
-  int frame_id = -1;
-  bool parent_is_main_frame = false;
-  int parent_frame_id = -1;
+  int render_frame_id = -1;
   int render_process_host_id = -1;
   int routing_id = -1;
   ResourceType resource_type = content::RESOURCE_TYPE_LAST_TYPE;
 
-  ExtractRequestInfoDetails(request, &is_main_frame, &frame_id,
-                            &parent_is_main_frame, &parent_frame_id,
-                            &render_process_host_id,
-                            &routing_id, &resource_type);
+  ExtractRequestInfoDetails(request, &is_main_frame, &render_frame_id,
+                            &render_process_host_id, &routing_id,
+                            &resource_type);
 
   return resource_type == content::RESOURCE_TYPE_MAIN_FRAME;
 }
@@ -1588,18 +1661,15 @@ ExtensionWebRequestEventRouter::GetMatchingListeners(
   *extra_info_spec = 0;
 
   bool is_main_frame = false;
-  int frame_id = -1;
-  bool parent_is_main_frame = false;
-  int parent_frame_id = -1;
+  int render_frame_id = -1;
   int render_process_host_id = -1;
   int routing_id = -1;
   ResourceType resource_type = content::RESOURCE_TYPE_LAST_TYPE;
   const GURL& url = request->url();
 
-  ExtractRequestInfoDetails(request, &is_main_frame, &frame_id,
-                            &parent_is_main_frame, &parent_frame_id,
-                            &render_process_host_id,
-                            &routing_id, &resource_type);
+  ExtractRequestInfoDetails(request, &is_main_frame, &render_frame_id,
+                            &render_process_host_id, &routing_id,
+                            &resource_type);
 
   bool is_request_from_extension =
       IsRequestFromExtension(request, extension_info_map);