Use FrameTreeNode ID as frameId in extension APIs

extensions/browser/extension_api_frame_id_map.cc

+// Copyright 2015 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/extension_api_frame_id_map.h"
+
+#include <list>
+#include <map>
+#include <tuple>
+
+#include "base/lazy_instance.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
+
+namespace extensions {
+
+namespace {
+
+struct RenderFrameIdKey;
+typedef std::list<ExtensionApiFrameIdMap::FrameIdCallback> FrameIdCallbackList;
+typedef std::map<RenderFrameIdKey, ExtensionApiFrameId> FrameIdMap;
+typedef std::map<RenderFrameIdKey, FrameIdCallbackList> CallbackMap;
+
+// Queued callbacks for use on the IO thread. This queue is only used when the
+// frameId needs to be fetched from the UI thread.
+base::LazyInstance<CallbackMap> g_callbacks_map = LAZY_INSTANCE_INITIALIZER;
+
+// This frameId map is only modified on the UI thread and used for two purposes:
+// - On the IO thread, it avoids unnecessary thread hops.
+// - On the UI thread and the IO thread, it ensures that the frameId remains
+//   constant, even after removing a frame.
+// Items are never removed from this map.
+base::LazyInstance<FrameIdMap> g_frame_id_map = LAZY_INSTANCE_INITIALIZER;

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

Is it ok that data in this map is never removed? Thinking of memory consumption
and collision of IDs due to the re-use of process IDs.

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

Collisions are not probably because those process IDs are not OS process IDs,
but RenderProcessHost IDs (32-bit unsigned integers).

I'm currently not sure when it is safe to remove items from the map. Nasko did
already mention that the map is unbounded, we can continue the discussion at
https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext....

+
+// A set of identifiers that uniquely identifies a RenderFrame.
+struct RenderFrameIdKey {
+  RenderFrameIdKey(int render_process_id, int frame_routing_id)
+      : render_process_id(render_process_id),
+        frame_routing_id(frame_routing_id) {}
+
+  // The process ID of the renderer that contains the RenderFrame.
+  int render_process_id;
+  // The routing ID of the RenderFrame.
+  int frame_routing_id;
+
+  bool operator<(const RenderFrameIdKey& other) const {
+    return std::tie(render_process_id, frame_routing_id) <
+           std::tie(other.render_process_id, other.frame_routing_id);
+  }
+};
+
+}  // namespace
+
+ExtensionApiFrameId::ExtensionApiFrameId()
+    : frame_id(kInvalidFrameId), parent_frame_id(kInvalidFrameId) {}
+
+int GetFrameIdFromFrame(content::RenderFrameHost* rfh) {
+  if (!rfh)
+    return ExtensionApiFrameId::kInvalidFrameId;
+  if (rfh->GetParent())
+    return rfh->GetFrameTreeNodeId();
+  return 0;  // Main frame.
+}
+
+void InitializeFromFrame(ExtensionApiFrameId& extension_api_frame_id,

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

https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

Parameter order and output parameter should be a pointer.

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

Done.

+                         content::RenderFrameHost* rfh) {
+  if (!rfh)
+    return;
+
+  extension_api_frame_id.frame_id = GetFrameIdFromFrame(rfh);
+  extension_api_frame_id.parent_frame_id =
+      GetFrameIdFromFrame(rfh->GetParent());
+}
+
+const ExtensionApiFrameId& LookupFrameIdOnUI(const RenderFrameIdKey& key) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  FrameIdMap& frame_id_map = g_frame_id_map.Get();
+  FrameIdMap::const_iterator frame_id_iter = frame_id_map.find(key);
+  if (frame_id_iter != frame_id_map.end())
+    return frame_id_iter->second;
+
+  ExtensionApiFrameId& extension_api_frame_id = frame_id_map[key];
+  InitializeFromFrame(
+      extension_api_frame_id,
+      content::RenderFrameHost::FromID(key.render_process_id,
+                                       key.frame_routing_id));
+  return extension_api_frame_id;
+}
+
+void GotFrameId(const RenderFrameIdKey& key) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  FrameIdMap& frame_id_map = g_frame_id_map.Get();

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

In LookupFrameIdOnUI you are accessing this on the UI thread. Isn't that a
recipe for disaster?

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

This is the sequence of actions:

GetFrameIdOnIO:
- If list of callbacks is non-empty, queue callback and return.
- If frame_id is found, run callback with frame_id and return.
- Otherwise queue callback and PostTask to UI thread.

LookupFrameIdOnUI:
- Store frame_id in map

GotFrameId (on IO, posted back after LookupFrameIdOnUI returned):
- Run every callback with the frame_id
- Remove callbacks.

This seems safe, but it is not because I did not account for the fact that
LookupFrameIdOnUI can be called on the UI thread (I added this later). Would a
lock for accessing the map a good solution, or can you think of something
better?

+  FrameIdMap::const_iterator frame_id_iter = frame_id_map.find(key);
+  CHECK(frame_id_iter != frame_id_map.end());
+
+  const ExtensionApiFrameId& extension_api_frame_id = frame_id_iter->second;
+  CallbackMap& callbacks_map = g_callbacks_map.Get();
+  FrameIdCallbackList& callbacks = callbacks_map[key];
+  // If GetFrameIdOnIO is called in one of the callbacks, another callback is
+  // appended to the list. So we cannot use a fancy range-based for loop.
+  for (FrameIdCallbackList::iterator it = callbacks.begin();
+       it != callbacks.end(); ++it) {
+    it->Run(extension_api_frame_id);
+  }
+
+  // Remove the callback list. Because the extension frame ID is cached, this
+  // list is never created again for |key|.
+  callbacks_map.erase(key);
+}
+
+void ExtensionApiFrameIdMap::GetFrameIdOnIO(int render_process_id,
+                                            int frame_routing_id,
+                                            FrameIdCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  const RenderFrameIdKey key(render_process_id, frame_routing_id);
+  CallbackMap& callbacks_map = g_callbacks_map.Get();
+  CallbackMap::iterator callbacks_iter = callbacks_map.find(key);
+  if (callbacks_iter != callbacks_map.end()) {
+    // There is a pending lookup for the extension frame ID.
+    callbacks_iter->second.push_back(callback);
+    return;
+  }
+
+  FrameIdMap& frame_id_map = g_frame_id_map.Get();
+  FrameIdMap::const_iterator frame_id_iter = frame_id_map.find(key);
+  if (frame_id_iter != frame_id_map.end()) {
+    // The extension frame ID has already been looked up.
+    callback.Run(frame_id_iter->second);
+    return;
+  }
+
+  // The key was seen for the first time, hop to the UI thread to look up the
+  // extension frame ID.
+  callbacks_map[key].push_back(callback);
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(base::IgnoreResult(&LookupFrameIdOnUI), key),
+      base::Bind(&GotFrameId, key));
+}
+
+const ExtensionApiFrameId& ExtensionApiFrameIdMap::GetFrameId(
+    int render_process_id, int frame_routing_id) {
+  return LookupFrameIdOnUI(RenderFrameIdKey(render_process_id,
+                                            frame_routing_id));
+}
+
+const ExtensionApiFrameId& ExtensionApiFrameIdMap::GetFrameId(
+    content::RenderFrameHost* rfh) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  FrameIdMap& frame_id_map = g_frame_id_map.Get();
+  if (!rfh)
+    return frame_id_map[RenderFrameIdKey(-1, -1)];
+
+  const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
+
+  FrameIdMap::const_iterator frame_id_iter = frame_id_map.find(key);
+  if (frame_id_iter != frame_id_map.end())
+    return frame_id_iter->second;
+
+  ExtensionApiFrameId& extension_api_frame_id = frame_id_map[key];
+  InitializeFromFrame(extension_api_frame_id, rfh);
+  return extension_api_frame_id;
+}
+
+content::RenderFrameHost* ExtensionApiFrameIdMap::GetRenderFrameHostById(
+    content::WebContents* web_contents,
+    int frame_id) {
+  // Although it is technically possible to map |frame_id| to a RenderFrameHost
+  // without WebContents, we choose to not do that because in the extension API
+  // frameIds are only guaranteed to be meaningful in combination with a tabId.
+  if (!web_contents)
+    return nullptr;
+
+  if (frame_id == 0)
+    return web_contents->GetMainFrame();
+
+  if (frame_id == -1)
+    return nullptr;
+
+  DCHECK_GE(frame_id, 1);
+  return web_contents->FindFrameByFrameTreeNodeId(frame_id);
+}
+
+}  // namespace extensions