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 <tuple>
+
+#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 {
+
+// The map is accessed on the IO and UI thread, so construct it once and never
+// delete it.

[nasko, 2016/01/05 00:39:58]

Should construction of the map also be protected by the lock as the constructor
is not atomic?

[robwu, 2016/01/05 10:59:03]

LazyInstance is documented to be thread-safe:
https://chromium.googlesource.com/chromium/src/+/9b6f42934e5a1e65ebfc668d91a2...
"The Type constructor will only ever be called once, even if two threads are
racing to create the object.  Get() and Pointer() will always return the same,
completely initialized instance."

+base::LazyInstance<ExtensionApiFrameIdMap>::Leaky g_map_instance =
+    LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+const int ExtensionApiFrameIdMap::kInvalidFrameId = -1;
+
+ExtensionApiFrameIdMap::CachedFrameIdPair::CachedFrameIdPair()
+    : frame_id(kInvalidFrameId), parent_frame_id(kInvalidFrameId) {}
+
+ExtensionApiFrameIdMap::CachedFrameIdPair::CachedFrameIdPair(
+    int frame_id,
+    int parent_frame_id)
+    : frame_id(frame_id), parent_frame_id(parent_frame_id) {}
+
+ExtensionApiFrameIdMap::RenderFrameIdKey::RenderFrameIdKey()
+    : render_process_id(-1), frame_routing_id(-1) {}

[nasko, 2016/01/05 00:39:58]

MSG_ROUTING_NONE for the routing id. 0 or kInvalidUniqueID for the process.

[robwu, 2016/01/05 10:59:03]

Done. The IDs have always been -1, but changing it to MSG_ROUTING_NONE (-2)
should not make a difference.

+
+ExtensionApiFrameIdMap::RenderFrameIdKey::RenderFrameIdKey(
+    int render_process_id,
+    int frame_routing_id)
+    : render_process_id(render_process_id),
+      frame_routing_id(frame_routing_id) {}
+
+ExtensionApiFrameIdMap::FrameIdCallbacks::FrameIdCallbacks()
+    : is_iterating(false) {}
+
+ExtensionApiFrameIdMap::FrameIdCallbacks::~FrameIdCallbacks() {}
+
+bool ExtensionApiFrameIdMap::RenderFrameIdKey::operator<(
+    const RenderFrameIdKey& other) const {
+  return std::tie(render_process_id, frame_routing_id) <
+         std::tie(other.render_process_id, other.frame_routing_id);
+}
+
+bool ExtensionApiFrameIdMap::RenderFrameIdKey::operator==(
+    const RenderFrameIdKey& other) const {
+  return render_process_id == other.render_process_id &&
+         frame_routing_id == other.frame_routing_id;
+}
+
+ExtensionApiFrameIdMap::ExtensionApiFrameIdMap() {}
+
+ExtensionApiFrameIdMap::~ExtensionApiFrameIdMap() {}
+
+ExtensionApiFrameIdMap* ExtensionApiFrameIdMap::Get() {
+  return g_map_instance.Pointer();
+}
+
+int ExtensionApiFrameIdMap::GetFrameId(content::RenderFrameHost* rfh) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (!rfh)
+    return kInvalidFrameId;
+  if (rfh->GetParent())
+    return rfh->GetFrameTreeNodeId();
+  return 0;  // Main frame.
+}
+
+int ExtensionApiFrameIdMap::GetParentFrameId(content::RenderFrameHost* rfh) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  return rfh ? GetFrameId(rfh->GetParent()) : kInvalidFrameId;
+}
+
+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 == -1)

[nasko, 2016/01/05 00:39:58]

kInvalidFrameId?

[robwu, 2016/01/05 10:59:03]

Done.

+    return nullptr;
+
+  if (frame_id == 0)
+    return web_contents->GetMainFrame();
+
+  DCHECK_GE(frame_id, 1);
+  return web_contents->FindFrameByFrameTreeNodeId(frame_id);
+}
+
+ExtensionApiFrameIdMap::CachedFrameIdPair ExtensionApiFrameIdMap::KeyToValue(
+    const RenderFrameIdKey& key) const {
+  content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
+      key.render_process_id, key.frame_routing_id);
+  return CachedFrameIdPair(GetFrameId(rfh), GetParentFrameId(rfh));
+}
+
+ExtensionApiFrameIdMap::CachedFrameIdPair
+ExtensionApiFrameIdMap::LookupFrameIdOnUI(const RenderFrameIdKey& key) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  FrameIdMap::const_iterator frame_id_iter = frame_id_map_.find(key);
+  if (frame_id_iter != frame_id_map_.end())
+    return frame_id_iter->second;
+
+  CachedFrameIdPair cached_frame_id_pair = KeyToValue(key);
+  // Don't save invalid values in the map.
+  if (cached_frame_id_pair.frame_id == kInvalidFrameId)
+    return cached_frame_id_pair;
+
+  auto kvpair = FrameIdMap::value_type(key, cached_frame_id_pair);
+  base::AutoLock lock(frame_id_map_lock_);
+  return frame_id_map_.insert(kvpair).first->second;
+}
+
+void ExtensionApiFrameIdMap::GotFrameIdOnIO(

[nasko, 2016/01/05 00:39:58]

I thought I put a comment earlier on (but can't find it), Get/Got are very
similarly named. Can we change the Got to something a bit more different? For
example "Received"?

[robwu, 2016/01/05 10:59:03]

Done.

+    const RenderFrameIdKey& key,
+    const CachedFrameIdPair& cached_frame_id_pair) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  FrameIdCallbacksMap::iterator map_iter = callbacks_map_.find(key);
+  if (map_iter == callbacks_map_.end()) {
+    // Can happen if GotFrameIdOnIO was called after the frame ID was resolved
+    // (e.g. via GetFrameIdOnIO), but before PostTaskAndReply replied.
+    return;
+  }
+
+  FrameIdCallbacks& callbacks = map_iter->second;
+
+  if (callbacks.is_iterating)
+    return;
+  callbacks.is_iterating = true;
+
+  // Note: Extra items can be appended to |callbacks| during this loop if a
+  // callback calls GetFrameIdOnIO().
+  for (std::list<FrameIdCallback>::iterator it = callbacks.callbacks.begin();
+       it != callbacks.callbacks.end(); ++it) {
+    it->Run(cached_frame_id_pair.frame_id,
+            cached_frame_id_pair.parent_frame_id);
+  }
+  callbacks_map_.erase(key);
+}
+
+void ExtensionApiFrameIdMap::GetFrameIdOnIO(int render_process_id,
+                                            int frame_routing_id,
+                                            const FrameIdCallback& callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  if (frame_routing_id <= -1) {
+    // frame_routing_id == -2 = MSG_ROUTING_NONE -> not a RenderFrameHost.
+    // frame_routing_id == -1 -> unknown RenderFrameHost

[nasko, 2016/01/05 00:39:58]

Let's avoid using -1 if possible. MSG_ROUTING_NONE should be used in cases where
we don't know of a RenderFrameHost.

[robwu, 2016/01/05 10:59:03]

Updated comment. I cannot add DCHECK_EQ(frame_routing_id, MSG_ROUTING_NONE)
because there are callers that pass -1 for unknown frames, even in content/.
E.g.
https://chromium.googlesource.com/chromium/src/+/8d34e46c9a294b4d9a5f74e97d9e...

[nasko, 2016/01/06 17:19:38]

Thanks for the TODO.

+    callback.Run(kInvalidFrameId, kInvalidFrameId);
+    return;
+  }
+  // A valid routing ID is only meaningful with a valid process ID.
+  DCHECK_GE(render_process_id, 0);
+
+  const RenderFrameIdKey key(render_process_id, frame_routing_id);
+  CachedFrameIdPair cached_frame_id_pair;
+  bool did_find_cached_frame_id_pair = false;
+
+  {
+    base::AutoLock lock(frame_id_map_lock_);
+    FrameIdMap::const_iterator frame_id_iter = frame_id_map_.find(key);
+    if (frame_id_iter != frame_id_map_.end()) {
+      // This is very likely to happen because CacheFrameId() is called as soon
+      // as the frame is created.
+      cached_frame_id_pair = frame_id_iter->second;
+      did_find_cached_frame_id_pair = true;
+    }
+  }
+
+  FrameIdCallbacksMap::iterator map_iter = callbacks_map_.find(key);
+
+  if (did_find_cached_frame_id_pair) {
+    // Value already cached, thread hopping is not needed.
+    if (map_iter == callbacks_map_.end()) {
+      // If the frame ID was cached, then it is likely that there are no pending
+      // callbacks. So do not unnecessarily copy the callback, but run it.
+      callback.Run(cached_frame_id_pair.frame_id,
+                   cached_frame_id_pair.parent_frame_id);
+    } else {
+      map_iter->second.callbacks.push_back(callback);
+      GotFrameIdOnIO(key, cached_frame_id_pair);
+    }
+    return;
+  }
+
+  // The key was seen for the first time (or the frame has been removed).
+  // Hop to the UI thread to look up the extension API frame ID.
+  callbacks_map_[key].callbacks.push_back(callback);
+  content::BrowserThread::PostTaskAndReplyWithResult(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&ExtensionApiFrameIdMap::LookupFrameIdOnUI,
+                 base::Unretained(this), key),
+      base::Bind(&ExtensionApiFrameIdMap::GotFrameIdOnIO,
+                 base::Unretained(this), key));
+}
+
+void ExtensionApiFrameIdMap::CacheFrameId(content::RenderFrameHost* rfh) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
+  CacheFrameId(key);
+  DCHECK(frame_id_map_.find(key) != frame_id_map_.end());
+}
+
+void ExtensionApiFrameIdMap::CacheFrameId(const RenderFrameIdKey& key) {
+  LookupFrameIdOnUI(key);
+}
+
+void ExtensionApiFrameIdMap::RemoveFrameId(content::RenderFrameHost* rfh) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(rfh);
+
+  const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
+  RemoveFrameId(key);
+}
+
+void ExtensionApiFrameIdMap::RemoveFrameId(const RenderFrameIdKey& key) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  base::AutoLock lock(frame_id_map_lock_);
+  frame_id_map_.erase(key);
+}
+
+}  // namespace extensions