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 "base/synchronization/lock.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 {
+
+// 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);
+  }
+};
+
+// This is a std::list so that iterators are not invalidated when the list is
+// modified during an iteration.
+using FrameIdCallbackList = std::list<ExtensionApiFrameIdMap::FrameIdCallback>;
+using FrameIdMap = std::map<RenderFrameIdKey, ExtensionApiFrameId>;
+using CallbackMap = std::map<RenderFrameIdKey, FrameIdCallbackList>;
+
+// 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;

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

When we have 3 different members, I think it would make sense to just have them
be private fields on ExtensionApiFrameIdMap and have a single lazy instance to
that object.

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

Done (separately from the other changes, in patch set 16).

+
+// 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.

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

This actually makes me a little worried.  Frames are pretty common on the web,
so I wouldn't be surprised if this grew to be, well, large enough to matter
(tens of thousands) - especially since people don't restart Chrome anymore. ;)

I'll defer to Nasko or Charlie who will probably have a better insight into
whether or not this is a concern, but my gut tells me we'll probably want to
clean this up occasionally.

[Charlie Reis, 2015/12/14 20:45:00]

Yes, this seems like a nontrivial leak to me, since some pages can have a lot of
iframe churn.  And especially so if this is a record of all frames that have
ever existed in this session.

+base::LazyInstance<FrameIdMap> g_frame_id_map = LAZY_INSTANCE_INITIALIZER;
+
+// This lock protects |g_frame_id_map| from being concurrently written on the UI
+// thread and read on the IO thread.
+base::LazyInstance<base::Lock>::Leaky g_frame_id_map_lock =
+    LAZY_INSTANCE_INITIALIZER;
+
+int GetFrameIdFromFrame(content::RenderFrameHost* rfh) {
+  if (!rfh)
+    return ExtensionApiFrameId::kInvalidFrameId;
+  if (rfh->GetParent())
+    return rfh->GetFrameTreeNodeId();
+  return 0;  // Main frame.
+}
+
+
+}  // namespace
+
+ExtensionApiFrameId::ExtensionApiFrameId()
+    : frame_id(kInvalidFrameId), parent_frame_id(kInvalidFrameId) {}
+
+ExtensionApiFrameId::ExtensionApiFrameId(content::RenderFrameHost* rfh)
+    : frame_id(GetFrameIdFromFrame(rfh)),
+      parent_frame_id(GetFrameIdFromFrame(rfh ? rfh->GetParent() : nullptr)) {
+}
+
+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;
+
+  content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
+      key.render_process_id, key.frame_routing_id);
+  auto kvpair = FrameIdMap::value_type(key, ExtensionApiFrameId(rfh));
+  base::AutoLock lock(g_frame_id_map_lock.Get());
+  return frame_id_map.insert(kvpair).first->second;
+}
+
+void GotFrameId(const RenderFrameIdKey& key) {

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

nit: let's add "OnIO"

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

Done.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  // |g_frame_id_map_lock| is not needed here because GotFrameId is only called

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

Wow, talk about relying on C++'s underlying guarantees.

I'm pretty sure a map *is* one of those objects that allows concurrent access
and insertion, but this still seems super subtle to me.  Let's see what happens,
but I'm not going to be entirely surprised if we find some bizarre exception as
a result of this.

And, looking at this more, would it be safe to just use
PostTaskAndReplyWithResult() in GetFrameIdOnIO, so that we don't have to do a
lookup at all?

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

It is indeed safe because the pointers are valid (due to their storage in the
global map), and I tried this at first, but it didn't compile.
Did you ever manage to have a function with return value "const T&" and bind it
using base::Bind?

An alternative is just changing the const ExtensionApiFrameId& signature to
ExtensionApiFrameId, and making copies. The struct consists of two 32-bit
integers, so it isn't that expensive.

WDYT?

[Devlin, 2015/12/14 21:31:55]

Hmm, yeah, passing a const& to a thread hop seems like it could be bad.  Just
copy the object.  It's cleaner, and copying is almost certainly cheaper than the
duplicate lookup we need this way.

+  // after returning from LookupFrameIdOnUI(). Items from the map are never
+  // removed, so it is safe to read values without lock.
+  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.
+  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);

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

In conjunction with the comment on line 108 and the logic on line 128, doesn't
this mean the callback that could potentially be added will never be called?

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

Both methods can only be called on the same thread, so I don't think that it's
possible to loose a callback.

(unless you tell me that there is an interrupt-like thing in Chromium that
suspends the execution at line 114, runs GetFrameIdOnIO, and then resumes the
execution at line 114.).

[Devlin, 2015/12/14 21:31:55]

Yeah, you're right, this should be safe.

+}
+
+void ExtensionApiFrameIdMap::GetFrameIdOnIO(int render_process_id,
+                                            int frame_routing_id,
+                                            const 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;
+  bool is_frame_id_found;

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

nit: initialize.

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

Done.

+  {
+    base::AutoLock lock(g_frame_id_map_lock.Get());
+    frame_id_iter = frame_id_map.find(key);
+    is_frame_id_found = frame_id_iter != frame_id_map.end();
+  }
+  if (is_frame_id_found) {
+    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(

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

Should this also use LookupFrameIdOnUI?

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

Yes. At first I split up the methods to save a map lookup (RFH->ID->RFH), but
the duplication of logic is not nice, so I'm now using LookupFrameOnUI.

+    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;
+
+  return frame_id_map.insert(
+      FrameIdMap::value_type(key, ExtensionApiFrameId(rfh))).first->second;
+}
+
+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