Enable spare RenderProcessHost to be preinitialized.

content/browser/storage_partition_impl.cc

Index: content/browser/storage_partition_impl.cc
diff --git a/content/browser/storage_partition_impl.cc b/content/browser/storage_partition_impl.cc
index 5c4270512f9614eafefc00b3d70723b7c9f8f3cd..aa7f8e742ec65ba92b5acfba425b47c9862c56be 100644
--- a/content/browser/storage_partition_impl.cc
+++ b/content/browser/storage_partition_impl.cc
@@ -22,6 +22,7 @@
 #include "content/browser/fileapi/browser_file_system_helper.h"
 #include "content/browser/gpu/shader_cache_factory.h"
 #include "content/browser/notifications/platform_notification_context_impl.h"
+#include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/common/dom_storage/dom_storage_types.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -401,6 +402,8 @@ StoragePartitionImpl::~StoragePartitionImpl() {
         base::Bind(&storage::DatabaseTracker::Shutdown, GetDatabaseTracker()));
   }
 
+  RenderProcessHostImpl::OnStoragePartitionShutdown(this);

[Charlie Reis, 2017/06/25 23:48:28]

Hmm.  This feels a bit like a layering violation / circular dependency, since
RPH depends on StoragePartition and StoragePartition doesn't know about RPH at
all.

I suppose I'm not eager to add an observer interface to StoragePartition,
though.  Are there other ways we can handle this?

[mattcary, 2017/06/26 14:45:07]

Not that I know of. The idea here is that if the profile behind the
StorageParition is deleted after a spare renderer is created, we don't want to
try to use that spare renderer. This is the only way we saw to get notified of
the StoragePartition shutdown absent an observer interface.

We could hoist OnStoragePartitionShutdown to the public interface, and have this
called from Profile deletion directly, but that seems like half a solution (this
really isn't a chrome-specific problem in general) as well as leaky as well.

At any rate, I'm happy to implement an alternative if there is one. This is the
best I could come up with.

[Charlie Reis, 2017/06/26 21:22:50]

If the profile is deleted while it still has RPHs, isn't that a bigger problem? 
Is that even possible?  I would expect that this is already addressed somewhere,
since it seems like we would have big use-after-free problems otherwise.

I assumed this change was about early StoragePartition deletion (e.g., for
non-default StoragePartitions).  Now that it's clear that we only create spare
RPHs for the default StoragePartition, I would imagine it would boil down to the
same thing-- the default StoragePartition shouldn't go away until the profile
goes away, and I'm surprised if that can happen while there's still RPHs using
it.
Yeah, I wouldn't recommend that route, but if we did, we could probably keep it
content-specific using BrowserContext, right?  (BrowserContext is the content
portion of Profile.)
Apart from unit tests, did you find a case where this was necessary in practice?
 For example, if a single Incognito window creates a spare RPH and you close the
Incognito window, do we have some existing code that will delete all the RPHs in
the BrowserContext as part of deleting the BrowserContext?  Or do we really
leave it stranded with a bad pointer?

If it's the latter, that seems like a bigger problem we should address at the
BrowserContext level.

[mattcary, 2017/06/28 13:14:38]

As far as I can tell, deleting a browser context will shutdown the RPHs
gracefully, eg,

https://cs.chromium.org/chromium/src/content/browser/browser_context.cc?rcl=8...

Cleanup() calls the observer that my SpareRPHManager is hooked into.

However, I'm not sure if StoragePartitions can be deleted in a different way. If
they're only stored as user data in a browser context, then from what I can see
it should only be necessary to make sure the right thing happens on destroying a
browser context.

Unfortunately, as far as I can tell, ShellBrowserContexts are only destroyed at
shutdown. So I don't think I can do any content shell tests for this. If you
have any alternatives to suggest, let me know...

[Charlie Reis, 2017/06/28 22:31:35]

That's interesting about BrowserContext::NotifyWillBeDestroyed.  It's making an
effort to clean up "extra" RPHs (those for ServiceWorkers and shared workers),
but I guess we have an assumption that all the normal RPHs for tabs (etc) will
be gone by then.

As I think you were saying, I'm going to propose that we tell RenderProcessHost
to clean up the spare RPH for a BrowserContext from
BrowserContext::NotifyWillBeDestroyed instead of from StoragePartition.

This would be good on two fronts.  First, it makes sure that the spare RPH goes
away if the BrowserContext goes away, regardless of StoragePartition behavior. 
(The default StoragePartition should go away at that time as well, but we don't
need to depend on that indirectly.)  Second, it avoids unnecessary calls from
non-default StoragePartition destruction, since those won't ever have a spare
RPH.  (It also avoids the RPH dependency in this class.)
That's too bad, but we might be able to get by without a test here if it doesn't
work out.  I thought about using CreateOffTheRecordBrowser() and then closing
the Shell it creates, but that doesn't seem to destroy the BrowserContext
either.  Maybe we could manually create a BrowserContext to pass to
Shell::CreateNewWindow and then later delete it?

I see you already removed the StoragePartition test?  That's ok if we switch to
BrowserContext::NotifyWillBeDestroyed.

[mattcary, 2017/06/29 12:57:09]

Actually, it's already done for us: NotifyWillBeDestroyed calls
ForceReleaseWorkerRefCounts which calls Cleanup() which notifies observers,
including my spare RPH Manager.
As far as I can tell, that doesn't work: the ShellBrowserContext has some weird
creation/destruction that mixes IO and UI thread processing and so can only be
done on startup/shutdown (eg, ShellBrowserContext::InitWhileIOAllowed).
Ack.

[Charlie Reis, 2017/06/29 21:09:16]

Oh, that's pretty indirect!  I'd worry about that breaking if we changed how
worker ref counts work.

Here's some things we could choose between to improve it:
1) Rename ForceReleaseWorkerRefCounts() to something like
CleanupExtraProcesses().
2) Add a comment at the call site saying it also clears the spare RPH (so people
think twice before changing/removing it).
3) Add a comment inside ForceReleaseWorkerRefCounts() above the Cleanup() call
saying that also clears the spare RPH, since it's unused.

We don't need to do all of them, but I think it's worth making the dependency
explicit in some way.
Ah, that's unfortunate.  Thanks for checking.

[mattcary, 2017/06/30 14:29:46]

Added the comment of (2) and (3).

+
   if (GetFileSystemContext())
     GetFileSystemContext()->Shutdown();