Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1797363002:
"Top Document Isolation" mode (Closed)
|
Created:
4 years, 9 months ago by ncarter (slow) Modified:
4 years, 8 months ago CC:
asvitkine+watch_chromium.org, blink-worker-reviews_chromium.org, brettw, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dominicc (has gone to gerrit), Xi Han, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, loading-reviews_chromium.org, michaeln, nasko+codewatch_chromium.org, nhiroki, ojan, serviceworker-reviews, tzik, Xiaocheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce --top-document-isolation mode, a new process model wherein the top
document is process separated from its subframes when possible, but the subframes
are all housed in a single renderer process.
Currently this mode only shares the subframe process within a browsing instance:
usually, that will mean sharing the subframe process within a tab. In follow up patches
we can extend the sharing so that we might share subframe RenderProcessHosts
among unrelated BrowsingInstances, when legal.
BUG=594217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/d5bbd0b067cf0c1c8e40c7b5c2d1a1a39c0db10f
Cr-Commit-Position: refs/heads/master@{#384364}
Patch Set 1 #Patch Set 2 : Add browser tests #Patch Set 3 : Rebased #Patch Set 4 : Two new browsertests. #
Total comments: 11
Patch Set 5 : New version that doesn't look at SiteURL #Patch Set 6 : Add Xiaocheng's test (without fixes) #Patch Set 7 : Fix assertions in xiaocheng's test. It has reasonable behavior now. #Patch Set 8 : Self review fixes. #Patch Set 9 : histograms.xml #Patch Set 10 : Add missing override #
Total comments: 62
Patch Set 11 : Reviewer fixes #Patch Set 12 : Wrap ExecuteScript in ASSERT_TRUE #Patch Set 13 : Fix up tests. Necessary rebase to obtain browsertest util behavior change. #
Total comments: 9
Patch Set 14 : Reviewer fixes. #Patch Set 15 : One does not simply mark SiteInstanceImpl as final #Patch Set 16 : Whitespace. #Patch Set 17 : merge #Patch Set 18 : scoped_refptr in GetDefaultSubframeSiteInstance #Patch Set 19 : Actually save browsing_instance.h #Patch Set 20 : Remove lame comment. #Patch Set 21 : SiteInstanceImpl #Patch Set 22 : Suppress tests under --site-per-process #Dependent Patchsets: Messages
Total messages: 64 (28 generated)
Description was changed from ========== --isolate-top-document mode BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce a new process model wherein the top document is process separated from its subframes when possible, and we share these subframe-only processes to the extent possible. Currently this mode only shares the subframe process within a browsing instance (usually, that will mean sharing the process within a tab). In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. There are some known corner cases that are definitely not right yet (see, especially, the behavior of IsolateTopDocumentBrowserTest.NavigateToSubframeSite, where we wind up in an upside-down state where the main frame is in the third-party process, and the subframe is in a dedicated process. However, I think this is a good starting point for experiments. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
nick@chromium.org changed reviewers: + brettw@chromium.org, creis@chromium.org, nasko@chromium.org
Input welcome from all. creis/nasko: This is still somewhat rough around the edges, but it seems to be working in the new browsertests, so I'd appreciate your thoughts on the overall approach. As we discussed in person, it's not yet clear if "for third party subframes" ought to be a kind of SiteInstance (as I've implemented), or if it's simply a state that a SiteInstance can transition in/out of. brettw: you'd expressed some opinions about names, so please let me know what you think about this. There are actually three different naming schemes at work here (the flag is "isolate top document"; the subframe processes have a bit called "for third party subframes"; and the SiteIsolationPolicy getter calls the mode UseDedicatedProcessForTopDocument()).
Description was changed from ========== Introduce a new process model wherein the top document is process separated from its subframes when possible, and we share these subframe-only processes to the extent possible. Currently this mode only shares the subframe process within a browsing instance (usually, that will mean sharing the process within a tab). In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. There are some known corner cases that are definitely not right yet (see, especially, the behavior of IsolateTopDocumentBrowserTest.NavigateToSubframeSite, where we wind up in an upside-down state where the main frame is in the third-party process, and the subframe is in a dedicated process. However, I think this is a good starting point for experiments. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce --isolate-top-document mode, a new process model wherein the top document is process separated from its subframes when possible, but the subframes are all housed in a single renderer process. Currently this mode only shares the subframe process within a browsing instance (usually, that will mean sharing the process within a tab). In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. I think this is a good starting point for experiments. There are some known corner cases that are definitely not right yet. For example, in the test IsolateTopDocumentBrowserTest.NavigateToSubframeSite we wind up in an upside- down state: the main frame is in the third-party process, and the subframe is in a non-third-party, because of the navigation order. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Question: in the same BrowsingInstance, is it allowed to split the frames of the same site into different SiteInstances? Consider this scenario: 1. Open a page with frame structure A(B, C). We should have a normal SiteInstance for A and a 3rd-party SiteInstance for B and C. 2. The main frame (A) opens a popup whose main frame is just C. Because C is in main frame now, in the same BrowsingInstance, a normal SiteInstance is created for C. Now frames of C are split into two SiteInstances in the same BrowsingInstance. If this is not allowed, then we should store in the BrowsingInstance the set of sites that have ever appeared as a subframe, and always use the 3rd-party SiteInstance for them (regardless of whether they appear in another page as the main frame).
On 2016/03/16 08:03:32, Xiaocheng wrote:
> Question: in the same BrowsingInstance, is it allowed to split the frames of
the
> same site into different SiteInstances?
>
> Consider this scenario:
> 1. Open a page with frame structure A(B, C). We should have a normal
> SiteInstance for A and a 3rd-party SiteInstance for B and C.
> 2. The main frame (A) opens a popup whose main frame is just C. Because C is
in
> main frame now, in the same BrowsingInstance, a normal SiteInstance is created
> for C.
>
> Now frames of C are split into two SiteInstances in the same BrowsingInstance.
>
> If this is not allowed, then we should store in the BrowsingInstance the set
of
> sites that have ever appeared as a subframe, and always use the 3rd-party
> SiteInstance for them (regardless of whether they appear in another page as
the
> main frame).
This is a great example and I'll add it as a new browsertest case.
The correct behavior here is for the c.com popup to wind up in the doghouse
process. This is required for use cases where a subframe creates a popup via
window.open('about:blank'), and then writes into its document. For that to work,
it needs to be same process.
In your example, this scenario works correctly for b.com, but I expect it may
fall apart for some c.com use case (a c.com about:blank may stay in the
doghouse, but I bet navigating the popup away from and then back to c.com would
create a new process and break synchronous scripting). This is, as you mention,
because b.com is registered as the SiteURL of the SiteInstance and we don't
record the existence of c.com activity. This functional bug likely exists with
--isolate-extensions as well, and I think it's a priority for the site isolation
team to fix the GetSiteURL() situation, and very soon.
Based on some conversations with charlie and nasko yesterday, I've jotted down a
some notes about how we might proceed with that:
https://docs.google.com/document/d/1PGfHM-c8d4ZZkXE4tF3ri7V6zaYOmbHk13hOpNUI2...
Also, Xiaocheng, if you hadn't looked already, please have a peek at the
IsolateTopDocumentBrowserTest.NavigateToSubframeSite, which illustrates a
similar bug with a different root cause. We are less certain how to keep that
navigation out of the 3rd party process, but I see two broad approaches:
Approach 1 is we switch to a new browsing instance for top-level navs like this
-- such switching is not always legal, but definitely is legal in the common
case if there's only one webcontents in the browsing instance. Approach 2
involves leaving these cases in the same '3rd party' process, and essentially
turning off the '3rd party process' mode for that process.
I'd like to accumulate a bunch of these tricky cases in
IsolateTopDocumentBrowserTest, and iterate on the solutions.
On 2016/03/16 at 18:58:27, nick wrote:
> On 2016/03/16 08:03:32, Xiaocheng wrote:
> > Question: in the same BrowsingInstance, is it allowed to split the frames of
the
> > same site into different SiteInstances?
> >
> > Consider this scenario:
> > 1. Open a page with frame structure A(B, C). We should have a normal
> > SiteInstance for A and a 3rd-party SiteInstance for B and C.
> > 2. The main frame (A) opens a popup whose main frame is just C. Because C is
in
> > main frame now, in the same BrowsingInstance, a normal SiteInstance is
created
> > for C.
> >
> > Now frames of C are split into two SiteInstances in the same
BrowsingInstance.
> >
> > If this is not allowed, then we should store in the BrowsingInstance the set
of
> > sites that have ever appeared as a subframe, and always use the 3rd-party
> > SiteInstance for them (regardless of whether they appear in another page as
the
> > main frame).
>
> This is a great example and I'll add it as a new browsertest case.
>
> The correct behavior here is for the c.com popup to wind up in the doghouse
process. This is required for use cases where a subframe creates a popup via
window.open('about:blank'), and then writes into its document. For that to work,
it needs to be same process.
>
> In your example, this scenario works correctly for b.com, but I expect it may
fall apart for some c.com use case (a c.com about:blank may stay in the
doghouse, but I bet navigating the popup away from and then back to c.com would
create a new process and break synchronous scripting). This is, as you mention,
because b.com is registered as the SiteURL of the SiteInstance and we don't
record the existence of c.com activity. This functional bug likely exists with
--isolate-extensions as well, and I think it's a priority for the site isolation
team to fix the GetSiteURL() situation, and very soon.
>
> Based on some conversations with charlie and nasko yesterday, I've jotted down
a some notes about how we might proceed with that:
https://docs.google.com/document/d/1PGfHM-c8d4ZZkXE4tF3ri7V6zaYOmbHk13hOpNUI2...
>
> Also, Xiaocheng, if you hadn't looked already, please have a peek at the
IsolateTopDocumentBrowserTest.NavigateToSubframeSite, which illustrates a
similar bug with a different root cause. We are less certain how to keep that
navigation out of the 3rd party process, but I see two broad approaches:
Approach 1 is we switch to a new browsing instance for top-level navs like this
-- such switching is not always legal, but definitely is legal in the common
case if there's only one webcontents in the browsing instance. Approach 2
involves leaving these cases in the same '3rd party' process, and essentially
turning off the '3rd party process' mode for that process.
>
> I'd like to accumulate a bunch of these tricky cases in
IsolateTopDocumentBrowserTest, and iterate on the solutions.
I haven't thought of new buggy cases with different causes, yet. I have some
idea on the process model, though.
It appears to me that Approach 1 is always a suboptimal solution:
- When there are multiple WebContents, we haven't figured out the correct
behavior yet, and it seems equally tricky for approach 1 to decide when to
switch to a new BrowsingInstance;
- When there is only one WebContents, it's actually the easy case (in my
understanding) because the correct behavior is clean and well-defined. Just
always put the main frame in a dedicated SiteInstance, and all 3rd-party
subframes in the doghouse. The bug in
IsolateTopDocumentBrowserTest.NavigateToSubframeSite should be solvable by
unregistering the doghouse SiteInstance before navigating in the main frame, and
hence there is no need to switch BrowsingInstance. I'm trying to implement this
part.
At a (30000 feet) high level, I'm thinking about the following approach. When
creating a new frame or navigating in an existing frame, we need to decide which
SiteInstance should be used by this frame. The decision should be done by look
at the resulting frame tree/forest of the operation.
- If it will be the only frame of its site, the decision simply depends on
whether the frame is a 3rd-party subframe or not;
- Otherwise, return the same SiteInstance that the other frames of the same site
are using.
This seems to be a more general approach and should be able to solve the bugs we
have found. To make it work, as far as I can see here is a list of things that
should be done (which I'm also trying to implement):
1. As proposed in the notes, allow SiteInstance to store a set of URLs instead
of just one, and correct the meaning of GetSiteURL(). BrowsingInstance should
also store a many-to-one map between URLs and SiteInstances.
2. RenderFrameHostManager::IsRendererTransferNeededForNavigation() should look
ahead into the resulting frame tree/forest of the navigation.
3. Solve the issue of having multiple SiteInstances of the same site during the
navigation. For example, during the navigation A(B, C) -> B(A, C)), there will
be two SiteInstances existing simultaneously for B, the doghouse and a new
SiteInstance dedicated to B in the main frame. This might not be an issue,
though, as the two SiteInstances cannot script each other during the navigation
(I guess). If we need to solve this, then we might need to manually unload all
the subframes first.
As before, please let me know if you spot any mistake because I don't have much
experience in this area...
Sorry to chime in late-- it's been a busy week. Looks like you all are discovering some of the fun issues here. At a high level, it's obviously impossible to get both "all same-origin frames in a BrowsingInstance live in the same process" and "subframe processes only have subframes." We'll need to decide where we make tradeoffs and why, and whether common inversions are worth avoiding. Ultimately, a shared doc will be a better place to decide these things than this CL discussion, but this gives us a good start. I do want to point out that we do *not* have the "all same-origin frames in a BrowsingInstance live in the same process" property in default Chrome today. Repro steps: page A opens another window to page A, same process. User navigates second window to B via omnibox, going cross-process. User now clicks a link to C in both windows. Since it's renderer-initiated, we don't do a process swap in either and we now have two C pages that live in different processes, despite having a window.opener connection. We could fix this with better knowledge of current sites in the BrowsingInstance map, but I'll point out that it hasn't really been a problem over the lifetime of Chrome, and it may give us some flexibility in which approach we choose here. (Rationale: the user kind of broke the script connection when navigating to B, so it's ok for the two C instances to be separate.) Some specific comments below. On 2016/03/17 10:07:34, Xiaocheng wrote: > On 2016/03/16 at 18:58:27, nick wrote: > > On 2016/03/16 08:03:32, Xiaocheng wrote: > > > Question: in the same BrowsingInstance, is it allowed to split the frames of > the > > > same site into different SiteInstances? > > > > > > Consider this scenario: > > > 1. Open a page with frame structure A(B, C). We should have a normal > > > SiteInstance for A and a 3rd-party SiteInstance for B and C. > > > 2. The main frame (A) opens a popup whose main frame is just C. Because C is > in > > > main frame now, in the same BrowsingInstance, a normal SiteInstance is > created > > > for C. > > > > > > Now frames of C are split into two SiteInstances in the same > BrowsingInstance. > > > > > > If this is not allowed, then we should store in the BrowsingInstance the set > of > > > sites that have ever appeared as a subframe, and always use the 3rd-party > > > SiteInstance for them (regardless of whether they appear in another page as > the > > > main frame). Agreed that this is a nice example. As I mention above, we don't actually always keep same-site pages in the same SiteInstance, even though that's necessary for cross-frame scripting. We might want to consider whether it's needed in this case. Specifically, consider the difference between whether the main frame opens the popup to C or whether the C subframe opens the popup to C. If the C subframe opens the popup to C, we definitely want to keep it in C's process. That's a common practice for things like OAuth. If the A main frame opens the popup to C, you might make the case that the subframe and popup don't really know about each other (except through A) and that it might not be necessary for them to script each other. That would allow us to use a main frame process for the C popup. In fact, it would be quite similar to our current process model, where the C popup would stay in A's process because there's no need for it to get a different process. (It's a renderer-initiated navigation from A to C.) I kind of like that this is minimally different from our current process model. To implement it, we simply wouldn't register the sites in the subframe process in the BrowsingInstance, and we'd use the RFH's last committed URL in process swap decisions for transfers (as we do for browser-initiated navigations today). The tradeoff here is that we get a simpler process model and avoid some cases of top frames being in subframe processes (and vice versa), at the risk of breaking cross-frame scripting between same-origin pages in a few more cases than we do today. That's a tough question in general but might be a good place to start for implementing this prototype. (So far, it looks like it handles cases we expect to be common pretty well, and only causes issues in cases that don't seem as likely to happen in practice.) > It appears to me that Approach 1 is always a suboptimal solution: > - When there are multiple WebContents, we haven't figured out the correct > behavior yet, and it seems equally tricky for approach 1 to decide when to > switch to a new BrowsingInstance; > - When there is only one WebContents, it's actually the easy case (in my > understanding) because the correct behavior is clean and well-defined. Just > always put the main frame in a dedicated SiteInstance, and all 3rd-party > subframes in the doghouse. The bug in > IsolateTopDocumentBrowserTest.NavigateToSubframeSite should be solvable by > unregistering the doghouse SiteInstance before navigating in the main frame, and > hence there is no need to switch BrowsingInstance. I'm trying to implement this > part. Nick and I chatted some yesterday about the problem of unregistering a subframe's site from the BrowsingInstance before the site is gone, and I don't think I'm a fan. There's a period of time where the pending top-level RFH for B coexists with the subframe for B, until we know that the top-level frame is going to commit. Even with PlzNavigate, there's still a short period of time where they overlap. We end up with a problem where a B popup could script both the old subframe and the new top-level page. That popup might not exist at the time we create the pending top-level RFH for B, but the subframe could create it during the time window after we create the pending RFH and before the subframe is deleted. I don't think there's a practical way to solve this without breaking some script connections. (As I mention above, though, maybe we're ok with some connections getting broken.) We definitely want to be aware of any code complexity we introduce, though. Trying to predict which subframes will go away due to a navigation when deciding which process to put it in is non-trivial when combined with the rest of the policy code. Let me clean up and share some of my notes as well. I think we need to make some decisions about which aspects of the process model we're going to solve now vs later (and whether delaying some of them to later makes things better or worse).
My comments below are to see if we can explore the other approach I suggested, which I think will be a bit less of a delta from today's behavior. We'll have some cases where same-site pages can't script each other (in ways that I think are unlikely to matteer in practice), but it should fit well with Chrome's default process model. Basic premise: keep a subframe SiteInstance per BrowsingInstance, and don't register it in the map. (It can have a site assigned to it, but we won't look it up.) We put subframes in there whenever they're a different site than the top frame's last committed URL, as well as popups that such a subframe opens. Other top-level navigations won't try to go into that process even if it's to a site that's in the process. https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... File content/browser/browsing_instance.cc (right): https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... content/browser/browsing_instance.cc:51: if (HasSiteInstance(url)) I'm going to suggest skipping this. Don't worry about breaking the script connections to preexisting main frames, at least for the time being. It would be unusual for the subframe to depend on such a window existing. https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... File content/browser/browsing_instance.h (right): https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... content/browser/browsing_instance.h:76: SiteInstance* GetSiteInstanceForThirdPartySubframes(const GURL& url); We might not need this if we don't register subframe sites in the map. https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... content/browser/browsing_instance.h:125: SiteInstance* site_instance_for_third_party_subframes_ = nullptr; We probably do want this, as the sole subframe SI (which isn't in the map). https://codereview.chromium.org/1797363002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1414: if (rfh->GetSiteInstance()->HasRelatedSiteInstance(effective_url)) { Note: this affects --isolate-extensions as well. Repro steps: 1) Page A opens a new window. 2) Navigate new window to B (new process, same BrowsingInstance). 3) Page A adds a subframe to B. We create an OOPIF for it in --isolate-extensions, even though there's no extensions involved. We should avoid that. https://codereview.chromium.org/1797363002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/1797363002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_version.h:427: mojo::InterfacePtr<Interface> interface_ptr) Not necessary in this CL? https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... content/browser/site_instance_impl.cc:151: browsing_instance_->RegisterSiteInstance(this); Maybe BI ignores this call for the subframe SI? https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... content/browser/site_instance_impl.cc:223: SiteIsolationPolicy::UseDedicatedProcessForTopDocument()) { Might not need this. https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... content/browser/site_instance_impl.h:47: SiteInstance* GetRelatedSiteInstanceForThirdPartySubframes(const GURL& url); Maybe drop "Related" and |url|? https://codereview.chromium.org/1797363002/diff/60001/content/common/site_iso... File content/common/site_isolation_policy.h (right): https://codereview.chromium.org/1797363002/diff/60001/content/common/site_iso... content/common/site_isolation_policy.h:42: static bool UseDedicatedProcessForTopDocument(); IsTopDocumentIsolationEnabled or something similar? In the new approach, we wouldn't actually be treating the top document as needing a dedicated process (which is a smaller delta from Chrome's current process model).
On 2016/03/18 21:15:13, Charlie Reis wrote: > My comments below are to see if we can explore the other approach I suggested, > which I think will be a bit less of a delta from today's behavior. We'll have > some cases where same-site pages can't script each other (in ways that I think > are unlikely to matteer in practice), but it should fit well with Chrome's > default process model. > > Basic premise: keep a subframe SiteInstance per BrowsingInstance, and don't > register it in the map. (It can have a site assigned to it, but we won't look > it up.) We put subframes in there whenever they're a different site than the > top frame's last committed URL, as well as popups that such a subframe opens. > Other top-level navigations won't try to go into that process even if it's to a > site that's in the process. > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... > File content/browser/browsing_instance.cc (right): > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... > content/browser/browsing_instance.cc:51: if (HasSiteInstance(url)) > I'm going to suggest skipping this. Don't worry about breaking the script > connections to preexisting main frames, at least for the time being. It would > be unusual for the subframe to depend on such a window existing. > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... > File content/browser/browsing_instance.h (right): > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... > content/browser/browsing_instance.h:76: SiteInstance* > GetSiteInstanceForThirdPartySubframes(const GURL& url); > We might not need this if we don't register subframe sites in the map. > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/browsin... > content/browser/browsing_instance.h:125: SiteInstance* > site_instance_for_third_party_subframes_ = nullptr; > We probably do want this, as the sole subframe SI (which isn't in the map). > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/frame_h... > File content/browser/frame_host/render_frame_host_manager.cc (right): > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/frame_h... > content/browser/frame_host/render_frame_host_manager.cc:1414: if > (rfh->GetSiteInstance()->HasRelatedSiteInstance(effective_url)) { > Note: this affects --isolate-extensions as well. Repro steps: > 1) Page A opens a new window. > 2) Navigate new window to B (new process, same BrowsingInstance). > 3) Page A adds a subframe to B. > > We create an OOPIF for it in --isolate-extensions, even though there's no > extensions involved. We should avoid that. > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/service... > File content/browser/service_worker/service_worker_version.h (right): > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/service... > content/browser/service_worker/service_worker_version.h:427: > mojo::InterfacePtr<Interface> interface_ptr) > Not necessary in this CL? > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... > File content/browser/site_instance_impl.cc (right): > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... > content/browser/site_instance_impl.cc:151: > browsing_instance_->RegisterSiteInstance(this); > Maybe BI ignores this call for the subframe SI? > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... > content/browser/site_instance_impl.cc:223: > SiteIsolationPolicy::UseDedicatedProcessForTopDocument()) { > Might not need this. > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... > File content/browser/site_instance_impl.h (right): > > https://codereview.chromium.org/1797363002/diff/60001/content/browser/site_in... > content/browser/site_instance_impl.h:47: SiteInstance* > GetRelatedSiteInstanceForThirdPartySubframes(const GURL& url); > Maybe drop "Related" and |url|? > > https://codereview.chromium.org/1797363002/diff/60001/content/common/site_iso... > File content/common/site_isolation_policy.h (right): > > https://codereview.chromium.org/1797363002/diff/60001/content/common/site_iso... > content/common/site_isolation_policy.h:42: static bool > UseDedicatedProcessForTopDocument(); > IsTopDocumentIsolationEnabled or something similar? In the new approach, we > wouldn't actually be treating the top document as needing a dedicated process > (which is a smaller delta from Chrome's current process model). Xiaocheng & Charlie: thank you both for this very detailed feedback. I will be back soon with an updated patch.
xiaochengh@chromium.org changed reviewers: + xiaochengh@chromium.org
I edited Charlie's document with a new case but haven't received any response. Sorry that I should have sent you some notification. https://codereview.chromium.org/1797363002/diff/60001/content/browser/isolate... File content/browser/isolate_top_document_browsertest.cc (right): https://codereview.chromium.org/1797363002/diff/60001/content/browser/isolate... content/browser/isolate_top_document_browsertest.cc:36: void SetUpOnMainThread() { Please add |override|. https://codereview.chromium.org/1797363002/diff/60001/content/browser/isolate... content/browser/isolate_top_document_browsertest.cc:408: Can we add the following test case? It doesn't contain any new bug, but the story seems quite natural. IN_PROC_BROWSER_TEST_F(IsolateTopDocumentBrowserTest, PopupAndRedirection) { GURL main_url(embedded_test_server()->GetURL( "a.com", "/cross_site_iframe_factory.html?a(b)")); // User opens page A which contains a subframe from an ad platform B. NavigateToURL(shell(), main_url); EXPECT_EQ( " Site A ------------ proxies for B\n" " +--Site B ------- proxies for A\n" "Where A = http://a.com/\n" " B = http://b.com/ (3rd party)", DepictFrameTree(root())); GURL c_url(embedded_test_server()->GetURL( "c.com", "/cross_site_iframe_factory.html?c")); // B retrieves an ad from advertiser C and redirects the subframe to C. NavigateFrameToURL(root()->child_at(0), c_url); // The subframe still uses the same third-party SiteInstance after navigation. EXPECT_EQ( " Site A ------------ proxies for B\n" " +--Site B ------- proxies for A\n" "Where A = http://a.com/\n" " B = http://b.com/ (3rd party)", DepictFrameTree(root())); GURL b_url(embedded_test_server()->GetURL( "b.com", "/cross_site_iframe_factory.html?b")); std::string script = "popup = window.open('" + b_url.spec() + "');"; ShellAddedObserver new_shell_observer; // User clicks the ad in the subframe, which opens a popup of B. EXPECT_TRUE(ExecuteScript(root()->child_at(0)->current_frame_host(), script)); Shell* popup = new_shell_observer.GetShell(); FrameTreeNode* popup_root = static_cast<WebContentsImpl*>(popup->web_contents()) ->GetFrameTree() ->root(); // TODO: This is a bug. There is no longer any subframe of B, so the popup // should use a dedicated SiteInstance instead of the 3rd party one. EXPECT_EQ( " Site A ------------ proxies for B\n" " +--Site B ------- proxies for A\n" "Where A = http://a.com/\n" " B = http://b.com/ (3rd party)", DepictFrameTree(root())); EXPECT_EQ( " Site B\n" "Where B = http://b.com/ (3rd party)", DepictFrameTree(popup_root)); // The popup redirects itself to the advertiser's website C. NavigateToURL(popup, c_url); // TODO: This is a bug, caused by the fact that the BrowsingInstance // didn't record that SiteInstance B is also in use by c.com. EXPECT_EQ( " Site A ------------ proxies for B C\n" " +--Site B ------- proxies for A C\n" "Where A = http://a.com/\n" " B = http://b.com/ (3rd party)\n" " C = http://c.com/", DepictFrameTree(root())); EXPECT_EQ( " Site C ------------ proxies for B\n" "Where B = http://b.com/ (3rd party)\n" " C = http://c.com/", DepictFrameTree(popup_root)); }
There are a couple TODOs still, but this is ready for another look from reviewers. Xiaocheng: Thank you for the test. I've incorporated it and updated the assertions to match the new behavior, which seems less buggy. Note that the ad platform is temporarily in its own (short-lived) process during the popup creation.
non-committer's l-g-t-m with nits. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... File content/browser/isolate_top_document_browsertest.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:338: // TODO(nick): These subframes ought to end up in the third-party process, nit: This TODO can be removed now. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:416: // subframe to C. nit: C -> ad.com
Very nice! I like the small delta from our current behavior and the effect that has on the tests. I think this is pretty much ready to go, after the relatively minor comments below. nit: Update CL description to say --top-document-isolation. Also, it looks like the CL description's comment about NavigateToSubframeSite might not be correct? That test doesn't look like it's upside down anymore. https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... File content/browser/browsing_instance.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... content/browser/browsing_instance.cc:47: SiteInstance* BrowsingInstance::GetDefaultSubframeSiteInstance() { Can we put a CHECK(IsTopDocumentIsolationEnabled()) here? Or at least AreCrossProcessFramesPossible()? Just want to make sure we don't call it in default Chrome. https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... content/browser/browsing_instance.cc:52: instance->SetSite(GURL("http://web-subframes.invalid")); Yeah, we can chat about what to put here long term (or how to avoid it). https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1351: // mode, but should be safe to enable unconditionally. I think I agree, but I also agree with guarding it behind the flag for now just in case. (I'd like to think about the opener case more closely for other modes.) https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1375: SiteInstanceRelation::RELATED_DEFAULT_SUBFRAME); Yes, I think these changes make sense. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1424: bool subframe = !frame_tree_node_->IsMainFrame(); nit: is_subframe (Does this help with clarity? We use the IsMainFrame() call directly in the logic above, so I wouldn't feel bad doing the same below.) https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1490: if (!candidate->GetLastCommittedOrigin().unique() && This check takes a while to understand and depends a lot on context (i.e., that we know dest_url is cross-site from the last successful URL). Can you add a comment clarifying what this is for? I thought it was so that we keep foo.com in the same process if the previous page was about:blank in a foo.com origin, but IsSameWebSite kind of accounts for about:blank. (Or maybe it doesn't handle that specific case?) Anyway, I'm a bit unclear here. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1494: return true; Maybe this overall block would be easier to read as: if (...has last successful url...) { if (...IsSameWebSite...) return true; return (...origin check...); } return (...IsSameWebSite...); https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:536: // In case |existing_site_instance| is null, specify if the new site should nit: specify how the new site is related to the current BrowsingInstance. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... File content/browser/isolate_top_document_browsertest.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:23: // TODO(nick): Rename this class/file to match the new switch name. Yep, worth doing. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:163: EXPECT_EQ( Maybe add a comment about how (at least for now) we're ok with the a.com subframe staying in the subframe process, despite having a connection (via window.top.opener.top) to another a.com frame? https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:173: EXPECT_EQ( Let's add a comment here as well, saying we're ok with the new b.com ending up in a top-level process and not the same process as the b.com popup frame (at least for now). https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:264: // party process as well. s/3rd party/subframe/ Also, this comment disagrees with the expectation below. Maybe we can replace this comment and the TODO with a description of how we currently behave? https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:265: GURL ba_url(embedded_test_server()->GetURL( nit: ca_url? https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:271: // feature? I think this is probably ok for now. The subframe that created the popup is gone, so any script connections after that are a step removed and might be ok to break. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:280: // c.com popup should remain where it was, in the subframe process. Is there a reason to check this? We haven't changed anything on the popup. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:319: // TODO(nick): Without --isolate-top-document, we would not swap processes nit: --top-document-isolation, here and below. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:321: // --isolate-top-document? I don't follow. We do swap processes for top-level cross-site browser-initiated navigations like these, even in default Chrome. Do you mean for renderer-initiated navigations? Hopefully --top-document-isolation doesn't introduce new swaps there. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:402: // User opens page on page.com which contains a subframe from adnetwork.com nit: End with period. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:417: // TODO(nick): Make this a renderer-inititated navigation. Yep, let's do this. Shouldn't change the outcome, but it's good coverage for the renderer-initiated case. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:429: // platform's domain. s/platform/network/ https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:439: EXPECT_EQ( Let's add a comment about how we think it's ok for the popup to break out of the subframe process because it's cross-site from its opener frame. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:455: // This must join its same-site opener, in the default subframe SiteInstance. Wow, I'm impressed this passes. That's a cool consequence of your opener change to DetermineSiteInstanceForURL. https://codereview.chromium.org/1797363002/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1521: switches::kTopDocumentIsolation, Ah yes. I was wondering why we need this in the renderer process, but it affects AreCrossProcessFramesPossible, which gets called there. https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... content/browser/site_instance_impl.h:164: size_t active_frame_count_ = 0; Style question: Is this a new practice that we're moving to in general? I haven't seen inline initialization in other code yet, and I kind of like knowing where to look (i.e., the constructor(s)). I won't object if this style has already been discussed, though. https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... content/browser/site_instance_impl.h:180: bool is_default_subframe_site_instance_ = false; nit: Add a blank line before and a comment about this. https://codereview.chromium.org/1797363002/diff/180001/content/common/site_is... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/common/site_is... content/common/site_isolation_policy.cc:32: // --site-per-process trumps --isolate-top-document. nit: --top-document-isolation This reminds me-- is there an easy way to run try jobs with custom command line flags? It'd be good to run the tests with only --isolate-extensions (to make sure we don't regress) and with both --isolate-extensions and --top-document-isolation (to make sure things work when a TDI user is in our Finch trial).
Description was changed from ========== Introduce --isolate-top-document mode, a new process model wherein the top document is process separated from its subframes when possible, but the subframes are all housed in a single renderer process. Currently this mode only shares the subframe process within a browsing instance (usually, that will mean sharing the process within a tab). In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. I think this is a good starting point for experiments. There are some known corner cases that are definitely not right yet. For example, in the test IsolateTopDocumentBrowserTest.NavigateToSubframeSite we wind up in an upside- down state: the main frame is in the third-party process, and the subframe is in a non-third-party, because of the navigation order. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce --top-document-isolation mode, a new process model wherein the top document is process separated from its subframes when possible, but the subframes are all housed in a single renderer process. Currently this mode only shares the subframe process within a browsing instance: usually, that will mean sharing the subframe process within a tab. In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... File content/browser/browsing_instance.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... content/browser/browsing_instance.cc:47: SiteInstance* BrowsingInstance::GetDefaultSubframeSiteInstance() { On 2016/03/25 19:47:12, Charlie Reis wrote: > Can we put a CHECK(IsTopDocumentIsolationEnabled()) here? Or at least > AreCrossProcessFramesPossible()? Just want to make sure we don't call it in > default Chrome. I made this the stricter variant, CHECK(IsTopDocumentIsolationEnabled()). It's still not 100% clear how SiteInstance coalescing will work in the extension(a,b) case, and whether that will use the default subframe siteinstance or not, so let's restrict it to --top-document-isolation for now, and relax this constraint as we make progress. https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... content/browser/browsing_instance.cc:52: instance->SetSite(GURL("http://web-subframes.invalid")); On 2016/03/25 19:47:12, Charlie Reis wrote: > Yeah, we can chat about what to put here long term (or how to avoid it). If we rip this out now, the PopupAndRedirection test sees a pretty big behavior change. I think that's a sign of us falling back to the SiteURL somewhere we shouldn't be, but for now I think it's safest to use a sentinel value here. Open to other suggestions here -- let me know if you have ideas. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1351: // mode, but should be safe to enable unconditionally. On 2016/03/25 19:47:12, Charlie Reis wrote: > I think I agree, but I also agree with guarding it behind the flag for now just > in case. (I'd like to think about the opener case more closely for other > modes.) Acknowledged. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1375: SiteInstanceRelation::RELATED_DEFAULT_SUBFRAME); On 2016/03/25 19:47:12, Charlie Reis wrote: > Yes, I think these changes make sense. Acknowledged. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1424: bool subframe = !frame_tree_node_->IsMainFrame(); On 2016/03/25 19:47:12, Charlie Reis wrote: > nit: is_subframe > > (Does this help with clarity? We use the IsMainFrame() call directly in the > logic above, so I wouldn't feel bad doing the same below.) I've inlined it -- this was an artifact of an earlier, more complicated comparison.. FWIW, I'm now passionate about unifying this with DetermineSiteInstanceForURL, because it seems like it's going to keep evolving. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1490: if (!candidate->GetLastCommittedOrigin().unique() && On 2016/03/25 19:47:12, Charlie Reis wrote: > This check takes a while to understand and depends a lot on context (i.e., that > we know dest_url is cross-site from the last successful URL). Can you add a > comment clarifying what this is for? > > I thought it was so that we keep http://foo.com in the same process if the previous > page was about:blank in a http://foo.com origin, but IsSameWebSite kind of accounts for > about:blank. (Or maybe it doesn't handle that specific case?) Anyway, I'm a > bit unclear here. > I've clarified the intent here with a comment. My understanding is that IsSameWebSite handles the dest_url == about:blank case, but it doesn't have enough info to correctly handle the src_url =="about:blank" case, even if it wanted to. So that's the main value to this origin check. Adding this origin check carries some risk of behavior change for non-TDI, but I actually think the test coverage for the about:blank cases is pretty solid. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1494: return true; On 2016/03/25 19:47:12, Charlie Reis wrote: > Maybe this overall block would be easier to read as: > > if (...has last successful url...) { > if (...IsSameWebSite...) > return true; > return (...origin check...); > } > > return (...IsSameWebSite...); I've rearranged the code, hopefully it's better now. I also put in a TODO with your name on it about eliminating the need for the GetSiteURL() fallback. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:536: // In case |existing_site_instance| is null, specify if the new site should On 2016/03/25 19:47:12, Charlie Reis wrote: > nit: specify how the new site is related to the current BrowsingInstance. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... File content/browser/isolate_top_document_browsertest.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:23: // TODO(nick): Rename this class/file to match the new switch name. On 2016/03/25 19:47:13, Charlie Reis wrote: > Yep, worth doing. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:163: EXPECT_EQ( On 2016/03/25 19:47:12, Charlie Reis wrote: > Maybe add a comment about how (at least for now) we're ok with the a.com > subframe staying in the subframe process, despite having a connection (via > window.top.opener.top) to another a.com frame? Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:173: EXPECT_EQ( On 2016/03/25 19:47:13, Charlie Reis wrote: > Let's add a comment here as well, saying we're ok with the new b.com ending up > in a top-level process and not the same process as the b.com popup frame (at > least for now). Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:264: // party process as well. On 2016/03/25 19:47:13, Charlie Reis wrote: > s/3rd party/subframe/ > > Also, this comment disagrees with the expectation below. Maybe we can replace > this comment and the TODO with a description of how we currently behave? Done. I also clarified what the difference is between the first and second version of this test is. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:265: GURL ba_url(embedded_test_server()->GetURL( On 2016/03/25 19:47:12, Charlie Reis wrote: > nit: ca_url? Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:271: // feature? On 2016/03/25 19:47:12, Charlie Reis wrote: > I think this is probably ok for now. The subframe that created the popup is > gone, so any script connections after that are a step removed and might be ok to > break. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:280: // c.com popup should remain where it was, in the subframe process. On 2016/03/25 19:47:12, Charlie Reis wrote: > Is there a reason to check this? We haven't changed anything on the popup. Yes. Proxies could have been created. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:319: // TODO(nick): Without --isolate-top-document, we would not swap processes On 2016/03/25 19:47:12, Charlie Reis wrote: > nit: --top-document-isolation, here and below. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:321: // --isolate-top-document? On 2016/03/25 19:47:12, Charlie Reis wrote: > I don't follow. We do swap processes for top-level cross-site browser-initiated > navigations like these, even in default Chrome. > > Do you mean for renderer-initiated navigations? Hopefully > --top-document-isolation doesn't introduce new swaps there. My original prototype was based on making the top level SiteInstances be dedicated, so we would have swapped processes even for renderer-initiated navs, which is what this comment is about. But I screwed up, and made them browser-initiated navigations. I've fixed this in the test (now it's a mix of renderer-and-browser initiated navs), and removed the comment. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:338: // TODO(nick): These subframes ought to end up in the third-party process, On 2016/03/25 08:30:52, Xiaocheng wrote: > nit: This TODO can be removed now. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:402: // User opens page on page.com which contains a subframe from adnetwork.com On 2016/03/25 19:47:12, Charlie Reis wrote: > nit: End with period. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:416: // subframe to C. On 2016/03/25 08:30:52, Xiaocheng wrote: > nit: C -> http://ad.com Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:417: // TODO(nick): Make this a renderer-inititated navigation. On 2016/03/25 19:47:13, Charlie Reis wrote: > Yep, let's do this. Shouldn't change the outcome, but it's good coverage for > the renderer-initiated case. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:429: // platform's domain. On 2016/03/25 19:47:12, Charlie Reis wrote: > s/platform/network/ Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:439: EXPECT_EQ( On 2016/03/25 19:47:12, Charlie Reis wrote: > Let's add a comment about how we think it's ok for the popup to break out of the > subframe process because it's cross-site from its opener frame. Done. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:455: // This must join its same-site opener, in the default subframe SiteInstance. On 2016/03/25 19:47:12, Charlie Reis wrote: > Wow, I'm impressed this passes. That's a cool consequence of your opener change > to DetermineSiteInstanceForURL. Acknowledged. https://codereview.chromium.org/1797363002/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1521: switches::kTopDocumentIsolation, On 2016/03/25 19:47:13, Charlie Reis wrote: > Ah yes. I was wondering why we need this in the renderer process, but it > affects AreCrossProcessFramesPossible, which gets called there. Exactly right. Hilarious things happen if you forget this, especially around the renderer process and browser process seeing different results for SiteIsolationPolicy::UseSubframeNavigationEntries(). https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... content/browser/site_instance_impl.h:164: size_t active_frame_count_ = 0; On 2016/03/25 19:47:13, Charlie Reis wrote: > Style question: Is this a new practice that we're moving to in general? I > haven't seen inline initialization in other code yet, and I kind of like knowing > where to look (i.e., the constructor(s)). I won't object if this style has > already been discussed, though. It's been allowed for over a year: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 (It's also on http://chromium-cpp.appspot.com/ as "non-static Non-Static Class Member Initializer"). Personally I prefer it if it's used consistently within a class, and it's particularly beneficial when there is more than one ctor. But I would be happy to retract this part of the change if you want. It didn't occur to me that it would be controversial. https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... content/browser/site_instance_impl.h:180: bool is_default_subframe_site_instance_ = false; On 2016/03/25 19:47:13, Charlie Reis wrote: > nit: Add a blank line before and a comment about this. Done. https://codereview.chromium.org/1797363002/diff/180001/content/common/site_is... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/common/site_is... content/common/site_isolation_policy.cc:32: // --site-per-process trumps --isolate-top-document. On 2016/03/25 19:47:13, Charlie Reis wrote: > nit: --top-document-isolation > > This reminds me-- is there an easy way to run try jobs with custom command line > flags? It'd be good to run the tests with only --isolate-extensions (to make > sure we don't regress) and with both --isolate-extensions and > --top-document-isolation (to make sure things work when a TDI user is in our > Finch trial). I'm not sure about the tryjobs, but I was planning on looking at that soon by writing some chrome browser tests that set both flags. This is on my tracking list.
Two random drive-bys. https://codereview.chromium.org/1797363002/diff/240001/content/browser/browsi... File content/browser/browsing_instance.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/browsi... content/browser/browsing_instance.cc:63: DCHECK(static_cast<SiteInstanceImpl*>(site_instance) Unrelated to this CL, but is there a reason RegisterSiteInstance() and UnregisterSiteInstance() don't just take a SiteInstanceImpl? It seems weird to always pass in SiteInstanceImpls as SiteInstance and then downcast back to SiteInstanceImpl. https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1186: // different than our current SiteInstance), so that it is ref counted. Perhaps this function should just return a scoped_refptr. It's only called in two places that both immediately store the returned value into a scoped_refptr anyway, and it makes it easier to not footgun ourselves in the future.
https://codereview.chromium.org/1797363002/diff/240001/content/browser/browsi... File content/browser/browsing_instance.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/browsi... content/browser/browsing_instance.cc:63: DCHECK(static_cast<SiteInstanceImpl*>(site_instance) On 2016/03/29 08:09:16, dcheng wrote: > Unrelated to this CL, but is there a reason RegisterSiteInstance() and > UnregisterSiteInstance() don't just take a SiteInstanceImpl? It seems weird to > always pass in SiteInstanceImpls as SiteInstance and then downcast back to > SiteInstanceImpl. Done locally, will upload with other reviewer fixes. https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1186: // different than our current SiteInstance), so that it is ref counted. On 2016/03/29 08:09:16, dcheng wrote: > Perhaps this function should just return a scoped_refptr. It's only called in > two places that both immediately store the returned value into a scoped_refptr > anyway, and it makes it easier to not footgun ourselves in the future. I agree, though obviously it doesn't belong in this CL. There's actually a longstanding TODO about this, dating back to the initial check-in: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... I think we can do that with some c++11 wizardry right?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1186: // different than our current SiteInstance), so that it is ref counted. On 2016/03/29 at 16:28:33, ncarter wrote: > On 2016/03/29 08:09:16, dcheng wrote: > > Perhaps this function should just return a scoped_refptr. It's only called in > > two places that both immediately store the returned value into a scoped_refptr > > anyway, and it makes it easier to not footgun ourselves in the future. > > I agree, though obviously it doesn't belong in this CL. > > There's actually a longstanding TODO about this, dating back to the initial check-in: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > I think we can do that with some c++11 wizardry right? scoped_refptr is already movable, so I think we can just go ahead and do that already. I'll prepare a patch for it… as soon as my machine cert finishes propagating.
LGTM! https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... File content/browser/browsing_instance.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/browsi... content/browser/browsing_instance.cc:47: SiteInstance* BrowsingInstance::GetDefaultSubframeSiteInstance() { On 2016/03/28 22:00:28, ncarter wrote: > On 2016/03/25 19:47:12, Charlie Reis wrote: > > Can we put a CHECK(IsTopDocumentIsolationEnabled()) here? Or at least > > AreCrossProcessFramesPossible()? Just want to make sure we don't call it in > > default Chrome. > > I made this the stricter variant, CHECK(IsTopDocumentIsolationEnabled()). It's > still not 100% clear how SiteInstance coalescing will work in the extension(a,b) > case, and whether that will use the default subframe siteinstance or not, so > let's restrict it to --top-document-isolation for now, and relax this constraint > as we make progress. Acknowledged. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1490: if (!candidate->GetLastCommittedOrigin().unique() && On 2016/03/28 22:00:28, ncarter wrote: > On 2016/03/25 19:47:12, Charlie Reis wrote: > > This check takes a while to understand and depends a lot on context (i.e., > that > > we know dest_url is cross-site from the last successful URL). Can you add a > > comment clarifying what this is for? > > > > I thought it was so that we keep http://foo.com in the same process if the > previous > > page was about:blank in a http://foo.com origin, but IsSameWebSite kind of > accounts for > > about:blank. (Or maybe it doesn't handle that specific case?) Anyway, I'm a > > bit unclear here. > > > > I've clarified the intent here with a comment. > > My understanding is that IsSameWebSite handles the dest_url == about:blank case, > but it doesn't have enough info to correctly handle the src_url =="about:blank" > case, even if it wanted to. So that's the main value to this origin check. > Adding this origin check carries some risk of behavior change for non-TDI, but I > actually think the test coverage for the about:blank cases is pretty solid. Acknowledged. https://codereview.chromium.org/1797363002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1494: return true; On 2016/03/28 22:00:28, ncarter wrote: > On 2016/03/25 19:47:12, Charlie Reis wrote: > > Maybe this overall block would be easier to read as: > > > > if (...has last successful url...) { > > if (...IsSameWebSite...) > > return true; > > return (...origin check...); > > } > > > > return (...IsSameWebSite...); > > I've rearranged the code, hopefully it's better now. I also put in a TODO with > your name on it about eliminating the need for the GetSiteURL() fallback. Acknowledged. https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... File content/browser/isolate_top_document_browsertest.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/isolat... content/browser/isolate_top_document_browsertest.cc:280: // c.com popup should remain where it was, in the subframe process. On 2016/03/28 22:00:29, ncarter wrote: > On 2016/03/25 19:47:12, Charlie Reis wrote: > > Is there a reason to check this? We haven't changed anything on the popup. > > Yes. Proxies could have been created. Acknowledged. https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/1797363002/diff/180001/content/browser/site_i... content/browser/site_instance_impl.h:164: size_t active_frame_count_ = 0; On 2016/03/28 22:00:29, ncarter wrote: > On 2016/03/25 19:47:13, Charlie Reis wrote: > > Style question: Is this a new practice that we're moving to in general? I > > haven't seen inline initialization in other code yet, and I kind of like > knowing > > where to look (i.e., the constructor(s)). I won't object if this style has > > already been discussed, though. > > It's been allowed for over a year: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 > > (It's also on http://chromium-cpp.appspot.com/ as "non-static Non-Static Class > Member Initializer"). Ah, interesting. Thanks for letting me know. > Personally I prefer it if it's used consistently within a class, and it's > particularly beneficial when there is more than one ctor. But I would be happy > to retract this part of the change if you want. It didn't occur to me that it > would be controversial. Let's back it out of this CL, anyway, just to avoid derailing the conversation. Maybe we can do it separately, but this seems like one of the cases people weren't happy with on the discussion list: initializers split between here and in the constructor, requiring you to look in both places. (That seems a little harder to read and harder to verify that all of the members get initialized, since it's easy to assume that anything missing must be handled in the other file.) https://codereview.chromium.org/1797363002/diff/180001/content/common/site_is... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/1797363002/diff/180001/content/common/site_is... content/common/site_isolation_policy.cc:32: // --site-per-process trumps --isolate-top-document. On 2016/03/28 22:00:29, ncarter wrote: > On 2016/03/25 19:47:13, Charlie Reis wrote: > > nit: --top-document-isolation > > > > This reminds me-- is there an easy way to run try jobs with custom command > line > > flags? It'd be good to run the tests with only --isolate-extensions (to make > > sure we don't regress) and with both --isolate-extensions and > > --top-document-isolation (to make sure things work when a TDI user is in our > > Finch trial). > > I'm not sure about the tryjobs, but I was planning on looking at that soon by > writing some chrome browser tests that set both flags. This is on my tracking > list. Sounds good. We'll also be running your test class with --site-per-process (on the Linux FYI bot) and --isolate-extensions (on the Win FYI bot), so we will have some coverage. https://codereview.chromium.org/1797363002/diff/240001/content/browser/top_do... File content/browser/top_document_isolation_browsertest.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/top_do... content/browser/top_document_isolation_browsertest.cc:371: // Browser-initiated navigation to a.com. nit: Blank line before.
dcheng/creis: all done brettw: could you approve for chrome/ ? https://codereview.chromium.org/1797363002/diff/240001/content/browser/browsi... File content/browser/browsing_instance.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/browsi... content/browser/browsing_instance.cc:63: DCHECK(static_cast<SiteInstanceImpl*>(site_instance) On 2016/03/29 16:28:33, ncarter wrote: > On 2016/03/29 08:09:16, dcheng wrote: > > Unrelated to this CL, but is there a reason RegisterSiteInstance() and > > UnregisterSiteInstance() don't just take a SiteInstanceImpl? It seems weird to > > always pass in SiteInstanceImpls as SiteInstance and then downcast back to > > SiteInstanceImpl. > > Done locally, will upload with other reviewer fixes. Done. https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1186: // different than our current SiteInstance), so that it is ref counted. On 2016/03/29 17:06:59, dcheng wrote: > On 2016/03/29 at 16:28:33, ncarter wrote: > > On 2016/03/29 08:09:16, dcheng wrote: > > > Perhaps this function should just return a scoped_refptr. It's only called > in > > > two places that both immediately store the returned value into a > scoped_refptr > > > anyway, and it makes it easier to not footgun ourselves in the future. > > > > I agree, though obviously it doesn't belong in this CL. > > > > There's actually a longstanding TODO about this, dating back to the initial > check-in: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > > > I think we can do that with some c++11 wizardry right? > > scoped_refptr is already movable, so I think we can just go ahead and do that > already. I'll prepare a patch for it… as soon as my machine cert finishes > propagating. Acknowledged. https://codereview.chromium.org/1797363002/diff/240001/content/browser/top_do... File content/browser/top_document_isolation_browsertest.cc (right): https://codereview.chromium.org/1797363002/diff/240001/content/browser/top_do... content/browser/top_document_isolation_browsertest.cc:371: // Browser-initiated navigation to a.com. On 2016/03/29 17:17:13, Charlie Reis wrote: > nit: Blank line before. Done.
nick@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for histograms.xml brettw -- I goofed, and don't actually need your approval (but would, of course, welcome your feedback if you have any)
Have you considered defining this using a base::Feature? Among other advantages, it allows you to later do a server-side experiment enabling this without any additional client code. See go/feature-api-announcement.
On 2016/03/30 15:31:13, Alexei Svitkine wrote: > Have you considered defining this using a base::Feature? Among other advantages, > it allows you to later do a server-side experiment enabling this without any > additional client code. See go/feature-api-announcement. I hadn't considered it -- should I? Would histograms.xml look any different if I used base::Feature? I'm expecting that our experiments will need to be custom (there's an existing isolate-extensions experiment, and they're not orthogonal, so when we roll this out it'll be as new groups in that existing trial).
lgtm
Ah, in that case it's fine for it to be separate since it will be a variation of an existing trial. On Wed, Mar 30, 2016 at 1:32 PM, <nick@chromium.org> wrote: > On 2016/03/30 15:31:13, Alexei Svitkine wrote: > > Have you considered defining this using a base::Feature? Among other > advantages, > > it allows you to later do a server-side experiment enabling this without > any > > additional client code. See go/feature-api-announcement > <https://goto.google.com/feature-api-announcement>. > > I hadn't considered it -- should I? Would histograms.xml look any > different if I > used base::Feature? > > I'm expecting that our experiments will need to be custom (there's an > existing > isolate-extensions experiment, and they're not orthogonal, so when we roll > this > out it'll be as new groups in that existing trial). > > https://codereview.chromium.org/1797363002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1797363002/#ps300001 (title: "Whitespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797363002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797363002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_site_isolation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1797363002/#ps320001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797363002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797363002/320001
The CQ bit was unchecked by nick@chromium.org
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1797363002/#ps340001 (title: "scoped_refptr in GetDefaultSubframeSiteInstance")
The CQ bit was unchecked by nick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797363002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797363002/340001
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1797363002/#ps380001 (title: "Remove lame comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797363002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797363002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1797363002/#ps400001 (title: "SiteInstanceImpl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797363002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797363002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1797363002/#ps420001 (title: "Suppress tests under --site-per-process")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797363002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797363002/420001
I've suppressed the browsertests when --site-per-process is enabled, and put this back in the CQ. Feel free to take another pass to see my various rebase / site isolation trybot fixes.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797363002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797363002/420001
Message was sent while issue was closed.
Description was changed from ========== Introduce --top-document-isolation mode, a new process model wherein the top document is process separated from its subframes when possible, but the subframes are all housed in a single renderer process. Currently this mode only shares the subframe process within a browsing instance: usually, that will mean sharing the subframe process within a tab. In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce --top-document-isolation mode, a new process model wherein the top document is process separated from its subframes when possible, but the subframes are all housed in a single renderer process. Currently this mode only shares the subframe process within a browsing instance: usually, that will mean sharing the subframe process within a tab. In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Introduce --top-document-isolation mode, a new process model wherein the top document is process separated from its subframes when possible, but the subframes are all housed in a single renderer process. Currently this mode only shares the subframe process within a browsing instance: usually, that will mean sharing the subframe process within a tab. In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce --top-document-isolation mode, a new process model wherein the top document is process separated from its subframes when possible, but the subframes are all housed in a single renderer process. Currently this mode only shares the subframe process within a browsing instance: usually, that will mean sharing the subframe process within a tab. In follow up patches we can extend the sharing so that we might share subframe RenderProcessHosts among unrelated BrowsingInstances, when legal. BUG=594217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/d5bbd0b067cf0c1c8e40c7b5c2d1a1a39c0db10f Cr-Commit-Position: refs/heads/master@{#384364} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/d5bbd0b067cf0c1c8e40c7b5c2d1a1a39c0db10f Cr-Commit-Position: refs/heads/master@{#384364} |
