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 <vector>
 
 #include "base/bind.h"
@@ skipping 19 matching lines @@
 #include "extensions/browser/api/declarative_webrequest/request_stage.h"
 #include "extensions/browser/api/declarative_webrequest/webrequest_constants.h"
 #include "extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h"
 #include "extensions/browser/api/extensions_api_client.h"
 #include "extensions/browser/api/web_request/upload_data_presenter.h"
 #include "extensions/browser/api/web_request/web_request_api_constants.h"
 #include "extensions/browser/api/web_request/web_request_api_helpers.h"
 #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"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/guest_view/guest_view_events.h"
 #include "extensions/browser/guest_view/web_view/web_view_constants.h"
 #include "extensions/browser/guest_view/web_view/web_view_renderer_state.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/browser/io_thread_extension_message_filter.h"
 #include "extensions/browser/runtime_data.h"
@@ skipping 71 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 50 matching lines @@
                     WebViewRendererState::WebViewInfo* web_view_info) {
   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_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();
+  *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();
 
   // 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;
 }
 
+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) {
   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);
+  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);
 
   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
@@ skipping 146 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;
+      }

[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)
       return embedder_process_id < that.embedder_process_id;
 
     return false;
   }
 
   EventListener()
       : histogram_value(events::UNKNOWN),
@@ skipping 195 matching lines @@
   if (rules_registry.get())
     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_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_) {
     web_request_event_router_delegate_->ExtractExtraRequestDetails(
         request, out);
   }
 }
 
 int ExtensionWebRequestEventRouter::OnBeforeRequest(
     void* browser_context,
@@ skipping 458 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);
+  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;
     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(

[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,
     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 133 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_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;
 }
 
 void ExtensionWebRequestEventRouter::NotifyPageLoad() {
   for (const auto& callback : callbacks_for_page_load_)
     callback.Run();
   callbacks_for_page_load_.clear();
 }
 
@@ skipping 119 matching lines @@
     void* browser_context,
     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_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);
 
   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();
 
   EventListeners matching_listeners;
@@ skipping 871 matching lines @@
   // Continue gracefully.
   RunSync();
 }
 
 bool WebRequestHandlerBehaviorChangedFunction::RunSync() {
   helpers::ClearCacheOnNavigation();
   return true;
 }
 
 }  // namespace extensions