PlzNavigate: Enforce frame-src CSP on the browser.

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 <string>
@@ skipping 22 matching lines @@
 #include "content/browser/frame_host/render_frame_proxy_host.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/frame_messages.h"
 #include "content/common/frame_owner_properties.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/view_messages.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host_observer.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
 
@@ skipping 751 matching lines @@
                                               request.common_params().url);
 
     no_renderer_swap |=
         !request.may_transfer() && !can_renderer_initiate_transfer;
   } else {
     // Subframe navigations will use the current renderer, unless specifically
     // allowed to swap processes.
     no_renderer_swap |= !CanSubframeSwapProcess(
         request.common_params().url, request.source_site_instance(),
         request.dest_site_instance(), was_server_redirect);
+
+    // Even if the url should warrant a process swap, check if the newly created
+    // SiteInstance would use the same storage partition as its parent. If

[nasko, 2017/03/03 23:04:23]

nit: StoragePartition

[arthursonzogni, 2017/03/06 15:10:13]

Done.

+    // that's not the case, the subframe should not swap processes, as there is
+    // not support for having an OOPIF that does not share the storage partition
+    // of its parent.

[nasko, 2017/03/03 23:04:23]

That seems like a good property to enforce long term, but I'm a bit weary of
introducing it as part of this patch, as it will be hard to discover.

[arthursonzogni, 2017/03/06 15:10:13]

I moved it to:
https://codereview.chromium.org/2732883003

+    BrowserContext* browser_context =
+        frame_tree_node_->navigator()->GetController()->GetBrowserContext();
+    bool share_parent_storage_partition =
+        BrowserContext::GetStoragePartition(browser_context,
+                                            dest_site_instance.get()) ==
+        BrowserContext::GetStoragePartition(browser_context,
+                                            frame_tree_node_->parent()
+                                                ->current_frame_host()
+                                                ->GetSiteInstance());
+    no_renderer_swap |=
+        (!SiteIsolationPolicy::UseDedicatedProcessesForAllSites() &&

[nasko, 2017/03/03 23:04:23]

Why check for UseDedicatedProcessesForAllSites? If we want to uniformly enforce
that no frame in the same WebContents uses a different StoragePartition, then
UseDedicatedProcessesForAllSites shouldn't factor in that decision.

[arthursonzogni, 2017/03/06 15:10:13]

I checked with Camille. It was because what is written in
SiteDetailsBrowserTest.PlatformAppsNotIsolated about --isolate-extensions. This
comment is no more valid since Chrome no longer allow external web content
inside sandboxed page in Platform Apps V2.
I removed it in https://codereview.chromium.org/2732883003

Not using a different process when UseDedicatedProcessesForAllSites is true is a
little bit weird though.

+         !share_parent_storage_partition);
   }
 
   if (no_renderer_swap) {
     // GetFrameHostForNavigation will be called more than once during a
     // navigation (currently twice, on request and when it's about to commit in
     // the renderer). In the follow up calls an existing pending WebUI should
     // not be recreated if the URL didn't change. So instead of calling
     // CleanUpNavigation just discard the speculative RenderFrameHost if one
     // exists.
     if (speculative_render_frame_host_)
@@ skipping 1990 matching lines @@
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content