Disable cross-process navigations in --single-process.

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 1057 matching lines @@
   static_cast<SiteInstanceImpl*>(site_instance)->AddObserver(this);
   return proxy_host;
 }
 
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* site_instance) {
   static_cast<SiteInstanceImpl*>(site_instance)->RemoveObserver(this);
   proxy_hosts_.erase(site_instance->GetId());
 }
 
 bool RenderFrameHostManager::ShouldTransitionCrossSite() {

[alexmos, 2017/06/30 20:40:38]

Just noticed that the comment for this in the header is a bit out of date
(doesn't include --single-process).

-  // The logic below is weaker than "are all sites isolated" -- it asks instead,
-  // "is any site isolated". That's appropriate here since we're just trying to
-  // figure out if we're in any kind of site isolated mode, and in which case,
-  // we ignore the kSingleProcess and kProcessPerTab settings.
-  //
-  // TODO(nick): Move all handling of kSingleProcess/kProcessPerTab into
-  // SiteIsolationPolicy so we have a consistent behavior around the interaction
-  // of the process model flags.
-  //
-  // TODO(creis, alexmos): This looks like it will break single-process and
-  // process-per-tab.  See https://crbug.com/688617.
-  if (SiteIsolationPolicy::AreCrossProcessFramesPossible())
-    return true;
-
-  // False in the single-process mode, as it makes RVHs to accumulate
-  // in swapped_out_hosts_.
-  // True if we are using process-per-site-instance (default) or
-  // process-per-site (kProcessPerSite).
-  // TODO(nick): Move handling of kSingleProcess and kProcessPerTab into
-  // SiteIsolationPolicy.
+  // False in single-process mode, which does not support cross-process
+  // navigations or OOPIFs.  Also false in --process-per-tab, which uses one
+  // process per BrowsingInstance.  ("Tab" is an over-simplifcation, since it's
+  // really a group of tabs, and it is still possible to swap BrowsingInstances
+  // within a tab.)

[alexmos, 2017/06/30 02:23:29]

Yes, I agree that it makes sense to prioritize --single-process over all other
modes, and this should fix the Android webview case if it's indeed the cause of
the perf regression there.

A few questions about --process-per-tab, which seems trickier: :)

1.  Won't we run into trouble with layout tests which still use it?  In
particular, I'm curious if we will now avoid OOPIFs in the tests that used to
have them (e.g., on our FYI bot with --site-per-process).  Also, it looks like
some devtools extensions layout tests are failing both with and without
--site-per-process, so I wonder if pfeldman's new oopif.test logic from
https://chromium-review.googlesource.com/c/533897/ would also be broken.  

We should probably reevaluate whether layout tests can run without
--process-per-tab now that we have swarming, but we probably want something in
the meantime - perhaps allowing ShouldTransitionCrossSite to return true for
layout tests via ShellContentBrowserClient, or just for --site-per-process?

2.  Just to verify, are we ok changing precedence so that --process-per-tab is
preferred over all our security modes (--isolate-extensions, --site-per-process,
etc.)?  Assuming people use process-per-tab for performance (to reduce overhead
from extra processes and process swaps), it makes sense to disable things like
TDI, but it might also make sense to prioritize some security modes over it,
kind of like we do in the force_swap cases.  I wonder if the Opera
process-per-tab use cases around embedded environments actually use extensions
-- because this effectively would unship isolate extensions for them.  Also,
overriding --site-per-process doesn't seem necessary for the Opera uses, as
we'll never ship it, and Opera can just not specify the flag. :)

3.  Isolated origins aren't really compatible with process-per-tab, and it's
probably fine to pick --process-per-tab if that's what the user wants.  One way
to expose that could be to log a warning and maybe an early return in
AddIsolatedOrigin if --process-per-tab is specified.  And/or, WDYT about adding
"process-per-tab" to kBadFlags, so users will see the warning that "stability
and security will suffer"?  (Which is true if we prefer it over
--isolate-extensions.)

I guess we're not too worried about not giving process isolation to sign-in on
Opera embedded devices, since we'll probably add the isolated origin from the
chrome layer anyway, so Opera won't add it if they build on top of content/ --
is that what they do?  Plus Opera won't have any of the sign-in stuff built into
Chrome.

4.  To help think about 2 and 3, I was curious how many people actually use
--process-per-tab in the wild -- do we have any UMAs on that? 

5. How does this interact with oopif-webview?  Does that whole infrastructure
still work with --process-per-tab?

[alexmos, 2017/06/30 20:40:38]

Note that Lukasz is removing --process-per-tab from layout tests in
https://codereview.chromium.org/2968753003/ :)

[Charlie Reis, 2017/06/30 23:45:15]

These are excellent questions.  :)

I'm getting the sense we should try to address --single-process and
--process-per-tab separately.

For --single-process, I think we may want to land something like this to unblock
Ken's CL, and then follow up on Nick's idea to see if we can make this
unnecessary for --single-process.  In other words, see if we can avoid the
memory overhead by ordering things such that the old document/RenderFrame is
unloaded before committing the new one, since they're all in the same process
and thread.  That might move us closer to not needing this function at all.

For --process-per-tab, we can get by a little longer now that layout tests don't
need it.  For Opera's use case, we should discuss these questions you've raised
on https://crbug.com/717459 and figure out the right API/behavior to expose.  It
may be that we eliminate --process-per-tab entirely and give embedders a way to
configure the behavior they want.

I'll see if I can update this CL to reflect that next week.  (Ken, feel free to
try out this CL locally in the meantime if you find a way to repro the
regression, to see if this will fix it.)

[Charlie Reis, 2017/07/10 16:18:38]

In more detail...

On 2017/06/30 23:45:15, Charlie Reis (slow) wrote:
Yes, those tests must somehow assume that we have OOPIFs and/or
--isolate-extensions?  When my CL was disabling that in --process-per-tab, it
caused those tests to fail.  Moot now that we don't use --process-per-tab for
layout tests.
Hooray!
It's probably not a good thing that my update to the --process-per-tab flag
turns off --isolate-extensions and isolated origins in Chrome.  I think that
mode should probably turn off "unnecessary" process swaps for general cross-site
navigations, but not ones that have been deemed necessary for security.

We'll need to handle that a bit differently, so let's leave --process-per-tab as
a no-op for now (as it has been on all platforms but Android for a while now)
and revisit to make it work properly after this CL lands.
I'm not aware of any UMAs for tracking uses of this flag.  We could probably
query crash reports to get a sense for how prevalent it is, though?
Good question.  With the new plan to make OOPIFs work within an updated
--process-per-tab mode, it shouldn't be a problem.

   return !base::CommandLine::ForCurrentProcess()->HasSwitch(
              switches::kSingleProcess) &&
          !base::CommandLine::ForCurrentProcess()->HasSwitch(
              switches::kProcessPerTab);
 }
 
 bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
     const GURL& current_effective_url,
     bool current_is_view_source_mode,
     SiteInstance* new_site_instance,
@@ skipping 1720 matching lines @@
                       ->is_hidden() != delegate_->IsHidden()) {
     if (delegate_->IsHidden()) {
       view->Hide();
     } else {
       view->Show();
     }
   }
 }
 
 }  // namespace content