(Adding SiteIsolation component mostly for visibility, not that it's a Site Isolation bug.)

SiteInstance sounds like the right abstraction to use here (all frames/windows that are same-site and have window references to each other).  BrowsingInstance includes cross-site frames/windows, and it's not clear to me that you'd need those to maintain the same Lifecycle state.

We're planning to ship Proactive Tab Discarding and Freezing in M69, so I'd suggest updating the discard/freezing logic to protect tabs that share the same browsing instance (i.e. add something like "if (web_contents()->SiteInstanc()->GetRelatedActiveContentsCount() > 1) return kDontDiscard;"). 

For M70 (or later) we'll see if we can safely restrict this to sites that share the same SiteInstance only (BrowsingInstance might be too broad), but it sounds like the safest approach concerning that there's only 2 days left before the M69 branch date.

Re: #2 - using BrowsingInstance safeguards against cases like login which can be cross-origin, so it's safer than SiteInstance alone.
That said, it likely makes sense to start with SiteInstance and later add BrowsingInstance only if we find breakages -- given the current desktop freezing logic it seems unlikely that we will break the login case.

Currently Proactive Tab Discarding and Tab Freezing are done on a per-tab basis, which means that we need to protect tabs that can talk to each others from any kind of intervention (until we add a notion of Tab groups to TabManager).

So what we want to do is:
For each tab:
  For each frame in this tab:
    Check if there's another tab that contains a frame sharing the same SiteInstance as this frame.
    If so, do not discard or freeze this tab.

This will imply doing some bookkeeping in TabLifecycleUnitSource and TabLifecycleUnit if we want this to be efficient. 

It looks like this isn't such a common case, 2 tabs that embed an iframe loaded with the same origin (e.g. doubleclick) might share the same process but won't share the same SiteInstance unless one page has directly opened the other (IIUC).

SiteInstance::GetRelatedActiveContentsCount [1] also sounds interesting but IIUC it's not necessarily per-tab (2 WCs can share the same SiteInstance within the same tab)


[1] https://cs.chromium.org/chromium/src/content/public/browser/site_instance.h?l=127&rcl=8bedb092995ef32665ffc262fa7cb91774ac5e4b

Comment 5:
> SiteInstance::GetRelatedActiveContentsCount [1] also sounds interesting but IIUC it's not necessarily per-tab (2 WCs can share the same SiteInstance within the same tab)

That's not quite right-- WebContents and tab are essentially the same, so you can't have "2 WCs ... within the same tab" (unless you're talking about a corner case like GuestView).

However, I was wrong about recommending GetRelatedActiveContentsCount(), since that actually returns whether there are other tabs in the same BrowsingInstance as the current SiteInstance.  Maybe it makes sense to track a similar GetActiveContentsCount() on SiteInstance, so that we know how many tabs it appears in.  Then you can iterate over the SiteInstances in a given tab and skip it if any of them appear in more than one tab.

Sorry, I meant "2 frames sharing the same SiteInstance within the same tab" :)

Adding a GetActiveContentsCount() method to SiteInstance sounds good, I'll do this.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94d6d3941e6a172f8254d168be15163dee6e1c05

commit 94d6d3941e6a172f8254d168be15163dee6e1c05
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Fri Jul 20 05:29:44 2018

Prevent a tab from being discarded if it shares its BrowsingInstance

This CL add a feature parameter to the Proactive Tab Discarding and
Freezing experiment to prevent discarding tabs that share their browsing
instance with another tab. This might be too agressive, in practice we
might just want to consider tabs under the same SiteInstance but this is
not an information that we can access yet.

Check this page for more details about the SiteInstance/BrowsingInstance
concepts: https://www.chromium.org/developers/design-documents/process-models#Implementation_Notes

UKM privacy document has been updated here (pending approval): https://docs.google.com/a/google.com/document/d/1BNQ5nLOtPuwP7oxr9r-XKNKr5iObXEiA_69WXAvuYAo/edit?disco=AAAABzM-vE0

Bug: 775644,  862203 
Change-Id: Iaf5107db1d68aa4a0155ecbae7283f487c23bb7c
Reviewed-on: https://chromium-review.googlesource.com/1144188
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576789}
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/chrome/browser/resource_coordinator/decision_details.cc
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/chrome/browser/resource_coordinator/decision_details.h
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/chrome/browser/resource_coordinator/tab_lifecycle_unit_unittest.cc
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/chrome/browser/resource_coordinator/tab_manager_features.cc
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/chrome/browser/resource_coordinator/tab_manager_features.h
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/chrome/browser/resource_coordinator/tab_manager_features_unittest.cc
[modify] https://crrev.com/94d6d3941e6a172f8254d168be15163dee6e1c05/tools/metrics/ukm/ukm.xml

I've added a flag to prevent freezing/discarding the tabs sharing the same BrowsingInstance for now (as it'll require more work to be able to find tabs sharing a SiteInstance), this might be too conservative but could prevent us from having to disable Proactive Tab Discarding entirely in M69. This is disabled by default, I'm still waiting on a privacy review for the UKM associated with this before enabling it (the UKMs will tell us if this is a frequent reason to not discard).

Adding the Merge-Request label to merge https://chromium-review.googlesource.com/1144188 into M69.

Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4dc421aedb46253a98832e534f02eb5907c1d75b

commit 4dc421aedb46253a98832e534f02eb5907c1d75b
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Wed Jul 25 19:21:56 2018

Prevent a tab from being discarded if it shares its BrowsingInstance

This CL add a feature parameter to the Proactive Tab Discarding and
Freezing experiment to prevent discarding tabs that share their browsing
instance with another tab. This might be too agressive, in practice we
might just want to consider tabs under the same SiteInstance but this is
not an information that we can access yet.

Check this page for more details about the SiteInstance/BrowsingInstance
concepts: https://www.chromium.org/developers/design-documents/process-models#Implementation_Notes

UKM privacy document has been updated here (pending approval): https://docs.google.com/a/google.com/document/d/1BNQ5nLOtPuwP7oxr9r-XKNKr5iObXEiA_69WXAvuYAo/edit?disco=AAAABzM-vE0

Bug: 775644,  862203 
Change-Id: Iaf5107db1d68aa4a0155ecbae7283f487c23bb7c
Reviewed-on: https://chromium-review.googlesource.com/1144188
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#576789}(cherry picked from commit 94d6d3941e6a172f8254d168be15163dee6e1c05)
Reviewed-on: https://chromium-review.googlesource.com/1150261
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#85}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/chrome/browser/resource_coordinator/decision_details.cc
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/chrome/browser/resource_coordinator/decision_details.h
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/chrome/browser/resource_coordinator/tab_lifecycle_unit_unittest.cc
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/chrome/browser/resource_coordinator/tab_manager_features.cc
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/chrome/browser/resource_coordinator/tab_manager_features.h
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/chrome/browser/resource_coordinator/tab_manager_features_unittest.cc
[modify] https://crrev.com/4dc421aedb46253a98832e534f02eb5907c1d75b/tools/metrics/ukm/ukm.xml

Hi, is there more to do on this bug? I'm pinging it only because
it is on the list of bugs that are considered priorities for gsuite.

I still need to work on preventing interventions on site that share the same SiteInstance, currently we're doing this at the BrowsingInstance level but it's not clear if it's ideal.

There's also some work to be done on in TabManager to support changing the state of several tabs at the same time (e.g. freeze all tabs that share the same SiteInstance at once).

I think that the gsuite concerns have been addressed (both by the BrowsingInstance stuff and by adding them to the intervention policy database).

To clarify comment #16: We're doing this at the BrowsingInstance level, which means we're being overly cautious. That is, all of gsuite's concerns should be currently addressed.

Moving forward we want to do 2 things:

- Instead of not applying lifecycles to all tabs in a BrowsingInstance with more than one tab, we instead want to treat them in lockstop (ie, only freeze if they're all in bg)
- Instead of applying the grouping logic at the level of BrowsingInstance we want to move to using SiteInstance.

Even with the above less conservative refinements, gSuite's concerns should remain covered.

I'd consider this Fixed for now. Seb?

Yep, this is fixed.