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;

[Devlin, 2015/12/10 21:53:07]

Can we just defined RenderFrameIdKey here?

[robwu, 2015/12/11 00:53:48]

Done.

+typedef std::list<ExtensionApiFrameIdMap::FrameIdCallback> FrameIdCallbackList;

[Devlin, 2015/12/10 21:53:07]

prefer using

[robwu, 2015/12/11 00:53:48]

Done.

+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;
+
+// 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(content::RenderFrameHost* rfh,

[Devlin, 2015/12/10 21:53:07]

Seems like this could just be a constructor of ExtensionApiFrameId?

[robwu, 2015/12/11 00:53:48]

Done.

+                         ExtensionApiFrameId* extension_api_frame_id) {
+  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];

[Devlin, 2015/12/10 21:53:07]

and then this set would become something like:
// Insert and return a new entry for the |rfh|.
return frame_id_map.insert(ExtensionApiFrameId(rfh)).first->second;

[robwu, 2015/12/11 00:53:48]

Done.

+  InitializeFromFrame(content::RenderFrameHost::FromID(key.render_process_id,
+                                                       key.frame_routing_id),
+                      &extension_api_frame_id);
+  return extension_api_frame_id;
+}
+
+void GotFrameId(const RenderFrameIdKey& key) {

[Devlin, 2015/12/10 21:53:07]

put this in the anonymous namespace?

[robwu, 2015/12/11 00:53:48]

Done.

[Devlin, 2015/12/12 14:25:00]

Wasn't done?

[robwu, 2015/12/14 20:55:41]

Done in 15, but made redundant by making it a private member of the class.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  FrameIdMap& frame_id_map = g_frame_id_map.Get();
+  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.

[Devlin, 2015/12/10 21:53:07]

Heh.  Tricky.

Might be worth a comment on FrameIdCallbackList that it needs to be a std::list
so that iterators aren't invalidated in push_back().

[robwu, 2015/12/11 00:53:48]

Done.

+  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();

[Devlin, 2015/12/10 21:53:07]

Is this safe?  Map lookup operations aren't purely atomic, so if it's also being
modified on the UI thread, won't we have a big problem?

[robwu, 2015/12/11 00:53:48]

The initial version was safe, but after adding the ability to trigger a write
from the UI thread, it is no longer safe. I've added a lock for protection.

+  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(rfh, &extension_api_frame_id);
+  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