Enable spare RenderProcessHost to be preinitialized.

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 3f18fa9012d39be745adae2d39e679720fe88a77..4f60035532db041a540fbe4efbeca6b20a18d748 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -466,6 +466,109 @@ class SessionStorageHolder : public base::SupportsUserData::Data {
 const void* const kDefaultSubframeProcessHostHolderKey =
     &kDefaultSubframeProcessHostHolderKey;

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

nit: Please keep this and the DefaultSubframeProcessHostHolder class next to
each other.

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

Whoops, I assumed it was for the preceeding class.

 
+// This class manages spare RenderProcessHosts.

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

Please elaborate.  :)  Is this one per BrowserContext?  At most one total?  (The
current comment makes it sound like there could be multiple.)  What API creates
it, and what considerations should you have before creating it?  What happens if
you try to create one in a different BrowserContext when there's an existing
one?

(Maybe some of this discussion can belong on the content/public API, but some is
worth having here.)

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

Done.

+class SpareRenderProcessHostManager : public RenderProcessHostObserver {
+ public:
+  SpareRenderProcessHostManager() {}
+
+  void WarmupSpareRenderProcessHost(BrowserContext* browser_context) {

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

This should return early if we're over the process limit, right?

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

Yes. See also your comment about low-end devices.

+    StoragePartitionImpl* current_partition =
+        static_cast<StoragePartitionImpl*>(
+            BrowserContext::GetStoragePartition(browser_context, nullptr));

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

Do we only allow spare processes in the default StoragePartition?  That seems
reasonable, but we should document that if so.

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

Done, in the implementation notes above.

+
+    if (spare_render_process_host_ &&
+        matching_browser_context_ == browser_context &&
+        matching_storage_partition_ == current_partition) {
+      return;  // Nothing to warm up.
+    }
+
+    CleanupSpareRenderProcessHost();
+
+    matching_browser_context_ = browser_context;
+    matching_storage_partition_ = current_partition;
+
+    spare_render_process_host_ = RenderProcessHostImpl::CreateRenderProcessHost(
+        browser_context, current_partition, false /* is_for_guests_only */);

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

If we can't use spare processes for is_for_guests_only==true, we should also
document that.

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

Done.

+    spare_render_process_host_->AddObserver(this);
+    spare_render_process_host_->Init();
+  }
+
+  RenderProcessHost* MaybeTakeSpareRenderProcessHost(
+      const BrowserContext* browser_context,
+      const StoragePartition* partition,
+      bool is_for_guests_only) {
+    if (!spare_render_process_host_ ||
+        browser_context != matching_browser_context_ ||
+        partition != matching_storage_partition_ || is_for_guests_only) {
+      // As a new RenderProcessHost will almost certainly be created, we cleanup
+      // the non-matching one so as not to waste resources.
+      CleanupSpareRenderProcessHost();
+      return nullptr;
+    }
+
+    RenderProcessHost* rph = spare_render_process_host_;
+    DropSpareRenderProcessHost(spare_render_process_host_);
+    return rph;
+  }
+
+  void OnStoragePartitionShutdown(StoragePartition* partition) {
+    if (partition == matching_storage_partition_) {

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

nit: No braces needed.

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

Done.

+      CleanupSpareRenderProcessHost();
+    }
+  }
+
+  RenderProcessHost* spare_render_process_host() {
+    return spare_render_process_host_;
+  }
+
+ private:
+  // RenderProcessHostObserver
+  void RenderProcessWillExit(RenderProcessHost* host) override {
+    DropSpareRenderProcessHost(host);
+  }
+
+  void RenderProcessExited(RenderProcessHost* host,
+                           base::TerminationStatus unused_status,
+                           int unused_exit_code) override {
+    DropSpareRenderProcessHost(host);
+  }
+
+  void RenderProcessHostDestroyed(RenderProcessHost* host) override {
+    DropSpareRenderProcessHost(host);
+  }
+
+  void CleanupSpareRenderProcessHost() {
+    if (spare_render_process_host_) {
+      spare_render_process_host_->Cleanup();

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

Is this the right method to be calling?  It looks like it can early exit and do
nothing in a bunch of cases, and it may be more about bookkeeping.  I would have
expected something more like Shutdown, but I'm concerned about the race that one
describes where it shouldn't be called before RenderProcessReady.

Can we figure out if this is always effective?

Also, can we add a check to verify that we aren't taking down other things with
us when we do this?  We've had bugs in the past where a process gets reused for
another tab just as it's being taken down from a different tab, and the new tab
ends up with a sad tab.  We want to verify that nothing ended up in this process
in the meantime.

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

It's possible that Shutdown is better. My reasoning was that, in addition to
only being able to call it after the connection to the child process is up, as
best as I could determine, Shutdown() asks the child process to terminate, which
eventually will trigger notifications about the termination. This means there
seems to be more chances for a race where the renderer gets used while being
shutdown.

Cleanup() calls RenderProcessExited and/or RenderProcessHostDestroyed on all
observers, so it seems to reduce that possibility.

Also, I was under the impressions that the RenderFrameHostImpl was responsible
for assigning processes, in which case the spare renderer will be managed
correctly (eg GetProcessHostForSiteInstance).

But please correct me if I'm wrong! Also any suggestions for tests would be
welcome. For example, I test that things work correctly if the spare renderer
process is killed before the spare renderer is used, but I'm sure there are a
lot of corner cases hiding.

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

Sorry, I missed this in the last round.  Looking closer, Cleanup() seems ok to
use for this, but I still have a question about it.  Do we expect it to be ok
for other features to use the spare RPH while it's still considered the spare
RPH?

That seems to be possible today.  GetExistingProcessHost() (which we call just
before using the spare RPH below) can return the spare RPH, since it selects
something randomly from g_all_hosts.  This means we might have tabs already
using the spare RPH when we decide to use or discard the spare RPH.

Is this intentional?  It looks like Cleanup() will become a no-op in that case,
because listeners.IsEmpty() is false.  On the plus side, that means we won't
kill the process if we get here and it's already in use.  And maybe it's not a
big deal if it happens?  Still, it's really subtle and a bit surprising, so we
should clearly document (and hopefully test it) if we want it to be supported.

If it's not intentional, we should exclude the spare RPH from
GetExistingProcessHost() and verify that no one else is using the spare RPH when
we get here.  I think that means checking:
 - listeners_.IsEmpty() should be true
 - GetWorkerRefCount() should be 0
 - pending_views_ should be 0
Is there a typo here?  (Maybe you meant RenderProcessHostImpl instead of
RenderFrameHostImpl?)

RPHI is responsible for process assignment, but I'm not sure we realized the
issue with GetExistingProcessHost I just noted above.
If we want to test the process reuse case, you can add a
--renderer-process-limit=N command line flag.  Maybe it's possible we could set
it to 1 and create a spare RPH before anything else (while we're still "below"
the process limit), then try to create a normal tab?  I would expect that would
get returned from GetExistingProcessHost but leave that RPH registered as the
spare (possibly incorrectly).

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

Good find! I don't think that's correct, GetExistingProcessHost should update
the state of the spare renderer. I think it's correct for GetExistingProcessHost
to use the spare renderer if it's available; if it didn't end up being used as
intended immediately after the warmup call, there's no reason not to use it as
soon as possible.
Yes, sorry, I meant RPHI.
Thanks, done. The test is necessarily a little indirect as there's no
test-visible difference to getting an RPH from GetExistingProcessHost or from
MaybeTakeSpareRPH. Also note that an initial RPH is created during the Shell
startup, so I can't do exactly as you suggest. The added test should cover the
code paths we're interested in. 

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

Thanks!  I added a few suggestions to improve these limitations in the test.

+      DropSpareRenderProcessHost(spare_render_process_host_);
+    }
+  }
+
+  // Remove |host| as a possible spare renderer. Does not shut it down cleanly,

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

nit: semicolon rather than comma

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

Done.

+  // the assumption is that the host has been shutdown somewhere else and has

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

nit: s/has/is/

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

Done (well, s/has been/was/, which maybe was your intention?)

+  // notifying the SpareRenderProcessHostManager.
+  void DropSpareRenderProcessHost(RenderProcessHost* host) {
+    if (host && host == spare_render_process_host_) {
+      spare_render_process_host_->RemoveObserver(this);
+      spare_render_process_host_ = nullptr;
+    }
+  }
+
+  // This is a bare pointer, because RenderProcessHost manages the lifetime of
+  // all its instances; see g_all_hosts, above.
+  RenderProcessHost* spare_render_process_host_;
+
+  // Used only to check if a creation request matches the
+  // spare, and not accessed.

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

nit: Rewrap

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

Done.

+  const BrowserContext* matching_browser_context_ = nullptr;
+  const StoragePartition* matching_storage_partition_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(SpareRenderProcessHostManager);
+};
+
+base::LazyInstance<SpareRenderProcessHostManager>::Leaky
+    g_spare_render_process_host_manager = LAZY_INSTANCE_INITIALIZER;
+
 class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
                                          public RenderProcessHostObserver {
  public:
@@ -484,7 +587,7 @@ class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
     // Is this the default storage partition? If it isn't, then just give it its
     // own non-shared process.
     if (partition != default_partition || is_for_guests_only) {
-      RenderProcessHostImpl* host = new RenderProcessHostImpl(
+      RenderProcessHost* host = RenderProcessHostImpl::CreateRenderProcessHost(
           browser_context_, static_cast<StoragePartitionImpl*>(partition),
           is_for_guests_only);
       host->SetIsNeverSuitableForReuse();
@@ -496,7 +599,12 @@ class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
     if (host_)
       return host_;
 
-    host_ = new RenderProcessHostImpl(
+    host_ =
+        g_spare_render_process_host_manager.Get()
+            .MaybeTakeSpareRenderProcessHost(browser_context_, partition,

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

Is there a reason not to do this within CreateRenderProcessHost itself?  Seems
like any call to that method should probably return the spare RPH if there is
one.

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

Yes, CreateRenderProcessHost is used in WarmupSpareRenderProcessHost. Making it
re-entrant seems overly complicated (not so much in the current implementation,
but this feels like the sort of thing that produces bugs down the road).

The idea is the CreateRenderProcessHost wraps up just the factory indirection
for testing and is really the same as just calling the RenderProcessHostImpl
constructor.

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

I see.  The trouble right now is that we're duplicating the internals of using
the spare RPH, and it's inconsistent with the public comment describing
WarmupSpareRenderProcessHost.

I would suggest either having a CreateRenderProcessHostInternal that does the
factory/constructor call and returning the spare from CreateRenderProcessHost
(as a way to make it transparent), or having a separate
CreateOrUseSpareRenderProcessHost that avoids the code duplication and that
people can use if they need an additional case.  The latter could be a private
method until/unless it's needed externally, but we'd need to update the public
comment if we go that route.

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

Note that your suggestion could change the behavior of the RPH creation at line
590, if the spare renderer's relation to the default partition changed. That
seems overly subtle.

I see something that might have been confusing: CreateRenderProcessHost is not
actually public (*); it was a mistake to mention it in render_process_host.h.
I've fixed the comment in render_process_host.h, is that more clear now?

(*) public in the content/public sense; it is public to
render_process_host_impl.h, only so that it can be used by the
anonymous-namespace classes.

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

FWIW, the default StoragePartition can't change, but I agree with your point
that we may not want to use the spare RPH in all cases.
I still think we should have a CreateOrUseSpareRenderProcessHost helper
function, given the duplication.  Especially so, since I just noticed there's a
bug in this copy-- we're calling CreateRenderProcessHost on line 607 below
whether or not we used the spare RPH here on line 602.  :)

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

:P point taken...

+                                             false /* is for guests only */);
+
+    host_ = RenderProcessHostImpl::CreateRenderProcessHost(
         browser_context_, static_cast<StoragePartitionImpl*>(partition),
         false /* for guests only */);
     host_->SetIsNeverSuitableForReuse();
@@ -517,7 +625,7 @@ class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
 
   // The default subframe render process used for the default storage partition
   // of this BrowserContext.
-  RenderProcessHostImpl* host_ = nullptr;
+  RenderProcessHost* host_ = nullptr;
 };
 
 void CreateMemoryCoordinatorHandle(
@@ -915,6 +1023,20 @@ void RenderProcessHost::SetMaxRendererProcessCount(size_t count) {
   g_max_renderer_count_override = count;
 }
 
+// static
+RenderProcessHost* RenderProcessHostImpl::CreateRenderProcessHost(
+    BrowserContext* browser_context,
+    StoragePartitionImpl* storage_partition_impl,
+    bool is_for_guests_only) {
+  if (g_render_process_host_factory_) {
+    return g_render_process_host_factory_->CreateRenderProcessHost(
+        browser_context);
+  }
+
+  return new RenderProcessHostImpl(browser_context, storage_partition_impl,
+                                   is_for_guests_only);
+}
+
 RenderProcessHostImpl::RenderProcessHostImpl(
     BrowserContext* browser_context,
     StoragePartitionImpl* storage_partition_impl,
@@ -1986,6 +2108,19 @@ void RenderProcessHostImpl::RemoveExpectedNavigationToSite(
   tracker->DecrementSiteProcessCount(site_url, render_process_host->GetID());
 }
 
+// static
+void RenderProcessHostImpl::OnStoragePartitionShutdown(
+    StoragePartition* partition) {
+  g_spare_render_process_host_manager.Get().OnStoragePartitionShutdown(
+      partition);
+}
+
+// static
+RenderProcessHost*
+RenderProcessHostImpl::GetSpareRenderProcessHostForTesting() {
+  return g_spare_render_process_host_manager.Get().spare_render_process_host();
+}
+
 bool RenderProcessHostImpl::IsForGuestsOnly() const {
   return is_for_guests_only_;
 }
@@ -2924,6 +3059,13 @@ bool RenderProcessHostImpl::IsSuitableHost(RenderProcessHost* host,
   return GetContentClient()->browser()->IsSuitableHost(host, site_url);
 }
 
+// static
+void RenderProcessHost::WarmupSpareRenderProcessHost(
+    content::BrowserContext* browser_context) {
+  g_spare_render_process_host_manager.Get().WarmupSpareRenderProcessHost(
+      browser_context);
+}
+
 // static
 bool RenderProcessHost::run_renderer_in_process() {
   return g_run_renderer_in_process_;
@@ -3123,18 +3265,20 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
     render_process_host = GetExistingProcessHost(browser_context, site_url);
   }
 
+  // Try to use a spare process if one is found.

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

nit: Start with "Before creating a new process,"

This will matter as other people insert additional cases into this method (e.g.,
https://chromium-review.googlesource.com/c/535220/), to make sure that we keep
this case near the end.

(Note: Or we could move this within CreateRenderProcessHost, as noted above.)

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

Done.

+  StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(
+      BrowserContext::GetStoragePartition(browser_context, site_instance));
+  if (!render_process_host) {
+    render_process_host =
+        g_spare_render_process_host_manager.Get()
+            .MaybeTakeSpareRenderProcessHost(browser_context, partition,
+                                             is_for_guests_only);
+  }
+
   // Otherwise (or if that fails), create a new one.
   if (!render_process_host) {
-    if (g_render_process_host_factory_) {
-      render_process_host =
-          g_render_process_host_factory_->CreateRenderProcessHost(
-              browser_context);
-    } else {
-      StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(
-          BrowserContext::GetStoragePartition(browser_context, site_instance));
-      render_process_host = new RenderProcessHostImpl(
-          browser_context, partition, is_for_guests_only);
-    }
+    render_process_host =
+        CreateRenderProcessHost(browser_context, partition, is_for_guests_only);
   }
   return render_process_host;

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

nit: Unrelated, but can you put a DCHECK(render_process_host) above the return
statement?  All the previous blocks might leave us with a null
render_process_host, but this last one should guarantee a new one.

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

Done.

 }