Don't split off NTP subframes with TDI.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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 "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 1402 matching lines @@
   // Use the current SiteInstance for same site navigations.
   if (IsCurrentlySameSite(render_frame_host_.get(), dest_url))
     return SiteInstanceDescriptor(render_frame_host_->GetSiteInstance());
 
   if (SiteIsolationPolicy::IsTopDocumentIsolationEnabled()) {
     // TODO(nick): Looking at the main frame and openers is required for TDI
     // mode, but should be safe to enable unconditionally.
     if (!frame_tree_node_->IsMainFrame()) {
       RenderFrameHostImpl* main_frame =
           frame_tree_node_->frame_tree()->root()->current_frame_host();
       if (IsCurrentlySameSite(main_frame, dest_url))

[ncarter (slow), 2016/11/15 21:42:35]

I think you've done the right thing here, but double-check my reasoning:

The other way to do this, is to add the "|| (embedder_disabled_tdi)" to this
case. They're not equivalent though, since one takes
DoesSiteRequireDedicatedProcess() into account. Practically I think either would
work with the oopif modes that exist today, but the way you've implemented it
seems structurally safer, if for some reason we had an interaction between TDI
and a future isolation mode (e.g. running with something like
--isolate-sites-for-testing=chrome*://* and --top-document-isolation).

[Avi (use Gerrit), 2016/11/16 20:12:11]

Um, yes, this is exactly why I did it.

Really, though, I'm not sure why I put it down there instead of up here, but
that reasoning makes sense for leaving it there.

This whole function needs tests and cleanup, though.

         return SiteInstanceDescriptor(main_frame->GetSiteInstance());
     }
 
     if (frame_tree_node_->opener()) {
       RenderFrameHostImpl* opener_frame =
           frame_tree_node_->opener()->current_frame_host();
       if (IsCurrentlySameSite(opener_frame, dest_url))
         return SiteInstanceDescriptor(opener_frame->GetSiteInstance());
     }
   }
 
   if (!frame_tree_node_->IsMainFrame() &&
       SiteIsolationPolicy::IsTopDocumentIsolationEnabled() &&
       !SiteInstanceImpl::DoesSiteRequireDedicatedProcess(browser_context,
                                                          dest_url)) {
+    if (GetContentClient()
+            ->browser()
+            ->ShouldFrameShareParentSiteInstanceDespiteTopDocumentIsolation(
+                dest_url, current_instance)) {
+      return SiteInstanceDescriptor(render_frame_host_->GetSiteInstance());
+    }
+
     // This is a cross-site subframe of a non-isolated origin, so place this
     // frame in the default subframe site instance.
     return SiteInstanceDescriptor(
         browser_context, dest_url,
         SiteInstanceRelation::RELATED_DEFAULT_SUBFRAME);
   }
 
   // Start the new renderer in a new SiteInstance, but in the current
   // BrowsingInstance.
   return SiteInstanceDescriptor(browser_context, dest_url,
@@ skipping 1273 matching lines @@
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content