Track frame openers in FrameTreeNodes instead of WebContents

chrome/browser/chrome_content_browser_client.cc

Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index e07ea7aa1525c34b4ffab0b1a833072daa79498a..c14e5edf01620f6c696a0d0713b6a1ae72f5874d 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -494,15 +494,14 @@ void SetApplicationLocaleOnIOThread(const std::string& locale) {
 }
 
 void HandleBlockedPopupOnUIThread(const BlockedWindowParams& params) {
-  // TODO(jochen): This code path should use RenderFrameHosts. See
-  // http://crbug.com/431769 for details.
-  RenderViewHost* render_view_host =
-      RenderViewHost::FromID(params.render_process_id(), params.opener_id());
-  if (!render_view_host)
+  RenderFrameHost* render_frame_host = RenderFrameHost::FromID(
+      params.render_process_id(), params.opener_render_frame_id());
+  if (!render_frame_host)
     return;
-  WebContents* tab = WebContents::FromRenderViewHost(render_view_host);
+  WebContents* tab = WebContents::FromRenderFrameHost(render_frame_host);
   // The tab might already have navigated away.
-  if (!tab || tab->GetRenderViewHost() != render_view_host)
+  if (!tab ||
+      tab->GetRenderViewHost() != render_frame_host->GetRenderViewHost())

[alexmos, 2015/06/01 17:44:55]

I guess this prevents the blocked popup UI from being shown if the main page has
already navigated away?  I found https://codereview.chromium.org/707223004 for
context.  I was thinking what should happen if the popup is opened from a
subframe which has already navigated away (or got removed) at this point, even
though the top frame is the same.  The existing check would not pass and still
show the UI in that case, which seems fine to me, but I'm curious what you
think.

[Charlie Reis, 2015/06/03 20:01:36]

Interesting.  I don't think this new check works, though, since it compares the
RVH of the subframe to the RVH of the main frame.

Is the FromRenderFrameHost call enough?  If we've navigated away from the main
frame, then we won't find the WebContents via an old subframe anymore.  (I don't
think that returns results for RFHs that are pending deletion, right?)

[alexmos, 2015/06/05 22:34:30]

Ah, right, this check won't work for subframes -- IIUC, their RFHs would've been
deleted during main frame navigation and we wouldn't even find the
render_frame_host for the first check above.

But don't we still need this check for main frames?  Even if the RFH is on
pending deletion list, it seems that its delegate() will still point to the same
WebContents (delegates seem to be set at init time and never change).  I
modified the check to only apply to main frames (and changed it to use RFHs).

[Charlie Reis, 2015/06/09 04:38:38]

Hmm, you're right, but I wonder if we should change that.  It seems really
unfortunate to require all callers of WebContents::FromRenderFrameHost to check
whether it's still really in use or not.  That's especially true now that
Nasko's CL has landed and we don't have swapped out RFHs anymore (at least in
--site-per-process), giving us a chance to clean up uses of RFHs.

I wonder if we should do a preliminary CL to make FromRenderFrameHost somehow
check whether the frame is on the pending deletion list, and then not return its
delegate WebContents.  (I guess we could clear its delegate, but I think that's
likely to cause problems.)  Let's chat with Nasko about this in person.

     return;
 
   prerender::PrerenderContents* prerender_contents =
@@ -545,9 +544,8 @@ class SafeBrowsingSSLCertReporter : public SSLCertReporter {
 #if defined(OS_ANDROID)
 
 void HandleSingleTabModeBlockOnUIThread(const BlockedWindowParams& params) {
-  WebContents* web_contents =
-      tab_util::GetWebContentsByID(params.render_process_id(),
-                                   params.opener_id());
+  WebContents* web_contents = tab_util::GetWebContentsByFrameID(
+      params.render_process_id(), params.opener_render_frame_id());
   if (!web_contents)
     return;
 
@@ -1851,7 +1849,8 @@ bool ChromeContentBrowserClient::CanCreateWindow(
     bool opener_suppressed,
     content::ResourceContext* context,
     int render_process_id,
-    int opener_id,
+    int opener_render_view_id,

[alexmos, 2015/06/01 17:44:55]

I didn't just convert opener_id to be opener_render_frame_id because of a
dependency of SingleTabModeTabHelper on the RVH's ID below.  I can't refactor
SingleTabModeTabHelper without this CL, since that needs the RFH's ID, so I
thought I'd keep both IDs here, and remove opener_render_view_id once I refactor
SingleTabModeTabHelper in a follow-up CL (this avoids entangling this CL with
non-trivial Android changes).  But let me know if it's better to do it here.

[Charlie Reis, 2015/06/03 20:01:36]

Yes, that approach sounds good.

+    int opener_render_frame_id,
     bool* no_javascript_access) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
@@ -1900,7 +1899,7 @@ bool ChromeContentBrowserClient::CanCreateWindow(
                                      user_gesture,
                                      opener_suppressed,
                                      render_process_id,
-                                     opener_id);
+                                     opener_render_frame_id);
 
   if (!user_gesture &&
       !base::CommandLine::ForCurrentProcess()->HasSwitch(
@@ -1919,7 +1918,8 @@ bool ChromeContentBrowserClient::CanCreateWindow(
   }
 
 #if defined(OS_ANDROID)
-  if (SingleTabModeTabHelper::IsRegistered(render_process_id, opener_id)) {
+  if (SingleTabModeTabHelper::IsRegistered(render_process_id,
+                                           opener_render_view_id)) {
     BrowserThread::PostTask(BrowserThread::UI,
                             FROM_HERE,
                             base::Bind(&HandleSingleTabModeBlockOnUIThread,