Top document isolation mode prototype

Top document isolation mode prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

nick's patch (crrev.com/1797363002) provides the same functionality in a cleaner way.

BUG=594217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[Xiaocheng, 2016-03-10 10:52:21]

Description was changed from

==========
Doghouse prototype

Based on hanxi's patch: crrev.com/1348203005
==========

to

==========
Doghouse prototype

Based on hanxi's patch: crrev.com/1348203005
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Xiaocheng, 2016-03-10 10:53:35]

xiaochengh@chromium.org changed reviewers:
+ dominicc@chromium.org, kenjibaheux@chromium.org

[Xiaocheng, 2016-03-10 10:55:55]

Description was changed from

==========
Doghouse prototype

Based on hanxi's patch: crrev.com/1348203005
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Doghouse prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

BUG=n/a
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Xiaocheng, 2016-03-10 11:01:12]

PTAL.

This is essentially the doghouse prototype we are using, with all debug info
printing removed.

Do we want to make it less hacky?

Do we need to do anything regarding the comments left in
|CheckNavigationPolicyOnUI()| in cross_site_resource_handler.cc?

[ncarter (slow), 2016-03-11 00:21:34]

Description was changed from

==========
Doghouse prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

BUG=n/a
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Doghouse prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

BUG=n/a
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2016-03-11 00:21:34]

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

[ncarter (slow), 2016-03-11 00:31:28]

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

[ncarter (slow), 2016-03-11 00:31:28]

Two main challenges I see in how to clean this up:

(1) How to modify SiteInstance in a sane way, so that we can identify which
SiteInstances contain main frames by the time we do RenderProcessHost
allocation. I bet there's an elegant way to do this that we haven't thought of
yet.

(2) Teaching RenderProcessHostImpl about the doghouse process (the current
approach works by using a global). This shouldn't be too
bad;https://bugs.chromium.org/p/chromium/issues/detail?id=513036

And, of course, we'll want some browsertests.

[Xiaocheng, 2016-03-11 02:06:27]

On 2016/03/11 at 00:31:28, nick wrote:
> Two main challenges I see in how to clean this up:
> 
> (1) How to modify SiteInstance in a sane way, so that we can identify which
SiteInstances contain main frames by the time we do RenderProcessHost
allocation. I bet there's an elegant way to do this that we haven't thought of
yet.

I think so, but haven't thought deeply into it yet.

> 
> (2) Teaching RenderProcessHostImpl about the doghouse process (the current
approach works by using a global). This shouldn't be too
bad;https://bugs.chromium.org/p/chromium/issues/detail?id=513036

How about creating a new subclass of RenderProcessHostImpl as the doghouse's
RPH? It should be helpful if we want to introduce specialized behavior to the
doghouse process.

> 
> And, of course, we'll want some browsertests.

[ncarter (slow), 2016-03-11 17:47:21]

On 2016/03/11 02:06:27, Xiaocheng wrote:
> On 2016/03/11 at 00:31:28, nick wrote:
> > Two main challenges I see in how to clean this up:
> > 
> > (1) How to modify SiteInstance in a sane way, so that we can identify which
> SiteInstances contain main frames by the time we do RenderProcessHost
> allocation. I bet there's an elegant way to do this that we haven't thought of
> yet.
> 
> I think so, but haven't thought deeply into it yet.

It's ultimately a plumbing question.

The meaning of DogHouseProcess is currently:
 - Create a new SiteInstance whenever an <iframe> is different-site from its
parent.
 - Causing subframe-only SiteInstances to be grouped a singleton doghouse
process.

This means that in the current prototype, if we encounter an A(B(C)) case, both
B and C are in the doghouse process, but in different SiteInstances, so they
will interact with each other as remote frames (each site will get its own copy
of the frametree in the doghouse process, and we'd allocate O(site^2)
RemoteFrame objects for a single tab, and B->C communication would roundtrip
between the browser process, since B and C don't really know that they're
actually same-process.

This detail of the prototype could easily explain the V8 bloat in the
experiments.

We should probably switch this to an approach where we maintain one 'doghouse
SiteInstance per browsinginstance'. This actually cleanly separates this CL:
first we implement 'doghouse SiteInstance per browsinginstance' (without making
any changes to RPH allocation), and then we can follow up with the logic that
would cause the doghouse siteinstances of different tabs to share a doghouse
process.

Hopefully that approach can guide us to a reasonable implementation. I expect
we'll see an is_doghouse_ bit on SiteInstanceImpl.

> > 
> > (2) Teaching RenderProcessHostImpl about the doghouse process (the current
> approach works by using a global). This shouldn't be too
> bad;https://bugs.chromium.org/p/chromium/issues/detail?id=513036
> 
> How about creating a new subclass of RenderProcessHostImpl as the doghouse's
> RPH? It should be helpful if we want to introduce specialized behavior to the
> doghouse process.

My instinct is that a subclass of RenderProcessHostImpl won't be necessary. Once
allocated, a RPH doesn't really need to behave all that differently between the
Doghouse and non-Doghouse cases. Already we have differentiated render process
types (extensions) while still only using on RenderProcessHostImpl.

To the extent we need to modify RenderProcessHostImpl, it may not go much deeper
than the process picking-and-assignment logic, which actually consists of static
methods (in which case, a subclass doesn't buy us anything). We might consider
encapsulating that logic in some kind of singleton allocator class, but I don't
think it's required. This means functions like
RenderProcessHost::ShouldTryToUseExistingProcessHost,
RenderProcessHost::GetExistingProcessHost,
RenderProcessHostImpl::GetProcessHostForSite. FWIW, that last one,
RenderProcessHostImpl::GetProcessHostForSite, is kind of nasty because in unit
tests it can be called with objects that are not RenderProcessHostImpls (it's
safe because it doesn't downcast case, but it actually feels improper to me).

> > And, of course, we'll want some browsertests.

Here are some corner cases we'll want to consider, and test.
 - Interactions between ServiceWorker and doghouse (is it possible for a SW to
wind up in a doghouse process, or can we always avoid that?)
 - Interactions between extensions and doghouse (likely, we ought not doghouse
extension frames)
 - Interactions between --isolate-extensions and --doghouse and their field
trial equivalents (does doghouse imply --isolate-extensions?)
 - Interactions between --site-per-process and --doghouse (Almost certainly,
--site-per-process should trump)
 - Interactions between <webview> and doghouse (the patch already handles some
cases here)

Any ideas on the above (in particular, the ServiceWorker question?)

[ncarter (slow), 2016-03-11 19:33:26]

Description was changed from

==========
Doghouse prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

BUG=n/a
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Doghouse prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

BUG=594217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Xiaocheng, 2016-03-14 09:30:53]

On 2016/03/11 at 17:47:21, nick wrote:
> On 2016/03/11 02:06:27, Xiaocheng wrote:
> > On 2016/03/11 at 00:31:28, nick wrote:
> > > Two main challenges I see in how to clean this up:
> > > 
> > > (1) How to modify SiteInstance in a sane way, so that we can identify
which
> > SiteInstances contain main frames by the time we do RenderProcessHost
> > allocation. I bet there's an elegant way to do this that we haven't thought
of
> > yet.
> > 
> > I think so, but haven't thought deeply into it yet.
> 
> It's ultimately a plumbing question.
> 
> The meaning of DogHouseProcess is currently:
>  - Create a new SiteInstance whenever an <iframe> is different-site from its
parent.
>  - Causing subframe-only SiteInstances to be grouped a singleton doghouse
process.
> 
> This means that in the current prototype, if we encounter an A(B(C)) case,
both B and C are in the doghouse process, but in different SiteInstances, so
they will interact with each other as remote frames (each site will get its own
copy of the frametree in the doghouse process, and we'd allocate O(site^2)
RemoteFrame objects for a single tab, and B->C communication would roundtrip
between the browser process, since B and C don't really know that they're
actually same-process.
> 
> This detail of the prototype could easily explain the V8 bloat in the
experiments.
> 
> We should probably switch this to an approach where we maintain one 'doghouse
SiteInstance per browsinginstance'. This actually cleanly separates this CL:
first we implement 'doghouse SiteInstance per browsinginstance' (without making
any changes to RPH allocation), and then we can follow up with the logic that
would cause the doghouse siteinstances of different tabs to share a doghouse
process.
> 
> Hopefully that approach can guide us to a reasonable implementation. I expect
we'll see an is_doghouse_ bit on SiteInstanceImpl.

This approach looks better in terms of both semantic and efficiency.

I'm implementing it but it's crashing. The repro steps are: (1) Open a page with
3rd-party iframes; (2) Navigate to a page without 3rd-party iframe (e.g.
google.com).

Maybe I'm not implementing it in the right way. My approach is simple: for each
BrowsingInstance, the first created SiteInstance is the main house; the second
created SiteInstance is the doghouse; and there is no other SiteInstance created
-- whenever we need a SiteInstance whose site is not the main house's site,
return the doghouse.

Please let me know if you spot any clear mistake in my implementation, as you
have much more experience on site isolation than me. I'll also investigate the
cause of the crash by myself (in some not that efficient way, though...).

> 
> > > 
> > > (2) Teaching RenderProcessHostImpl about the doghouse process (the current
> > approach works by using a global). This shouldn't be too
> > bad;https://bugs.chromium.org/p/chromium/issues/detail?id=513036
> > 
> > How about creating a new subclass of RenderProcessHostImpl as the doghouse's
> > RPH? It should be helpful if we want to introduce specialized behavior to
the
> > doghouse process.
> 
> My instinct is that a subclass of RenderProcessHostImpl won't be necessary.
Once allocated, a RPH doesn't really need to behave all that differently between
the Doghouse and non-Doghouse cases. Already we have differentiated render
process types (extensions) while still only using on RenderProcessHostImpl.
> 
> To the extent we need to modify RenderProcessHostImpl, it may not go much
deeper than the process picking-and-assignment logic, which actually consists of
static methods (in which case, a subclass doesn't buy us anything). We might
consider encapsulating that logic in some kind of singleton allocator class, but
I don't think it's required. This means functions like
RenderProcessHost::ShouldTryToUseExistingProcessHost,
RenderProcessHost::GetExistingProcessHost,
RenderProcessHostImpl::GetProcessHostForSite. FWIW, that last one,
RenderProcessHostImpl::GetProcessHostForSite, is kind of nasty because in unit
tests it can be called with objects that are not RenderProcessHostImpls (it's
safe because it doesn't downcast case, but it actually feels improper to me).

I'm convinced that a doghouse subclass of RPHImpl is unnecessary. Besides, its
functionality seems to be subsumed by the doghouse SiteInstance you suggested.

> 
> > > And, of course, we'll want some browsertests.
> 
> Here are some corner cases we'll want to consider, and test.
>  - Interactions between ServiceWorker and doghouse (is it possible for a SW to
wind up in a doghouse process, or can we always avoid that?)
>  - Interactions between extensions and doghouse (likely, we ought not doghouse
extension frames)
>  - Interactions between --isolate-extensions and --doghouse and their field
trial equivalents (does doghouse imply --isolate-extensions?)

Semantically, doghouse should imply --isolate-extensions since both options
require isolating 3rd-party content.

>  - Interactions between --site-per-process and --doghouse (Almost certainly,
--site-per-process should trump)

When multiple site isolation options are turned on, do we just pick the one that
results in the finest process granularity? If so, then --site-per-process should
trump.

However, this prototype relies on --site-per-process to enable doghouse. Some
major change is needed if we want to change that.

>  - Interactions between <webview> and doghouse (the patch already handles some
cases here)
> 
> Any ideas on the above (in particular, the ServiceWorker question?)

[ncarter (slow), 2016-03-14 17:42:24]

https://codereview.chromium.org/1777233002/diff/20001/content/browser/browsin...
File content/browser/browsing_instance.cc (right):

https://codereview.chromium.org/1777233002/diff/20001/content/browser/browsin...
content/browser/browsing_instance.cc:57: instance->SetForDoghouse(use_doghouse);
I think this choice (of which SiteInstance becomes the doghouse) needs to
somehow be aware of main frame / subframe state, which simply isn't available at
this level.

My expectation is that the first SiteInstance we create will be for the main
frame, which we want to be non-doghouse process.

The crashes you see may be process kills due to the LockToOrigin checks.
Basically, you cannot have UseDedicatedProcessesForAllSites() return true and
then also put two different sites in the same renderer process: the process will
be locked only to the first origin, and we will kill the renderer when the
second site tries to e.g. read document.cookie.

The solution is to not have UseDedicatedProcessesForAllSites() return true in
doghouse mode. Ultimately doghouse is going to be a fourth site isolation mode
(after --site-per-process, --isolate-extensions, and
--isolate-sites-for-testing). Getting this mode implemented right seems like
something that will require a pretty deep knowledge of the process model, so I'm
happy to take it on -- I actually started hacking on it on Friday. I am
optimistic that I can have a patch up today.

[Xiaocheng, 2016-03-15 08:10:12]

On 2016/03/14 at 17:42:24, nick wrote:
>
https://codereview.chromium.org/1777233002/diff/20001/content/browser/browsin...
> File content/browser/browsing_instance.cc (right):
> 
>
https://codereview.chromium.org/1777233002/diff/20001/content/browser/browsin...
> content/browser/browsing_instance.cc:57:
instance->SetForDoghouse(use_doghouse);
> I think this choice (of which SiteInstance becomes the doghouse) needs to
somehow be aware of main frame / subframe state, which simply isn't available at
this level.
> 
> My expectation is that the first SiteInstance we create will be for the main
frame, which we want to be non-doghouse process.

With more testing, I found out that this is not always the case. When opening a
new tab, the first SiteInstance is created for chrome-search://remote-ntp/. If I
navigate to another site, in some cases (which I haven't figured out exactly)
the navigation is done without creating a new BrowsingInstance, and hence, the
SiteInstance created for the main frame is not first created SiteInstance.

Of course we can do something like ignoring internal protocols (like
chrome-search://) when deciding if a SiteInstance should be for doghouse. It
looks like quite a dirty hack, though. A cleaner approach should be, as you
said, looking at the frame tree.

> The crashes you see may be process kills due to the LockToOrigin checks.
Basically, you cannot have UseDedicatedProcessesForAllSites() return true and
then also put two different sites in the same renderer process: the process will
be locked only to the first origin, and we will kill the renderer when the
second site tries to e.g. read document.cookie.

Thanks for your comments -- it is indeed the cause of the crash. It no longer
crashes after adding "if (IsForDoghouse()) return" in
SiteInstanceImpl::LockToOrigin.

> The solution is to not have UseDedicatedProcessesForAllSites() return true in
doghouse mode. Ultimately doghouse is going to be a fourth site isolation mode
(after --site-per-process, --isolate-extensions, and
--isolate-sites-for-testing). Getting this mode implemented right seems like
something that will require a pretty deep knowledge of the process model, so I'm
happy to take it on -- I actually started hacking on it on Friday. I am
optimistic that I can have a patch up today.

A probably more complicated issue is what we should do when navigating from
doghouse (say, clicking a link in an ad which opens a new tab)? The ad should be
in the doghouse, while the new tab should not. However, both tabs are in the
same BrowsingInstance, which means that the ad and the new tab are using the
same SiteInstance, which in turn means the new tab has to be in the doghouse.

I'm wondering if there's any solution without altering the current model of
SiteInstance and BrowsingInstance. Otherwise, maybe we need to redesign
SiteInstance to allow multiple SiteInstances of the same site in the same
BrowsingInstance.

[ncarter (slow), 2016-03-16 19:34:55]

On 2016/03/15 08:10:12, Xiaocheng wrote:
> On 2016/03/14 at 17:42:24, nick wrote:
> >
>
https://codereview.chromium.org/1777233002/diff/20001/content/browser/browsin...
> > File content/browser/browsing_instance.cc (right):
> > 
> >
>
https://codereview.chromium.org/1777233002/diff/20001/content/browser/browsin...
> > content/browser/browsing_instance.cc:57:
> instance->SetForDoghouse(use_doghouse);
> > I think this choice (of which SiteInstance becomes the doghouse) needs to
> somehow be aware of main frame / subframe state, which simply isn't available
at
> this level.
> > 
> > My expectation is that the first SiteInstance we create will be for the main
> frame, which we want to be non-doghouse process.
> 
> With more testing, I found out that this is not always the case. When opening
a
> new tab, the first SiteInstance is created for chrome-search://remote-ntp/. If
I
> navigate to another site, in some cases (which I haven't figured out exactly)
> the navigation is done without creating a new BrowsingInstance, and hence, the
> SiteInstance created for the main frame is not first created SiteInstance.
> 
> Of course we can do something like ignoring internal protocols (like
> chrome-search://) when deciding if a SiteInstance should be for doghouse. It
> looks like quite a dirty hack, though. A cleaner approach should be, as you
> said, looking at the frame tree.

I think the right strategy here is for the internal URLs ought to be registered
as ProcessPerSite, and for content to enforce a policy of 'keep ProcessPerSite
URLs out of the doghouse'.

For the New Tab Page, to prevent the iframes from being put in the 3rd party
process, I think we just need to do a bit more work, to make it so that
ChromeContentBrowserClient::ShouldUseProcessPerSite() returns true for the
iframe URLs (e.g. chrome-search://most-visited/ ). We can either keep the
mostvisited iframes in the NTP process, or we can put them in a second singleton
process.

> > The crashes you see may be process kills due to the LockToOrigin checks.
> Basically, you cannot have UseDedicatedProcessesForAllSites() return true and
> then also put two different sites in the same renderer process: the process
will
> be locked only to the first origin, and we will kill the renderer when the
> second site tries to e.g. read document.cookie.
> 
> Thanks for your comments -- it is indeed the cause of the crash. It no longer
> crashes after adding "if (IsForDoghouse()) return" in
> SiteInstanceImpl::LockToOrigin.
> 
> > The solution is to not have UseDedicatedProcessesForAllSites() return true
in
> doghouse mode. Ultimately doghouse is going to be a fourth site isolation mode
> (after --site-per-process, --isolate-extensions, and
> --isolate-sites-for-testing). Getting this mode implemented right seems like
> something that will require a pretty deep knowledge of the process model, so
I'm
> happy to take it on -- I actually started hacking on it on Friday. I am
> optimistic that I can have a patch up today.
> 
> A probably more complicated issue is what we should do when navigating from
> doghouse (say, clicking a link in an ad which opens a new tab)? The ad should
be
> in the doghouse, while the new tab should not. However, both tabs are in the
> same BrowsingInstance, which means that the ad and the new tab are using the
> same SiteInstance, which in turn means the new tab has to be in the doghouse.

Some top-level navigations will have to commit into the 3rd party process; this
is unavoidable. However, each subsequent navigation might give us an opportunity
to break free of the 3rd party process. I think the behavior we want is
something like "Top-level navigations should switch away from the 3rd party
process whenever possible". Historically we don't switch processes for normal
cross-site navigations, unless required for security reasons, because it it's
considered more efficient to avoid the process startup cost. But I expect that
cost is worth paying, if the alternative is paying a performance penalty by
staying in the 3rd party process.

We'll need code for this, and we'll need to test back/forward cases as well,
since the SiteInstance decisions we make are actually cached in the navigation
entries, I expect some tricky cases there.

> I'm wondering if there's any solution without altering the current model of
> SiteInstance and BrowsingInstance. Otherwise, maybe we need to redesign
> SiteInstance to allow multiple SiteInstances of the same site in the same
> BrowsingInstance.

It sounds like we're thinking about the same things.

I hope to avoid having multiple SiteInstances for the same site within a
BrowsingInstance, because that means allowing same-site frames to exist which
can postmessage each other, but not script each other, which is not how the
platform is supposed to work. I hope that we can solve this simply, by switching
BrowsingInstances; if that doesn't work in practice, we can talk about
approaches that involve architectural changes to the
SiteInstance/BrowsingInstance model.

[Xiaocheng, 2016-03-17 06:15:07]

Description was changed from

==========
Doghouse prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

BUG=594217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Top document isolation mode prototype

Based on hanxi's patch (crrev.com/1348203005) with modification.

nick's patch (crrev.com/1797363002) provides the same functionality in a cleaner
way.

BUG=594217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========