Track frame openers in FrameTreeNodes instead of WebContents

chrome/browser/chrome_content_browser_client.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/chrome_content_browser_client.h"
 
 #include <set>
 #include <utility>
 #include <vector>
 
@@ skipping 476 matching lines @@
   return -1;
 }
 #endif  // defined(OS_POSIX) && !defined(OS_ANDROID) && !defined(OS_MACOSX)
 
 void SetApplicationLocaleOnIOThread(const std::string& locale) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   g_io_thread_application_locale.Get() = 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 =
       prerender::PrerenderContents::FromWebContents(tab);
   if (prerender_contents) {
     prerender_contents->Destroy(prerender::FINAL_STATUS_CREATE_NEW_WINDOW);
     return;
   }
 
   PopupBlockerTabHelper* popup_helper =
@@ skipping 22 matching lines @@
     }
   }
 
  private:
   const scoped_refptr<SafeBrowsingUIManager> safe_browsing_ui_manager_;
 };
 
 #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;
 
   SingleTabModeTabHelper::FromWebContents(web_contents)->HandleOpenUrl(params);
 }
 
 float GetDeviceScaleAdjustment() {
   static const float kMinFSM = 1.05f;
   static const int kWidthForMinFSM = 320;
   static const float kMaxFSM = 1.3f;
@@ skipping 1283 matching lines @@
     const GURL& source_origin,
     WindowContainerType container_type,
     const GURL& target_url,
     const content::Referrer& referrer,
     WindowOpenDisposition disposition,
     const WebWindowFeatures& features,
     bool user_gesture,
     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));
 
   *no_javascript_access = false;
 
   // If the opener is trying to create a background window but doesn't have
   // the appropriate permission, fail the attempt.
   if (container_type == WINDOW_CONTAINER_TYPE_BACKGROUND) {
 #if defined(ENABLE_EXTENSIONS)
     ProfileIOData* io_data = ProfileIOData::FromResourceContext(context);
@@ skipping 28 matching lines @@
 
   HostContentSettingsMap* content_settings =
       ProfileIOData::FromResourceContext(context)->GetHostContentSettingsMap();
   BlockedWindowParams blocked_params(target_url,
                                      referrer,
                                      disposition,
                                      features,
                                      user_gesture,
                                      opener_suppressed,
                                      render_process_id,
-                                     opener_id);
+                                     opener_render_frame_id);
 
   if (!user_gesture &&
       !base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisablePopupBlocking)) {
     if (content_settings->GetContentSetting(opener_top_level_frame_url,
                                             opener_top_level_frame_url,
                                             CONTENT_SETTINGS_TYPE_POPUPS,
                                             std::string()) !=
         CONTENT_SETTING_ALLOW) {
       BrowserThread::PostTask(BrowserThread::UI,
                               FROM_HERE,
                               base::Bind(&HandleBlockedPopupOnUIThread,
                                          blocked_params));
       return false;
     }
   }
 
 #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,
                                        blocked_params));
     return false;
   }
 #endif
 
   return true;
 }
@@ skipping 513 matching lines @@
       switches::kDisableWebRtcEncryption,
     };
     to_command_line->CopySwitchesFrom(from_command_line,
                                       kWebRtcDevSwitchNames,
                                       arraysize(kWebRtcDevSwitchNames));
   }
 }
 #endif  // defined(ENABLE_WEBRTC)
 
 }  // namespace chrome