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 33616986882c26ad23ae7afcf88338c3875cb35b..87ff4ad4eeefff215dabc79a584e1a81e4e9baa7 100644
--- a/extensions/browser/api/web_request/web_request_api.cc
+++ b/extensions/browser/api/web_request/web_request_api.cc
@@ -127,10 +127,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,8 +198,6 @@ 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_process_host_id,
                                int* routing_id,
                                ResourceType* resource_type) {
@@ -213,8 +207,6 @@ void ExtractRequestInfoDetails(const net::URLRequest* request,
 
   *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();
 
@@ -225,6 +217,40 @@ void ExtractRequestInfoDetails(const net::URLRequest* request,
     *resource_type = content::RESOURCE_TYPE_LAST_TYPE;
 }
 
+// Enriches |dict| with information that can only be determined on the UI
+// thread, such as frameId. |dict| must be populated by ExtractRequestInfo
+// before calling this method.
+void ExtractRequestInfoOnUI(base::DictionaryValue* dict) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(dict);
+
+  // Update frameId / parentFrameId.
+  // -1 if unknown
+  //  0 if top-level frame
+  // >0 otherwise.
+  int render_process_id = -1;
+  int render_frame_id = -1;

[nasko, 2015/11/06 23:28:27]

What type of id is this? The FTN or the routing id? Let's be very precise.

+  int frame_tree_node_id = -1;
+  int parent_frame_tree_node_id = -1;
+  if (dict->GetInteger(keys::kFrameIdKey, &render_frame_id) &&
+      dict->GetInteger(keys::kProcessIdKey, &render_process_id)) {
+    content::RenderFrameHost* rfh =
+        content::RenderFrameHost::FromID(render_process_id, render_frame_id);

[nasko, 2015/11/06 23:28:27]

If this comes from caller of the extension API, wouldn't this be a FTN id? If
that's the case, this is the incorrect method to use for look up.

[robwu, 2015/11/07 00:23:40]

Heh. This ID is correct. I'm abusing kFrameIdKey as a temporary transport for
frameId from the caller to this method.

Before: frameId = RFH routing ID.
After : frameId = RFH FTN ID.

+    if (rfh) {
+      if (content::RenderFrameHost* parent = rfh->GetParent()) {

[nasko, 2015/11/06 23:28:27]

Assignment should not be done inside an if clause.

+        frame_tree_node_id = rfh->GetFrameTreeNodeID();
+        parent_frame_tree_node_id =
+            parent->GetParent() ? parent->GetFrameTreeNodeID() : 0;
+      } else {
+        frame_tree_node_id = 0;
+      }
+    }
+  }
+  dict->SetInteger(keys::kFrameIdKey, frame_tree_node_id);
+  dict->SetInteger(keys::kParentFrameIdKey, parent_frame_tree_node_id);
+  dict->Remove(keys::kProcessIdKey, nullptr);

[nasko, 2015/11/06 23:28:27]

Why are we removing this? I feel strongly about keeping it, as it gives
extension developers good idea of how frames are distributed across processes. I
even use it in an extension of mine to visualize that we do the right thing in
the SiteInstance logic.

[robwu, 2015/11/07 00:23:40]

This is the webRequest API. processId was never included in its public extension
API, and I'm only using the dict to propagate the frameId from
ExtractRequestInfo to this method.

+}
+
 // Extracts the body from |request| and writes the data into |out|.
 void ExtractRequestInfoBody(const net::URLRequest* request,
                             base::DictionaryValue* out) {
@@ -357,6 +383,7 @@ void SendOnMessageEventOnUI(
     return;
 
   scoped_ptr<base::ListValue> event_args(new base::ListValue);
+  ExtractRequestInfoOnUI(event_argument.get());
   event_args->Append(event_argument.release());
 
   EventRouter* event_router = EventRouter::Get(browser_context);
@@ -523,8 +550,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;
+      }
     }
 
     if (embedder_process_id != that.embedder_process_id)
@@ -741,27 +769,21 @@ void ExtensionWebRequestEventRouter::ExtractRequestInfo(
     base::DictionaryValue* out) {
   bool is_main_frame = false;
   int frame_id = -1;

[nasko, 2015/11/06 23:28:27]

I think this is the frame routing id, correct? If yes, let's name it explicitly
so it is clear which id we are using here.

[robwu, 2015/12/07 23:44:22]

Done.

-  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_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);
-  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 used by ExtractRequestInfoOnUI to
+  // find a RenderFrameHost, which provides the final frameId.
+  out->SetInteger(keys::kFrameIdKey, 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_) {
@@ -1254,21 +1276,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());
@@ -1286,6 +1299,18 @@ 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();
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&ExtractRequestInfoOnUI, base::Unretained(args_copy)),
+      base::Bind(&ExtensionWebRequestEventRouter::DispatchEventOnIO,
+                 base::Unretained(this), browser_context,
+                 base::Passed(&listeners_to_dispatch), base::Owned(args_copy)));
+
   if (num_handlers_blocking > 0) {
     BlockedRequest& blocked_request = blocked_requests_[request->identifier()];
     blocked_request.request = request;
@@ -1298,6 +1323,54 @@ bool ExtensionWebRequestEventRouter::DispatchEvent(
   return false;
 }
 
+void ExtensionWebRequestEventRouter::DispatchEventOnIO(
+    void* browser_context,
+    scoped_ptr<std::vector<EventListener>> listeners,
+    base::DictionaryValue* dict) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(listeners.get());
+  DCHECK_GT(listeners->size(), 0UL);
+  DCHECK(dict);
+
+  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,
@@ -1452,14 +1525,11 @@ 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_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);
 
@@ -1602,15 +1672,12 @@ ExtensionWebRequestEventRouter::GetMatchingListeners(
 
   bool is_main_frame = false;
   int frame_id = -1;
-  bool parent_is_main_frame = false;
-  int parent_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);