Use FrameTreeNode ID as frameId in extension APIs

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 109 matching lines @@
       return keys::kOnResponseStarted;
     case ExtensionWebRequestEventRouter::kOnErrorOccurred:
       return keys::kOnErrorOccurred;
     case ExtensionWebRequestEventRouter::kOnCompleted:
       return keys::kOnCompleted;
   }
   NOTREACHED();
   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,
                        webview::kWebViewEventPrefix,
                        base::CompareCase::SENSITIVE)) {
     web_request_event_name.replace(
         0, strlen(webview::kWebViewEventPrefix), kWebRequestEventPrefix);
   }
   const auto web_request_events_end =
       kWebRequestEvents + arraysize(kWebRequestEvents);
@@ skipping 51 matching lines @@
   int render_process_host_id = -1;
   int routing_id = -1;
   ExtractRequestRoutingInfo(request, &render_process_host_id, &routing_id);
   return WebViewRendererState::GetInstance()->GetInfo(
       render_process_host_id, routing_id, web_view_info);
 }
 
 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) {
   const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
   if (!info)
     return;
 
   *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();
 
   // Restrict the resource type to the values we care about.
   if (helpers::IsRelevantResourceType(info->GetResourceType()))
     *resource_type = info->GetResourceType();
   else
     *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.
+scoped_ptr<base::DictionaryValue> ExtractRequestInfoOnUI(
+    scoped_ptr<base::DictionaryValue> dict) {

[Devlin, 2015/10/30 01:49:39]

Seems like this should be a void method that takes in a DictionaryValue*?

[robwu, 2015/11/02 19:08:34]

Done.

+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(dict.get());

[Devlin, 2015/10/30 01:49:39]

nit: s/.get()//

[robwu, 2015/11/02 19:08:34]

Done.

+
+  // Update frameId / parentFrameId.
+  // -1 if unknown
+  //  0 if top-level frame
+  // >0 otherwise.
+  int render_process_id = -1;
+  int render_frame_id = -1;
+  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);
+    if (rfh) {
+      if (content::RenderFrameHost* parent = rfh->GetParent()) {
+        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);
+
+  return dict.Pass();
+}
+
 // Extracts the body from |request| and writes the data into |out|.
 void ExtractRequestInfoBody(const net::URLRequest* request,
                             base::DictionaryValue* out) {
   const net::UploadDataStream* upload_data = request->get_upload();
   if (!upload_data ||
       (request->method() != "POST" && request->method() != "PUT")) {
     return;  // Need to exit without "out->Set(keys::kRequestBodyKey, ...);" .
   }
 
   base::DictionaryValue* request_body = new base::DictionaryValue();
@@ skipping 112 matching lines @@
     const WebViewRendererState::WebViewInfo& web_view_info,
     scoped_ptr<base::DictionaryValue> event_argument) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   content::BrowserContext* browser_context =
       reinterpret_cast<content::BrowserContext*>(browser_context_id);
   if (!ExtensionsBrowserClient::Get()->IsValidContext(browser_context))
     return;
 
   scoped_ptr<base::ListValue> event_args(new base::ListValue);
-  event_args->Append(event_argument.release());
+  event_args->Append(ExtractRequestInfoOnUI(event_argument.Pass()).release());
 
   EventRouter* event_router = EventRouter::Get(browser_context);
 
   EventFilteringInfo event_filtering_info;
 
   events::HistogramValue histogram_value = events::UNKNOWN;
   std::string event_name;
   // The instance ID uniquely identifies a <webview> instance within an embedder
   // process. We use a filter here so that only event listeners for a particular
   // <webview> will fire.
@@ skipping 145 matching lines @@
       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) {
       // Do not filter by process ID for non-webviews, because this comparator
       // 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)
       return embedder_process_id < that.embedder_process_id;
 
     return false;
   }
 
   EventListener()
       : histogram_value(events::UNKNOWN),
@@ skipping 196 matching lines @@
     rules_registries_[key] = rules_registry;
   else
     rules_registries_.erase(key);
 }
 
 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_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 is not final. The (frameId, processId) pair is used to
+  // find a RFH in ExtractRequestInfoOnUI, which sets the final frameId.

[battre, 2015/11/02 13:54:57]

Can you expand the abbreviation?

[robwu, 2015/11/02 19:08:34]

Done.

+  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_) {
     web_request_event_router_delegate_->ExtractExtraRequestDetails(
         request, out);
   }
 }
 
 int ExtensionWebRequestEventRouter::OnBeforeRequest(
     void* browser_context,
@@ skipping 472 matching lines @@
 }
 
 bool ExtensionWebRequestEventRouter::DispatchEvent(
     void* browser_context,
     net::URLRequest* request,
     const std::vector<const EventListener*>& listeners,
     const base::ListValue& args) {
   // 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;
+
+  scoped_ptr<std::vector<EventListener>> listeners_to_dispatch(
+      new std::vector<EventListener>());
+  listeners_to_dispatch->reserve(listeners.size());
   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());
+    listeners_to_dispatch->push_back(*listener);
     if (listener->extra_info_spec &
         (ExtraInfoSpec::BLOCKING | ExtraInfoSpec::ASYNC_BLOCKING)) {
       listener->blocked_requests.insert(request->identifier());
       // If this is the first delegate blocking the request, go ahead and log
       // it.
       if (num_handlers_blocking == 0) {
         std::string delegate_info = l10n_util::GetStringFUTF8(
             IDS_LOAD_STATE_PARAMETER_EXTENSION,
             base::UTF8ToUTF16(listener->extension_name));
         // LobAndReport allows extensions that block requests to be displayed in
         // the load status bar.
         request->LogAndReportBlockedBy(delegate_info.c_str());
       }
       ++num_handlers_blocking;
     }
   }
 
+  // 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);
+  scoped_ptr<base::DictionaryValue> args_copy = dict->CreateDeepCopy();
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&ExtractRequestInfoOnUI, base::Passed(&args_copy)),
+      base::Bind(&ExtensionWebRequestEventRouter::DispatchEventOnIO,
+                 base::Unretained(this), browser_context,
+                 base::Passed(&listeners_to_dispatch)));

[battre, 2015/11/02 13:54:57]

I am a little bit concerned about this thread hopping. Matt implemented a lot of
logic for the webrequest API for the single purpose of not hopping between
threads. I wonder whether we need to investigate the performance implications
before launching this.

Do you see a way to work around this frame hopping?

[robwu, 2015/11/02 19:08:34]

Unfortunately not, the conclusion of the discussion at
https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/qlnf2fEAfB8...
is that we have to hop to the UI thread if we want to get a meaningful FTN ID.

I hope to revert the hopping in the webRequest API when every request passes the
UI thread, there is a field trial to investigate the performance impact of doing
so (https://crbug.com/524228).

Matt's work is not completely redundant, the blocking callbacks are directly
handled on the IO thread.

[Matt Perry, 2015/11/02 21:06:59]

Your last statement doesn't seem to be true. It's been a while since I looked at
this code, but it looks like we still go to the UI thread and back, even for
blocking event handlers. There was a measurable difference in roundtrip time
when I made the change to handle everything on the IO thread.

Note that these event handlers will block every single network request until the
extension responds, so performance is pretty important here.

[robwu, 2015/11/02 21:33:32]

By blocking callback, I mean the handling of the webRequest event listener's
return value. This doesn't hop to the UI thread. This patch introduces "only"
one IO->UI->IO hop before every event.
I'll optimize this code a bit by caching the frameId and using a queue to ensure
that the events are not delivered out of order.

+
   if (num_handlers_blocking > 0) {
     BlockedRequest& blocked_request = blocked_requests_[request->identifier()];
     blocked_request.request = request;
     blocked_request.is_incognito |= IsIncognitoBrowserContext(browser_context);
     blocked_request.num_handlers_blocking += num_handlers_blocking;
     blocked_request.blocking_time = base::Time::Now();
     return true;
   }
 
   return false;
 }
 
+void ExtensionWebRequestEventRouter::DispatchEventOnIO(
+    void* browser_context,
+    scoped_ptr<std::vector<EventListener>> listeners,
+    scoped_ptr<base::DictionaryValue> dict) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(listeners.get());
+  DCHECK_GT(listeners->size(), 0UL);
+  DCHECK(dict.get());
+
+  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,
     const std::string& event_name,
     const std::string& sub_event_name,
     uint64_t 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|.
   EventListener listener;
@@ skipping 134 matching lines @@
 
 void ExtensionWebRequestEventRouter::AddCallbackForPageLoad(
     const base::Closure& callback) {
   callbacks_for_page_load_.push_back(callback);
 }
 
 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);
 
   return resource_type == content::RESOURCE_TYPE_MAIN_FRAME;
 }
 
 void ExtensionWebRequestEventRouter::NotifyPageLoad() {
   for (const auto& callback : callbacks_for_page_load_)
     callback.Run();
   callbacks_for_page_load_.clear();
@@ skipping 122 matching lines @@
     const InfoMap* extension_info_map,
     const std::string& event_name,
     const net::URLRequest* request,
     int* extra_info_spec) {
   // TODO(mpcomplete): handle browser_context == NULL (should collect all
   // listeners).
   *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_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);
 
   bool is_request_from_extension =
       IsRequestFromExtension(request, extension_info_map);
 
   const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
   // We are conservative here and assume requests are asynchronous in case
   // we don't have an info object. We don't want to risk a deadlock.
   bool is_async_request = !info || info->IsAsync();
@@ skipping 873 matching lines @@
   // Continue gracefully.
   RunSync();
 }
 
 bool WebRequestHandlerBehaviorChangedFunction::RunSync() {
   helpers::ClearCacheOnNavigation();
   return true;
 }
 
 }  // namespace extensions