Consolidate all TDI SiteInstances into one render process.

Consolidate all TDI SiteInstances into one render process.

BUG=674215
Review-Url: https://codereview.chromium.org/2573213002
Cr-Commit-Position: refs/heads/master@{#441832}
Committed: https://chromium.googlesource.com/chromium/src/+/6d686db1331d37430e668c0cf87201815f01c128

[Avi (use Gerrit), 2016-12-14 21:46:14]

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

[commit-bot: I haz the power, 2016-12-14 21:46:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 22:16:38]

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

[commit-bot: I haz the power, 2016-12-14 22:16:38]

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

[Avi (use Gerrit), 2016-12-21 18:05:55]

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

[commit-bot: I haz the power, 2016-12-21 18:06:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 18:57:57]

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

[commit-bot: I haz the power, 2016-12-21 18:57:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2016-12-21 21:48:16]

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

[commit-bot: I haz the power, 2016-12-21 21:48:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 23:07:35]

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

[commit-bot: I haz the power, 2016-12-21 23:07:36]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-12-21 23:07:45]

avi@chromium.org changed reviewers:
+ nick@chromium.org

[Avi (use Gerrit), 2016-12-21 23:07:46]

WDYT?

[Avi (use Gerrit), 2016-12-21 23:13:37]

Description was changed from

==========
Consolidate all TDI SiteInstances into one render process.

BUG=674215
==========

to

==========
Consolidate all TDI SiteInstances into one render process.

BUG=674215
==========

[ncarter (slow), 2017-01-03 21:29:56]

https://codereview.chromium.org/2573213002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2573213002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2427: if
(host->IsNeverSuitableForReuse())
See my later comment about why this might be problematic.

I wonder if we should instead just combine the IsNeverSuitableForReuse bit with
the GetContentClient()->browser()->MayReuseHost check?

Like, have the IsNeverSuitableForReuse getter call
GetContentClient()->browser()->MayReuseHost(), and replace the two existing
calls to the MayReuseHost function, to calls for RPHI::IsNeverSuitableForReuse()
?

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
content/browser/site_instance_impl.cc:102:
BrowserContext::GetDefaultStoragePartition(browser_context_));
Hmm, I hadn't anticipated this. Calling GetDefaultStoragePartition means we
might risk teleporting subframes to a different storage partition from the main
frame.

Since the storage partition is passed to the RPH ctor, and a RPH seems to really
only support one storage partition (its message filters assume a single storage
partition), I think we need to figure out how TDI interacts with something that
uses a non-default storage partition; <webview> tag with a storage partition ID.
Also, Charlie says that expanded usage of storage partitions are in the
development pipeline.

So we can do one of the following things:
 [1] Suppress TDI when the main frame is in a non-default storage partition, or
 [2] Make the DefaultSubframeProcessHostHolder be one-per-StoragePartition.
 [3] Pass the storage partition as a parameter, and just have this function
return a new RPH each time (with IsNeverSuitableForReuse = true) for the
non-default storage partition case.

[1] should be a straightforward extension to the existing exceptions we
currently have, but I think [3] is even easier.

We could implement [2] by making the StoragePartitionImpl have a
default_subframe_site_instance_holder_ member directly. You can get the storage
partition for siteinstance via the BrowserContext::GetStoragePartition() variant
that accepts a SiteInstance* argument.

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
content/browser/site_instance_impl.cc:286: }
I'm worried that this will break --isolate-extensions &&
--top-document-isolation, if you navigate from
a(b) to a(chrome-extension). Does it?

What's tricky is that HasWrongProcessForURL has a factor that asks, "is this a
special URL that shouldn't be in any ordinary renderer process", and another
factor that considers "is this a special renderer process that shouldn't house
ordinary URLs." For TDI we still want to allow swaps out of the RPH due to the
first factor, but we want to suppress incidental reuse due to the second factor.

I think we should probably drop these lines, and hoist the consideration of the
RPH-is-garbage-heap bit out of RPHI::IsSuitableHost to the callers of that
function.

[ncarter (slow), 2017-01-03 21:34:56]

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
content/browser/site_instance_impl.cc:286: }
On 2017/01/03 21:29:56, ncarter wrote:
> I'm worried that this will break --isolate-extensions &&
> --top-document-isolation, if you navigate from
> a(b) to a(chrome-extension). Does it?
> 
> What's tricky is that HasWrongProcessForURL has a factor that asks, "is this a
> special URL that shouldn't be in any ordinary renderer process", and another
> factor that considers "is this a special renderer process that shouldn't house
> ordinary URLs." For TDI we still want to allow swaps out of the RPH due to the
> first factor, but we want to suppress incidental reuse due to the second
factor.
> 
> I think we should probably drop these lines, and hoist the consideration of
the
> RPH-is-garbage-heap bit out of RPHI::IsSuitableHost to the callers of that
> function.

Note that
https://cs.chromium.org/chromium/src/chrome/browser/site_details_browsertest....
is a test that exercises this case. I don't think it has expectations for the
TDI  case, but it could be a starting point for investigation.

[Avi (use Gerrit), 2017-01-05 19:33:24]

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

[commit-bot: I haz the power, 2017-01-05 19:33:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-01-05 19:44:53]

https://codereview.chromium.org/2573213002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2573213002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2427: if
(host->IsNeverSuitableForReuse())
On 2017/01/03 21:29:56, ncarter wrote:
> See my later comment about why this might be problematic.
> 
> I wonder if we should instead just combine the IsNeverSuitableForReuse bit
with
> the GetContentClient()->browser()->MayReuseHost check?
> 
> Like, have the IsNeverSuitableForReuse getter call
> GetContentClient()->browser()->MayReuseHost(), and replace the two existing
> calls to the MayReuseHost function, to calls for
RPHI::IsNeverSuitableForReuse()
> ?

I'm not sure what the difference would be. All the calls to MayReuseHost are
next to RenderProcessHostImpl::IsSuitableHost.

BTW, the difference between ContentBrowserClient's IsSuitableHost and
MayReuseHost is subtle and not clear.

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
content/browser/site_instance_impl.cc:102:
BrowserContext::GetDefaultStoragePartition(browser_context_));
On 2017/01/03 21:29:56, ncarter wrote:
> Hmm, I hadn't anticipated this. Calling GetDefaultStoragePartition means we
> might risk teleporting subframes to a different storage partition from the
main
> frame.
> 
> Since the storage partition is passed to the RPH ctor, and a RPH seems to
really
> only support one storage partition (its message filters assume a single
storage
> partition), I think we need to figure out how TDI interacts with something
that
> uses a non-default storage partition; <webview> tag with a storage partition
ID.
> Also, Charlie says that expanded usage of storage partitions are in the
> development pipeline.
> 
> So we can do one of the following things:
>  [1] Suppress TDI when the main frame is in a non-default storage partition,
or
>  [2] Make the DefaultSubframeProcessHostHolder be one-per-StoragePartition.
>  [3] Pass the storage partition as a parameter, and just have this function
> return a new RPH each time (with IsNeverSuitableForReuse = true) for the
> non-default storage partition case.
> 
> [1] should be a straightforward extension to the existing exceptions we
> currently have, but I think [3] is even easier.
> 
> We could implement [2] by making the StoragePartitionImpl have a
> default_subframe_site_instance_holder_ member directly. You can get the
storage
> partition for siteinstance via the BrowserContext::GetStoragePartition()
variant
> that accepts a SiteInstance* argument.

[3] as discussed in chat.

https://codereview.chromium.org/2573213002/diff/40001/content/browser/site_in...
content/browser/site_instance_impl.cc:286: }
On 2017/01/03 21:29:56, ncarter wrote:
> I'm worried that this will break --isolate-extensions &&
> --top-document-isolation, if you navigate from
> a(b) to a(chrome-extension). Does it?

I don't know.

> I think we should probably drop these lines, and hoist the consideration of
the
> RPH-is-garbage-heap bit out of RPHI::IsSuitableHost to the callers of that
> function.

What I was trying to do with these lines is to fix the popup case. If we have
A(B) and the B subframe does a popup, then we need to force that popup to live
in the TDI process with the B subframe. Without these two lines, what happens is
that RPHI is consulted for where to put the popup, sees the TDI process, says
"nope" to reusing it, and puts the popup in its own process.

I'm confused about your suggestion above.

[Avi (use Gerrit), 2017-01-05 20:13:35]

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

[commit-bot: I haz the power, 2017-01-05 20:13:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-01-05 21:06:37]

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

[commit-bot: I haz the power, 2017-01-05 21:07:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-01-05 21:17:36]

Well, your suggestion works. I'm still trying to get my head around the
difference between IsSuitableHost and MayReuseHost.

[Avi (use Gerrit), 2017-01-05 21:17:37]

Well, your suggestion works. I'm still trying to get my head around the
difference between IsSuitableHost and MayReuseHost.

[ncarter (slow), 2017-01-05 22:17:36]

On 2017/01/05 21:17:37, Avi wrote:
> Well, your suggestion works. I'm still trying to get my head around the
> difference between IsSuitableHost and MayReuseHost.

There are places where we call IsSuitableHost without calling MayReuseHost. It's
kind of a can/should distinction.

IsSuitableHost is about "would it be secure/correct to put this activity in this
process?". Importantly it's called from HasWrongProcessForURL, which we used to
trigger transfers in some cases.

MayReuseHost is about "Would I have a good time if I were randomly transferred
into this process (assuming it would be secure to do so)? Or does it contain an
unleashed dog that is likely to maul me?".

[ncarter (slow), 2017-01-05 22:29:05]

lgtm

https://codereview.chromium.org/2573213002/diff/100001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2573213002/diff/100001/content/browser/site_i...
content/browser/site_instance_impl.cc:139: // The render process for the default
storage partition of this
I'd probably say "the TDI render process" or something similar, in case
someone's reading this without scrolling up to see the class name.

https://codereview.chromium.org/2573213002/diff/100001/content/browser/site_i...
content/browser/site_instance_impl.cc:171: bool is_for_guests_only =
site_.SchemeIs(kGuestScheme);
Currently, |is_for_guests_only| is mutually exclusive with
IsDefaultSubframeSiteInstance(). But I think the way you've factored things out
here seems reasonable and safe.

[commit-bot: I haz the power, 2017-01-05 22:36:59]

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

[commit-bot: I haz the power, 2017-01-05 22:37:00]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-01-06 01:19:45]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-01-06 01:19:45]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2573213002/#ps120001
(title: "comment nit")

[Avi (use Gerrit), 2017-01-06 01:20:12]

https://codereview.chromium.org/2573213002/diff/100001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2573213002/diff/100001/content/browser/site_i...
content/browser/site_instance_impl.cc:139: // The render process for the default
storage partition of this
On 2017/01/05 22:29:05, ncarter wrote:
> I'd probably say "the TDI render process" or something similar, in case
> someone's reading this without scrolling up to see the class name.

Done.

https://codereview.chromium.org/2573213002/diff/100001/content/browser/site_i...
content/browser/site_instance_impl.cc:171: bool is_for_guests_only =
site_.SchemeIs(kGuestScheme);
On 2017/01/05 22:29:05, ncarter wrote:
> Currently, |is_for_guests_only| is mutually exclusive with
> IsDefaultSubframeSiteInstance(). But I think the way you've factored things
out
> here seems reasonable and safe.

Acknowledged.

[commit-bot: I haz the power, 2017-01-06 01:21:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 02:29:01]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483665585313860,
"parent_rev": "02ce1e46f9fa3be6c17254a5410c05af121fe241", "commit_rev":
"6d686db1331d37430e668c0cf87201815f01c128"}

[commit-bot: I haz the power, 2017-01-06 02:29:29]

Description was changed from

==========
Consolidate all TDI SiteInstances into one render process.

BUG=674215
==========

to

==========
Consolidate all TDI SiteInstances into one render process.

BUG=674215

Review-Url: https://codereview.chromium.org/2573213002
Cr-Commit-Position: refs/heads/master@{#441832}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d686db1331d37430e668c0cf872...
==========

[commit-bot: I haz the power, 2017-01-06 02:29:30]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/6d686db1331d37430e668c0cf872...