commit 33e05cb7b3af9dcadddb78e703f19c49f6664218
author Alex Moshchuk <alexmos@chromium.org> Sat May 20 00:12:12 2023
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Sat May 20 00:12:12 2023
tree 46c09203070660f7d5aca9acc8a242864be9fa6c
parent 0a045060ae37533acf4a33375ce76c8f3ed5460b

Remove SiteInstanceDeleting usage outside of tests.

Currently, the SiteInstance destructor notifies the embedder that it's
about to go away via ContentBrowserClient::SiteInstanceDeleting().
Due to some recent security bugs and a "complicated" relationship
between SiteInstance and BrowserContext destruction, it is desirable
to simplify SiteInstance destruction and, in particular, avoid running
arbitrary embedder code triggered by the destructor.  Outside of
tests, only the extension code uses SiteInstanceDeleting today to
remove entries from the extensions ProcessMap, and, fortunately, it
appears that this is not actually needed:
ExtensionService::RenderProcessHostDestroyed() already removes all
appropriate ProcessMap entries when an extension renderer process gets
destroyed (using RemoveAllFromProcess()), and there appears to be no
use for tracking when individual extension SiteInstance IDs are added
or removed.

Therefore, this CL removes the use of SiteInstanceDeleting() in the
extension code and changes ProcessMap to track a set of <extension_id,
process_id> rather than <extension_id, process_id, site_instance_id>.
Entries are removed from this set in
ExtensionService::RenderProcessHostDestroyed() (which should usually
run before the SiteInstance destructor anyway).  They are still added
by SiteInstanceGotProcess() - this could also eventually be switched
away from SiteInstance observation to process observation via
something like content::RenderProcessHostCreationObserver(), but we'd
need to look for all processes with ProcessLocks that correspond to
extensions, including hosted apps (so, we'd need to look at both site
and lock URLs), and this ProcessLock information isn't yet exposed
outside of //content.

Note that originally, site_instance_id was added to ProcessMap in
https://crrev.com/112921 for https://crbug.com/105328.  It appears
that the DCHECK that used to fire when adding a second entry with the
same process ID for the same extension is no longer there, and this
works just fine (and is covered in tests).  Also, some of the
assumptions are now simpler due to strict extension isolation, which
guarantees at most one extension being associated with each renderer
process.

After this change, ContentBrowserClient::SiteInstanceDeleting() is
only used by a few unit tests inside //content.  These will be cleaned
up in a followup CL.

In review, we discovered one slight behavioral change in a rare corner
case. Namely, if two hosted apps cover different paths on the *same*
origin, and they are opened in unrelated tabs, and we're over the
process limit, the two apps will share a process today (regardless of
this CL).  Then, if hosted app 1's tab is closed, its <app1_id,
process_id> entry in ProcessMap would remain until the hosted app 2 is
also closed and their shared process actually exits.  Previously, the
corresponding <app1_id, process_id, site_instance1_id> entry would be
removed when app 1 is closed.  This implies being a bit more
conservative but still safe (i.e., the process is considered to
contain app1 slightly longer than it actually does).

Change-Id: I2b847a80e2a57523af19ef2bcd04a1070cdad573
Bug: 1444204, 1444438
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4521894
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1146796}