Allow a main interstitial frame to act as an ExtensionMessagePort opener

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

 // Copyright (c) 2012 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 "chrome/browser/extensions/api/messaging/extension_message_port.h"
 
 #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"
 #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 {
 
 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:
   explicit FrameTracker(ExtensionMessagePort* port)
-      : pm_observer_(this), port_(port) {}
+      : pm_observer_(this), port_(port), interstitial_frame_(nullptr) {}
   ~FrameTracker() override {}
 
   void TrackExtensionProcessFrames() {
     pm_observer_.Add(ProcessManager::Get(port_->browser_context_));
   }
 
   void TrackTabFrames(content::WebContents* tab) {
     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)
       override {
     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 {
     if (!details.is_in_page)
       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,
       content::RenderFrameHost* render_frame_host) override {
     if (extension_id == port_->extension_id_)
       port_->UnregisterFrame(render_frame_host);
   }
 
   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);
 };
 
 ExtensionMessagePort::ExtensionMessagePort(
     base::WeakPtr<MessageService> message_service,
     int port_id,
     const std::string& extension_id,
     content::RenderProcessHost* extension_process)
     : weak_message_service_(message_service),
@@ skipping 20 matching lines @@
     bool include_child_frames)
     : weak_message_service_(message_service),
       port_id_(port_id),
       extension_id_(extension_id),
       browser_context_(rfh->GetProcess()->GetBrowserContext()),
       extension_process_(nullptr),
       did_create_port_(false),
       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,
                                  base::Unretained(this)));
   } else {
     RegisterFrame(rfh);
   }
 }
 
 ExtensionMessagePort::~ExtensionMessagePort() {}
@@ skipping 129 matching lines @@
     return;
   }
   for (content::RenderFrameHost* rfh : frames_) {
     IPC::Message* msg_copy = new IPC::Message(*msg.get());
     msg_copy->set_routing_id(rfh->GetRoutingID());
     rfh->Send(msg_copy);
   }
 }
 
 }  // namespace extensions