"Top Document Isolation" mode

content/browser/browsing_instance.cc

Index: content/browser/browsing_instance.cc
diff --git a/content/browser/browsing_instance.cc b/content/browser/browsing_instance.cc
index 1d5f8953769f92efbc770b3ca88331235d4d8002..7b93401b9e455508eda695b6da933e6dfd0c07df 100644
--- a/content/browser/browsing_instance.cc
+++ b/content/browser/browsing_instance.cc
@@ -36,7 +36,6 @@ SiteInstance* BrowsingInstance::GetSiteInstanceForURL(const GURL& url) {
   if (i != site_instance_map_.end())
     return i->second;
 
-
   // No current SiteInstance for this site, so let's create one.
   SiteInstanceImpl* instance = new SiteInstanceImpl(this);
 
@@ -45,11 +44,29 @@ SiteInstance* BrowsingInstance::GetSiteInstanceForURL(const GURL& url) {
   return instance;
 }
 
+SiteInstance* BrowsingInstance::GetDefaultSubframeSiteInstance() {

[Charlie Reis, 2016/03/25 19:47:12]

Can we put a CHECK(IsTopDocumentIsolationEnabled()) here?  Or at least
AreCrossProcessFramesPossible()?  Just want to make sure we don't call it in
default Chrome.

[ncarter (slow), 2016/03/28 22:00:28]

I made this the stricter variant, CHECK(IsTopDocumentIsolationEnabled()). It's
still not 100% clear how SiteInstance coalescing will work in the extension(a,b)
case, and whether that will use the default subframe siteinstance or not, so
let's restrict it to --top-document-isolation for now, and relax this constraint
as we make progress.

[Charlie Reis, 2016/03/29 17:17:12]

Acknowledged.

+  if (!default_subframe_site_instance_) {
+    SiteInstanceImpl* instance = new SiteInstanceImpl(this);
+    instance->set_is_default_subframe_site_instance();
+    // TODO(nick): This is a hack for now.
+    instance->SetSite(GURL("http://web-subframes.invalid"));

[Charlie Reis, 2016/03/25 19:47:12]

Yeah, we can chat about what to put here long term (or how to avoid it).

[ncarter (slow), 2016/03/28 22:00:28]

If we rip this out now, the PopupAndRedirection test sees a pretty big behavior
change. I think that's a sign of us falling back to the SiteURL somewhere we
shouldn't be, but for now I think it's safest to use a sentinel value here.

Open to other suggestions here -- let me know if you have ideas.

+    default_subframe_site_instance_ = instance;
+  }
+
+  return default_subframe_site_instance_;
+}
+
 void BrowsingInstance::RegisterSiteInstance(SiteInstance* site_instance) {
   DCHECK(static_cast<SiteInstanceImpl*>(site_instance)
              ->browsing_instance_.get() ==
          this);
   DCHECK(static_cast<SiteInstanceImpl*>(site_instance)->HasSite());
+
+  // Don't register the default subframe SiteInstance, to prevent it from being
+  // returned by GetSiteInstanceForURL.
+  if (default_subframe_site_instance_ == site_instance)
+    return;
+
   std::string site = site_instance->GetSiteURL().possibly_invalid_spec();
 
   // Only register if we don't have a SiteInstance for this site already.
@@ -79,6 +96,8 @@ void BrowsingInstance::UnregisterSiteInstance(SiteInstance* site_instance) {
     // Matches, so erase it.
     site_instance_map_.erase(i);
   }
+  if (default_subframe_site_instance_ == site_instance)
+    default_subframe_site_instance_ = nullptr;
 }
 
 BrowsingInstance::~BrowsingInstance() {