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.

(Passed all try jobs, but stuck on flaky linux_android_rel_ng job.)
NOTRY=true

BUG=567424
TEST=See bug for repro steps.

Committed: https://crrev.com/31e1b2453015fc63ab7aa69cace36092eea55760
Cr-Commit-Position: refs/heads/master@{#363857}

[Charlie Reis, 2015-12-08 19:32:03]

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

[Charlie Reis, 2015-12-08 19:32:03]

Nasko/Nick: Can you give this a sanity check?  It's just the combination of my
https://codereview.chromium.org/1506093002/ and Nick's
https://codereview.chromium.org/1507333002/, allowing us to land them both at
once instead of having them depend on each other.

[ncarter (slow), 2015-12-08 20:14:13]

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2723:
TestNavigationObserver observer(shell()->web_contents());
This TNO (and its enclosing scope) might be unnecessary.

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2745: if
(SiteIsolationPolicy::UseSubframeNavigationEntries()) {
I think you can EXPECT_TRUE(SiteIsolationPolicy::UseSubframeNavigationEntries())
at the top of the test, and drop the else {} branch here.

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2782: if
(SiteIsolationPolicy::UseDedicatedProcessesForAllSites()) {
AreAllSitesIsolatedForTesting() would be a better call here, though the
difference between the two doesn't currently exist (I expect it will reappear)

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2325: if
(!SiteIsolationPolicy::AreCrossProcessFramesPossible())
I think it might be cleaner to structure this as follows:

if (!frame_tree_node_->IsMainFrame()) {
  if (!IsRendererTransferNeededForNavigation(render_frame_host_.get(),
                                             dest_url)) {
    return render_frame_host_.get();
  }
  // Continuing in this case will result in an out-of-process iframe.
  CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
}

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2331: if
(!SiteIsolationPolicy::UseDedicatedProcessesForAllSites() &&
I don't think you need to check UseDedicatedProcessesForAllSites here.
IsRendererTransferNeededForNavigation takes it into consideration.

[Charlie Reis, 2015-12-08 20:34:12]

Thanks!  I'll run a CQ dry run.

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2723:
TestNavigationObserver observer(shell()->web_contents());
On 2015/12/08 20:14:13, ncarter wrote:
> This TNO (and its enclosing scope) might be unnecessary.

Done.

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2745: if
(SiteIsolationPolicy::UseSubframeNavigationEntries()) {
On 2015/12/08 20:14:13, ncarter wrote:
> I think you can
EXPECT_TRUE(SiteIsolationPolicy::UseSubframeNavigationEntries())
> at the top of the test, and drop the else {} branch here.

Done, with AreCrossProcessFramesPossible.

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2782: if
(SiteIsolationPolicy::UseDedicatedProcessesForAllSites()) {
On 2015/12/08 20:14:13, ncarter wrote:
> AreAllSitesIsolatedForTesting() would be a better call here, though the
> difference between the two doesn't currently exist (I expect it will reappear)

Done.

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2325: if
(!SiteIsolationPolicy::AreCrossProcessFramesPossible())
On 2015/12/08 20:14:13, ncarter wrote:
> I think it might be cleaner to structure this as follows:
> 
> if (!frame_tree_node_->IsMainFrame()) {
>   if (!IsRendererTransferNeededForNavigation(render_frame_host_.get(),
>                                              dest_url)) {
>     return render_frame_host_.get();
>   }
>   // Continuing in this case will result in an out-of-process iframe.
>   CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
> }

Sure.  As you noted offline, we may not get to the code below in
--site-per-process if we're already in the right site, but that's basically a
no-op anyway.  (We'll also need to improve
IsRendererTransferNeededForNavigation, but we already knew that.)

https://codereview.chromium.org/1505343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2331: if
(!SiteIsolationPolicy::UseDedicatedProcessesForAllSites() &&
On 2015/12/08 20:14:13, ncarter wrote:
> I don't think you need to check UseDedicatedProcessesForAllSites here.
> IsRendererTransferNeededForNavigation takes it into consideration.

Acknowledged.

[Charlie Reis, 2015-12-08 20:34:20]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 20:38:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505343002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505343002/20001

[commit-bot: I haz the power, 2015-12-08 20:59:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 20:59:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Reis, 2015-12-08 21:29:51]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 21:36:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505343002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505343002/40001

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

Here's the new version that passes
SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs.  I'll rebase to
pick up https://codereview.chromium.org/1507333002/.

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

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.

This also adds a content_shell flag to turn on partial site isolation.
(This is from nick@'s https://codereview.chromium.org/1507333002/.)

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.
==========

[ncarter (slow), 2015-12-08 22:16:18]

lgtm!

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

https://codereview.chromium.org/1505343002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2706:
DCHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
EXPECT_TRUE

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

https://codereview.chromium.org/1505343002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2330: // here for
session restore.
Maybe move these 3 comment lines down to just before IsRendererTransferNeeded?

https://codereview.chromium.org/1505343002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2331: // Note that if
dest_url is a unique origin like about:blank, then the need
drop "Note that"

[ncarter (slow), 2015-12-08 22:16:20]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-12-08 22:17:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505343002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505343002/100001

[Charlie Reis, 2015-12-09 00:13:01]

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.

(Passed all try jobs, but stuck on flaky linux_android_rel_ng job.)
NOTRY=true

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

[Charlie Reis, 2015-12-09 00:13:05]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2015-12-09 00:13:26]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-12-09 00:14:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 00:14:02]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Charlie Reis, 2015-12-09 00:14:36]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-12-09 00:18:31]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2015-12-09 00:18:37]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-12-09 00:23:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505343002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505343002/100001

[commit-bot: I haz the power, 2015-12-09 00:46:51]

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.

(Passed all try jobs, but stuck on flaky linux_android_rel_ng job.)
NOTRY=true

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.

(Passed all try jobs, but stuck on flaky linux_android_rel_ng job.)
NOTRY=true

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

[commit-bot: I haz the power, 2015-12-09 00:46:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-09 00:48:04]

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.

(Passed all try jobs, but stuck on flaky linux_android_rel_ng job.)
NOTRY=true

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.

(Passed all try jobs, but stuck on flaky linux_android_rel_ng job.)
NOTRY=true

BUG=567424
TEST=See bug for repro steps.

Committed: https://crrev.com/31e1b2453015fc63ab7aa69cace36092eea55760
Cr-Commit-Position: refs/heads/master@{#363857}
==========

[commit-bot: I haz the power, 2015-12-09 00:48:05]

Patchset 6 (id:??) landed as
https://crrev.com/31e1b2453015fc63ab7aa69cace36092eea55760
Cr-Commit-Position: refs/heads/master@{#363857}