Use FrameTreeNode ID as frameId in extension APIs

chrome/browser/extensions/api/messaging/extension_message_port.cc

Index: chrome/browser/extensions/api/messaging/extension_message_port.cc
diff --git a/chrome/browser/extensions/api/messaging/extension_message_port.cc b/chrome/browser/extensions/api/messaging/extension_message_port.cc
index 1e4f87f901d6234975f9d3a6dccfcf139172fd3b..caaca145a5aa8da2e4fd30f95c5d6ce8753a6a02 100644
--- a/chrome/browser/extensions/api/messaging/extension_message_port.cc
+++ b/chrome/browser/extensions/api/messaging/extension_message_port.cc
@@ -5,36 +5,136 @@
 #include "chrome/browser/extensions/api/messaging/extension_message_port.h"
 
 #include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/navigation_details.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/process_manager.h"
+#include "extensions/browser/process_manager_observer.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/manifest_handlers/background_info.h"
 
 namespace extensions {
 
-ExtensionMessagePort::ExtensionMessagePort(content::RenderProcessHost* process,
-                                           int routing_id,
-                                           const std::string& extension_id)
-     : process_(process),
-       routing_id_(routing_id),
-       extension_id_(extension_id),
-       background_host_ptr_(NULL) {
+const char kReceivingEndDoesntExistError[] =
+    "Could not establish connection. Receiving end does not exist.";
+
+// Helper class to detect when frames are destroyed.
+class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver,
+                                           public ProcessManagerObserver {
+ public:
+  // Watch all frames in an extension process.
+  explicit FrameTracker(ExtensionMessagePort* port) : port_(port) {
+    ProcessManager::Get(port_->browser_context_)->AddObserver(this);
+  }
+  // Watch all frames in a tab.
+  // TODO(robwu): Check whether this includes background pages and <webview>s.
+  FrameTracker(ExtensionMessagePort* port, content::WebContents* tab)
+      : content::WebContentsObserver(tab), port_(port) {
+  }
+  ~FrameTracker() override {
+    ProcessManager::Get(port_->browser_context_)->RemoveObserver(this);
+  }

[Devlin, 2015/10/30 01:49:39]

nit: newline

[robwu, 2015/11/02 19:08:34]

Done.

+ private:
+  // content::WebContentsObserver overrides:
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host)

[Devlin, 2015/10/30 01:49:39]

I would have thought FrameDeleted here - are you sure RenderFrameDeleted is
correct?

[robwu, 2015/10/31 00:10:45]

I initially used FrameDeleted (as you can see in patch set 2), but that is not
triggered when the tab is killed. I've added a new test
(ExtensionApiTest.MessagingCrash) to catch this use case.
This is also a stepping stone to a fix for https://crbug.com/439780 (the test is
included in this CL because it shows the usefulness of using RenderFrameDeleted
over FrameDeleted, and some stub behavior will be replaced with desired behavior
when I fix this bug).

[Devlin, 2015/10/31 00:19:56]

Interesting.  I defer to Charlie's judgment for what's best here.  IIRC,
sometimes RenderFrameDeleted misses cases that FrameDeleted would have caught.

[Charlie Reis, 2015/11/03 00:23:49]

RenderFrameDeleted is for cases where you care whether the RenderFrame in the
renderer process has gone away.  It should include all cases in which
FrameDeleted is called (such as closing a tab or detaching a frame), as well as
after cross-process navigations or process crashes.

(If there's cases that FrameDeleted is called and RenderFrameDeleted is not,
that's possibly a bug that we should fix.)

If you're interested in when RenderFrameHosts go away (and not process crashes),
then FrameDeleted and RenderFrameHostChanged are a better fit.  Since it sounds
like you care about process crashes, I'm guessing RenderFrameDeleted is what you
want.

+      override {
+    port_->UnregisterFrame(render_frame_host);
+  }
+  void DidNavigateAnyFrame(content::RenderFrameHost* render_frame_host,
+                           const content::LoadCommittedDetails& details,
+                           const content::FrameNavigateParams&) override {
+    if (!details.is_in_page)
+      port_->UnregisterFrame(render_frame_host);
+  }
+
+  // extensions::ProcessManagerObserver overrides:
+  void OnExtensionFrameUnregistered(
+      const std::string& extension_id,
+      content::RenderFrameHost* render_frame_host) override {
+    if (extension_id == port_->extension_id_)
+    {

[Devlin, 2015/10/30 01:49:39]

bracket

[robwu, 2015/11/02 19:08:33]

Done.

+      DVLOG(2) << "OnExtensionFrameUnregistered " << port_->port_id_ << " for "

[Devlin, 2015/10/30 01:49:39]

We don't usually like to commit logs (even [D]VLOGS).  A few are maybe okay, but
there are many strewn throughout this CL.  Let's trim them a bit.

[robwu, 2015/10/31 00:10:45]

Oops, I did not intend to commit this one. I usually do not commit logging, but
I added them now because they helped with debugging. Which DVLOGs do you want to
be removed (all of them?)?

[Devlin, 2015/10/31 00:19:56]

Probably all of them, but if there's one or two you found to be invaluable and
will likely need again, it's okay to leave them.

[robwu, 2015/11/02 19:08:34]

Done.

+          << "frame " << render_frame_host << " " << " in extension "
+          << extension_id;
+      port_->UnregisterFrame(render_frame_host);
+    }
+  }
+
+  ExtensionMessagePort* port_;
+  DISALLOW_COPY_AND_ASSIGN(FrameTracker);
+};
+
+ExtensionMessagePort::ExtensionMessagePort(
+    base::WeakPtr<MessageService> message_service,
+    int port_id,
+    const std::string& extension_id,
+    content::RenderProcessHost* extension_process)
+    : weak_message_service_(message_service),
+      port_id_(port_id),
+      extension_id_(extension_id),
+      did_create_port_(false),
+      browser_context_(extension_process->GetBrowserContext()),
+      frames_(ProcessManager::Get(browser_context_)->
+              GetRenderFrameHostsForExtension(extension_id)),
+      extension_process_(extension_process),
+      background_host_ptr_(nullptr),
+      frame_tracker_(new FrameTracker(this)) {
+  DVLOG(2) << "Created ExtensionMessagePort " << port_id_ << " for extension "
+           << extension_id_ << " with " << frames_.size() << " frame(s).";
+}
+
+bool ExtensionMessagePort::IsValidPort() {
+  return !frames_.empty();
+}
+
+ExtensionMessagePort::ExtensionMessagePort(
+    base::WeakPtr<MessageService> message_service,
+    int port_id,
+    const std::string& extension_id,
+    content::RenderFrameHost* rfh,
+    bool include_child_frames)
+    : weak_message_service_(message_service),
+      port_id_(port_id),
+      extension_id_(extension_id),
+      did_create_port_(false),
+      browser_context_(rfh->GetProcess()->GetBrowserContext()),
+      frames_(),
+      extension_process_(nullptr),
+      background_host_ptr_(nullptr),
+      frame_tracker_() {
+  content::WebContents* tab = content::WebContents::FromRenderFrameHost(rfh);
+  DCHECK(tab);
+  frame_tracker_.reset(new FrameTracker(this, tab));
+  if (include_child_frames) {
+    tab->ForEachFrame(base::Bind(&ExtensionMessagePort::RegisterFrame,
+                                 base::Unretained(this)));
+  } else {
+    RegisterFrame(rfh);
+  }
+  DVLOG(2) << "Created ExtensionMessagePort " << port_id_ << " for "
+      << frames_.size() << " frame(s).";
+}
+
+ExtensionMessagePort::~ExtensionMessagePort() {
+  frame_tracker_.reset();
+  DVLOG(2) << "Destroyed ExtensionMessagePort " << port_id_;
 }
 
 void ExtensionMessagePort::DispatchOnConnect(
-    int dest_port_id,
     const std::string& channel_name,
     scoped_ptr<base::DictionaryValue> source_tab,
     int source_frame_id,
-    int target_tab_id,
-    int target_frame_id,
     int guest_process_id,
     int guest_render_frame_routing_id,
     const std::string& source_extension_id,
     const std::string& target_extension_id,
     const GURL& source_url,
     const std::string& tls_channel_id) {
+  DVLOG(2) << "ExtensionMessagePort::DispatchOnConnect " << port_id_;
+
   ExtensionMsg_TabConnectionInfo source;
   if (source_tab)
     source.tab.Swap(source_tab.get());
@@ -44,32 +144,30 @@ void ExtensionMessagePort::DispatchOnConnect(
   info.target_id = target_extension_id;
   info.source_id = source_extension_id;
   info.source_url = source_url;
-  info.target_tab_id = target_tab_id;
-  info.target_frame_id = target_frame_id;
   info.guest_process_id = guest_process_id;
   info.guest_render_frame_routing_id = guest_render_frame_routing_id;
 
-  process_->Send(new ExtensionMsg_DispatchOnConnect(
-      routing_id_, dest_port_id, channel_name, source, info, tls_channel_id));
+  SendToPort(new ExtensionMsg_DispatchOnConnect(
+      MSG_ROUTING_NONE, port_id_, channel_name, source, info, tls_channel_id));
 }
 
 void ExtensionMessagePort::DispatchOnDisconnect(
-    int source_port_id,
     const std::string& error_message) {
-  process_->Send(new ExtensionMsg_DispatchOnDisconnect(
-      routing_id_, source_port_id, error_message));
+  DVLOG(2) << "ExtensionMessagePort::DispatchOnDisconnect " << port_id_;
+
+  SendToPort(new ExtensionMsg_DispatchOnDisconnect(
+      MSG_ROUTING_NONE, port_id_, error_message));
 }
 
-void ExtensionMessagePort::DispatchOnMessage(const Message& message,
-                                             int target_port_id) {
-  process_->Send(new ExtensionMsg_DeliverMessage(
-      routing_id_, target_port_id, message));
+void ExtensionMessagePort::DispatchOnMessage(const Message& message) {
+  DVLOG(2) << "ExtensionMessagePort::DispatchOnMessage " << port_id_;
+
+  SendToPort(new ExtensionMsg_DeliverMessage(
+      MSG_ROUTING_NONE, port_id_, message));
 }
 
 void ExtensionMessagePort::IncrementLazyKeepaliveCount() {
-  Profile* profile =
-      Profile::FromBrowserContext(process_->GetBrowserContext());
-  extensions::ProcessManager* pm = ProcessManager::Get(profile);
+  ProcessManager* pm = ProcessManager::Get(browser_context_);
   ExtensionHost* host = pm->GetBackgroundHostForExtension(extension_id_);
   if (host && BackgroundInfo::HasLazyBackgroundPage(host->extension()))
     pm->IncrementLazyKeepaliveCount(host->extension());
@@ -80,16 +178,77 @@ void ExtensionMessagePort::IncrementLazyKeepaliveCount() {
 }
 
 void ExtensionMessagePort::DecrementLazyKeepaliveCount() {
-  Profile* profile =
-      Profile::FromBrowserContext(process_->GetBrowserContext());
-  extensions::ProcessManager* pm = ProcessManager::Get(profile);
+  ProcessManager* pm = ProcessManager::Get(browser_context_);
   ExtensionHost* host = pm->GetBackgroundHostForExtension(extension_id_);
   if (host && host == background_host_ptr_)
     pm->DecrementLazyKeepaliveCount(host->extension());
 }
 
-content::RenderProcessHost* ExtensionMessagePort::GetRenderProcessHost() {
-  return process_;
+void ExtensionMessagePort::OpenPort(int process_id, int routing_id) {
+  DCHECK(routing_id != MSG_ROUTING_NONE || extension_process_);
+  DVLOG_IF(2, !extension_process_) << "ExtensionMessagePort::OpenPort "
+      << port_id_ << " from frame "
+      << content::RenderFrameHost::FromID(process_id, routing_id);
+  DVLOG_IF(2, extension_process_) << "ExtensionMessagePort::OpenPort "
+      << port_id_ << " from extension " << extension_id_;
+
+  // Mark port as active, so we can distinguish abnormal port closure from
+  // explicit port closure via disconnect().
+  did_create_port_ = true;
+}
+
+void ExtensionMessagePort::ClosePort(int process_id, int routing_id) {
+  if (routing_id == MSG_ROUTING_NONE) {
+    // The only non-frame-specific message is the response to an unhandled
+    // onConnect event in the extension process.
+    DCHECK(extension_process_);
+    DVLOG(2) << "ExtensionMessagePort::ClosePort " << port_id_ << " from "
+             << "extension " << extension_id_;
+
+    frames_.clear();
+    CloseChannel();
+    return;
+  }
+
+  content::RenderFrameHost* rfh =
+      content::RenderFrameHost::FromID(process_id, routing_id);
+  DVLOG(2) << "ExtensionMessagePort::ClosePort " << port_id_ << " for frame "
+           << rfh;
+  if (rfh)
+    UnregisterFrame(rfh);
+}
+
+void ExtensionMessagePort::CloseChannel() {
+  DVLOG(2) << "ExtensionMessagePort::CloseChannel " << port_id_;
+
+  std::string error_message = did_create_port_ ? std::string() :
+      kReceivingEndDoesntExistError;
+  if (weak_message_service_)
+    weak_message_service_->CloseChannel(port_id_, error_message);
+}
+
+void ExtensionMessagePort::RegisterFrame(content::RenderFrameHost* rfh) {
+  frames_.insert(rfh);
+}
+
+void ExtensionMessagePort::UnregisterFrame(content::RenderFrameHost* rfh) {
+  if (frames_.erase(rfh) != 0 && frames_.empty())
+    CloseChannel();
+}
+
+void ExtensionMessagePort::SendToPort(IPC::Message* msg) {
+  DCHECK_GT(frames_.size(), 0UL);
+  if (extension_process_) {
+    msg->set_routing_id(MSG_ROUTING_CONTROL);
+    extension_process_->Send(msg);
+    return;
+  }
+  for (content::RenderFrameHost* rfh : frames_) {
+    IPC::Message* msg_copy = new IPC::Message(*msg);
+    msg_copy->set_routing_id(rfh->GetRoutingID());
+    rfh->Send(msg_copy);
+  }
+  delete msg;
 }
 
 }  // namespace extensions