Allow a main interstitial frame to act as an ExtensionMessagePort opener

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 aba30480fc5a704aa6b5e8512e73c5d42844f1c4..32982e8e42c306764870493674f38c0e90a51b15 100644
--- a/chrome/browser/extensions/api/messaging/extension_message_port.cc
+++ b/chrome/browser/extensions/api/messaging/extension_message_port.cc
@@ -6,6 +6,7 @@
 
 #include "base/scoped_observer.h"
 #include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -27,7 +28,7 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver,
                                            public ProcessManagerObserver {
  public:
   explicit FrameTracker(ExtensionMessagePort* port)
-      : pm_observer_(this), port_(port) {}
+      : pm_observer_(this), port_(port), interstitial_frame_(nullptr) {}
   ~FrameTracker() override {}
 
   void TrackExtensionProcessFrames() {
@@ -38,6 +39,12 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver,
     Observe(tab);
   }
 
+  void TrackInterstitialFrame(content::WebContents* tab,
+                              content::RenderFrameHost* interstitial_frame) {
+    interstitial_frame_ = interstitial_frame;
+    Observe(tab);
+  }
+
  private:
   // content::WebContentsObserver overrides:
   void RenderFrameDeleted(content::RenderFrameHost* render_frame_host)
@@ -45,12 +52,6 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver,
     port_->UnregisterFrame(render_frame_host);
   }
 
-  // TODO(robwu): This should be superfluous with RenderFrameDeleted above, but
-  // we are not entirely sure.
-  void FrameDeleted(content::RenderFrameHost* render_frame_host) override {
-    port_->UnregisterFrame(render_frame_host);
-  }
-
   void DidNavigateAnyFrame(content::RenderFrameHost* render_frame_host,
                            const content::LoadCommittedDetails& details,
                            const content::FrameNavigateParams&) override {
@@ -58,6 +59,12 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver,
       port_->UnregisterFrame(render_frame_host);
   }
 
+  void DidDetachInterstitialPage() override {

[Charlie Reis, 2016/01/25 21:47:38]

Hmm.  I suppose this will work, since the interstitial will go away if its RFH
dies.  (It's a bit unfortunate that we don't have a delete-time equivalent to
WCO::RenderFrameForInterstitialPageCreated, but that's probably unnecessary to
add here.)

[robwu, 2016/01/25 22:54:41]

Acknowledged.

+    if (interstitial_frame_) {

[Charlie Reis, 2016/01/25 21:47:38]

nit: No braces for a one-line body.

[robwu, 2016/01/25 22:54:41]

Done.

+      port_->UnregisterFrame(interstitial_frame_);
+    }
+  }
+
   // extensions::ProcessManagerObserver overrides:
   void OnExtensionFrameUnregistered(
       const std::string& extension_id,
@@ -68,6 +75,7 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver,
 
   ScopedObserver<ProcessManager, ProcessManagerObserver> pm_observer_;
   ExtensionMessagePort* port_;  // Owns this FrameTracker.
+  content::RenderFrameHost* interstitial_frame_;

[Charlie Reis, 2016/01/25 21:47:38]

This needs a comment saying that it's only present for interstitial main frames,
and that it's (presumably?) mutually exclusive the frames tracked normal tabs
(wherever those are tracked).

[robwu, 2016/01/25 22:54:41]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(FrameTracker);
 };
@@ -108,7 +116,24 @@ ExtensionMessagePort::ExtensionMessagePort(
       background_host_ptr_(nullptr),
       frame_tracker_(new FrameTracker(this)) {
   content::WebContents* tab = content::WebContents::FromRenderFrameHost(rfh);
-  CHECK(tab);
+  content::InterstitialPage* interstitial = nullptr;
+  if (!tab) {
+    interstitial = content::InterstitialPage::FromRenderFrameHost(rfh);
+    // A RenderFrameHost must be hosted in a WebContents or InterstitialPage.
+    CHECK(interstitial);
+    tab = interstitial->web_contents();

[Charlie Reis, 2016/01/25 21:47:38]

This isn't obviously safe to me.  It depends on what you'll be doing with |tab|,
since in many respects it's not safe to treat the interstitial rfh like a frame
of the WebContents.  It's not in the FrameTree, and it won't get the same
notifications.

Is there a way to reason about why it's safe to use it below?  Can we add a
comment to that effect?

[robwu, 2016/01/25 22:54:41]

Done.

[Charlie Reis, 2016/01/26 01:14:30]

I don't see any justification for this, but I think I understand it now.  Please
add the comment mentioned below.

[robwu, 2016/01/26 11:29:32]

Done.

+    // This is not expected to happen. When it happens, investigate why it

[Charlie Reis, 2016/01/25 21:47:38]

Let's elaborate on what "This" means here.  Are you talking about the case that
the interstitial doesn't have a WebContents?

(nit: Add blank line above this block.)

[robwu, 2016/01/25 22:54:41]

Done.

+    // happens anyway and fix it or return early. This is a CHECK, not a DCHECK
+    // because we really need an observer subject to detect frame removal.
+    CHECK(tab);
+    if (!rfh->GetParent()) {
+      // Only the main frame of an interstitial is supported.

[Charlie Reis, 2016/01/25 21:47:38]

Let's give a reason why subframes aren't supported (e.g., the sanity checker
doesn't allow it).  I don't think there are any interstitials with subframes,
but it's possible someone could hit this case and wonder about the reason.

[robwu, 2016/01/25 22:54:41]

Done.

+      frame_tracker_->TrackInterstitialFrame(tab, rfh);
+      RegisterFrame(rfh);
+    }
+    return;
+  }
+
   frame_tracker_->TrackTabFrames(tab);
   if (include_child_frames) {
     tab->ForEachFrame(base::Bind(&ExtensionMessagePort::RegisterFrame,