Don't create unexpected OOPIFs in --isolate-extensions.

Don't create unexpected OOPIFs in --isolate-extensions.

Session restore was leading to OOPIFs for cross-site iframes in modes
with more limited OOPIF-creation policies, because the SiteInstance
groupings are not restored.  Instead, use the same logic as the transfer
case to determine whether a process swap is needed.

BUG=567424
TEST=See bug for repro steps.

[Charlie Reis, 2015-12-08 17:57:23]

Patchset #2 (id:20001) has been deleted

[Charlie Reis, 2015-12-08 18:01:10]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-12-08 18:01:11]

Nasko, can you take a look?  The fix seemed to do well in try jobs in patch set
1, and I added a test for it in patch set 2.  The test won't fail yet because
there's no OOPIF mode in content/ other than --site-per-process.

Nick is adding a flag which will make the test meaningful.

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2691: //
TODO(creis): Turn on OOPIF framework without isolating all sites.
Nick is adding a command line flag to content to make this possible.

[nasko, 2015-12-08 18:23:35]

LGTM with a few nits.

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2685: class
NavigationControllerOOPIFBrowserTest
nit: Per the newest discussions on chromium-dev, abbreviations are supposed to
be CamelCased as well, so NavigationControllerOopifBrowserTest.

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2706:
NavigateToURL(shell(), main_url_a);
nit: EXPECT_TRUE

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2720:
FrameNavigateParamsCapturer capturer(root->child_at(0));
Why not use the usual loading stopped event? Or TestNavigationObserver?

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2777: //
The subframe should only be in a different process if OOPIFs are required
nit: "different SiteInstance" as process allocation is in a layer below
SiteInstances.

[Charlie Reis, 2015-12-08 19:04:57]

Thanks!  Nick's CL is here:
https://codereview.chromium.org/1507333002/

I'll prepare this one to land separately from that one, but I'll also spin up a
second backup CL that has both in case the infrastructure issues today make the
timeframe tight.

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2685: class
NavigationControllerOOPIFBrowserTest
On 2015/12/08 18:23:35, nasko wrote:
> nit: Per the newest discussions on chromium-dev, abbreviations are supposed to
> be CamelCased as well, so NavigationControllerOopifBrowserTest.

Done.

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2706:
NavigateToURL(shell(), main_url_a);
On 2015/12/08 18:23:34, nasko wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2720:
FrameNavigateParamsCapturer capturer(root->child_at(0));
On 2015/12/08 18:23:35, nasko wrote:
> Why not use the usual loading stopped event? Or TestNavigationObserver?

Done.

https://codereview.chromium.org/1506093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2777: //
The subframe should only be in a different process if OOPIFs are required
On 2015/12/08 18:23:35, nasko wrote:
> nit: "different SiteInstance" as process allocation is in a layer below
> SiteInstances.

Done.

[nasko, 2015-12-08 19:08:42]

One last nit.

https://codereview.chromium.org/1506093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1506093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2720:
TestNavigationObserver capturer(shell()->web_contents());
nit: It is no longer a capturer, but an observer.

[Charlie Reis, 2015-12-08 19:30:51]

Thanks!  I also added the switch so that the test is meaningful when Nick's
https://codereview.chromium.org/1507333002/ lands.

I also created a backup CL that combines the two in case we only have time to
land one CL today: https://codereview.chromium.org/1505343002/.

https://codereview.chromium.org/1506093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1506093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2720:
TestNavigationObserver capturer(shell()->web_contents());
On 2015/12/08 19:08:42, nasko wrote:
> nit: It is no longer a capturer, but an observer.

Done.

[Charlie Reis, 2015-12-08 22:11:49]

Description was changed from

==========
Don't create unexpected OOPIFs in --isolate-extensions.

Session restore was leading to OOPIFs for cross-site iframes in modes
with more limited OOPIF-creation policies, because the SiteInstance
groupings are not restored.  Instead, use the same logic as the transfer
case to determine whether a process swap is needed.

BUG=567424
TEST=See bug for repro steps.
==========

to

==========
Don't create unexpected OOPIFs in --isolate-extensions.

Session restore was leading to OOPIFs for cross-site iframes in modes
with more limited OOPIF-creation policies, because the SiteInstance
groupings are not restored.  Instead, use the same logic as the transfer
case to determine whether a process swap is needed.

BUG=567424
TEST=See bug for repro steps.
==========

[Charlie Reis, 2015-12-08 22:12:06]

Closing in favor of https://codereview.chromium.org/1505343002/.