Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929113002:
Enable spare RenderProcessHost to be preinitialized. (Closed)
DescriptionEnable spare RenderProcessHost to be preinitialized.
Enables a preinitialization of an unbound RenderProcessHost, and refactors
RenderProcessHost creation to allow that to be used where appropriate.
BUG=730587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2929113002
Cr-Commit-Position: refs/heads/master@{#486372}
Committed: https://chromium.googlesource.com/chromium/src/+/28c8676d89927c00b9e64dfdb7681027d93a37df
Patch Set 1 #Patch Set 2 : remove race from test #
Total comments: 6
Patch Set 3 : comments #Patch Set 4 : GetProcesses should be in this CL. #Patch Set 5 : actually track storage shutdown #Patch Set 6 : check connection #
Total comments: 10
Patch Set 7 : comments #Patch Set 8 : Add WarmupManager hook #
Total comments: 4
Patch Set 9 : comments #Patch Set 10 : browsertests #
Total comments: 7
Patch Set 11 : comments #
Total comments: 1
Patch Set 12 : Omnibox hook #
Total comments: 56
Patch Set 13 : comments #
Total comments: 14
Patch Set 14 : comments #Patch Set 15 : comments #Patch Set 16 : remove unneeded include #
Total comments: 4
Patch Set 17 : comments #Patch Set 18 : tweaks #
Total comments: 14
Patch Set 19 : comments #Patch Set 20 : added comments #
Total comments: 15
Patch Set 21 : comments #Patch Set 22 : feature/bugfix #Patch Set 23 : rebase #
Total comments: 2
Patch Set 24 : isNTPPage/rebase #Patch Set 25 : git cl format added histograms.xml erroneously to the cl #Patch Set 26 : bugfix #Patch Set 27 : Change creation of storage partition to not break unittests with subtle threading issues #Messages
Total messages: 81 (28 generated)
lizeb@chromium.org changed reviewers: + lizeb@chromium.org
https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:486: spare_render_process_host_ = RenderProcessHostImpl::CreateRenderProcessHost( Wouldn't this leak the previous one in the case where spare_render_process_host_ was not null above? Actually, shouldn't it be a unique_ptr? https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3001: if (!g_spare_render_process_host_manager) { since this is a leaky singleton, what about a LazyInstance? We don't need it to be threadsafe though... https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3216: CreateRenderProcessHost(browser_context, partition, is_for_guests_only); Doesn't this bypass the factory?
https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:486: spare_render_process_host_ = RenderProcessHostImpl::CreateRenderProcessHost( On 2017/06/09 14:35:22, Benoit L wrote: > Wouldn't this leak the previous one in the case where spare_render_process_host_ > was not null above? > > Actually, shouldn't it be a unique_ptr? I don't think so. RPH seem to handle their own deletion; see the destructor: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro... There appears to be a global host list. Probably it can't be deleted arbitrarily as it needs to wait for cleanup IPCs from the renderer process? And indeed, in browsertests that assertion does fire (browsertests are complicated by other reasons and not included here). There may be something I'm not understanding, though, so any insight you have would be appreciated... https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3001: if (!g_spare_render_process_host_manager) { On 2017/06/09 14:35:22, Benoit L wrote: > since this is a leaky singleton, what about a LazyInstance? > We don't need it to be threadsafe though... Done https://codereview.chromium.org/2929113002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3216: CreateRenderProcessHost(browser_context, partition, is_for_guests_only); On 2017/06/09 14:35:22, Benoit L wrote: > Doesn't this bypass the factory? Nope, that's exactly why I added CreateRenderProcessHost.
I added handling for partition shutdown, it's sort of caveman, though. LMK.
mattcary@chromium.org changed reviewers: + clamy@chromium.org, falken@chromium.org
Camille and Matt--- Here is a start at the spare renderer idea (aka SW speed-up plan B). Does this look reasonable to you? Note there are several details yet to be worked out, in addition to adding browser test support. In particular I'm just checking the connection to see if the process has died before using the spare renderer, and I suspect there's a better way to do it as they way I'm doing it now is resisting having a browser test (although I may be holding the browser tests wrong).
mattcary@chromium.org changed reviewers: + creis@chromium.org
+creis as well on Camille's advice.
Thanks-- I'll try to give this a full review tomorrow, but would it be possible to include a caller outside content for the new public method? Or point to a CL that will use it? (We generally don't allow new public methods in the content module unless they have a caller, which helps understand how they will be used and helps avoids dead code like you're removing in your other CL.) :) Gotta run.
Looks good. Made some comments. https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:507: if (!spare_render_process_host_->HasConnection()) { I'm curious why this is needed. (I'm not really a render process host expert.) Does this signify the RPH crashed? Could the RPH just not be ready yet? Do we also want to check something like FastShutdownStarted()? https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:517: void OnStorageParitionShutdown(StoragePartition* partition) { typo: Partition https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2079: void RenderProcessHostImpl::OnStorageParitionShutdown( typo: Partition https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3229: BrowserContext::GetStoragePartition(browser_context, site_instance)); really naive question: I wonder if we have to somehow stop the render process from being discovered and used by some other thing (so it would no longer be a spare, unused render process). E,g, anyone could probably discover it via AllHostsIterator()? I guess everything that "claims" a render process comes through GetProcessHostForSiteInstance(). https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:368: // Use CreateRenderProcessHostImpl() instead of calling this constructor typo? CreateRenderProcessHost()
On 2017/06/14 23:51:55, Charlie Reis (slow) wrote: > Thanks-- I'll try to give this a full review tomorrow, but would it be possible > to include a caller outside content for the new public method? Or point to a CL > that will use it? > > (We generally don't allow new public methods in the content module unless they > have a caller, which helps understand how they will be used and helps avoids > dead code like you're removing in your other CL.) :) > > Gotta run. Thanks! It will be called from the omnibox. Where exactly to do that has been worked in on parallel, but I'll add it to this CL when I can.
https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:507: if (!spare_render_process_host_->HasConnection()) { On 2017/06/15 06:46:49, falken wrote: > I'm curious why this is needed. (I'm not really a render process host expert.) > Does this signify the RPH crashed? > > Could the RPH just not be ready yet? Do we also want to check something like > FastShutdownStarted()? I'm not sure it is needed. The case I'm worried about is if the spare renderer process is killed the warmup and this function. Any advice on the best way to handle that would be great, as I'm not personally familiar with the best way to hook into the RPH lifecycle (in particular, it seems that the RPHObserver isn't sufficient, although it may be necessary). I'll look into FastShutdownStarted. https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:517: void OnStorageParitionShutdown(StoragePartition* partition) { On 2017/06/15 06:46:49, falken wrote: > typo: Partition Done. https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2079: void RenderProcessHostImpl::OnStorageParitionShutdown( On 2017/06/15 06:46:49, falken wrote: > typo: Partition Done (and in all the other instances too). https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3229: BrowserContext::GetStoragePartition(browser_context, site_instance)); On 2017/06/15 06:46:49, falken wrote: > really naive question: I wonder if we have to somehow stop the render process > from being discovered and used by some other thing (so it would no longer be a > spare, unused render process). E,g, anyone could probably discover it via > AllHostsIterator()? > That's a good point. I just searched through the code, all uses of AllHostsIterator appear to be for updating hosts when events come though. So that seems to be okay (although I guess this might expose some bugs where the renderer is expected to be fully initialized with a web contents and such). > I guess everything that "claims" a render process comes through > GetProcessHostForSiteInstance(). Yes, that was what we understood. https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:368: // Use CreateRenderProcessHostImpl() instead of calling this constructor On 2017/06/15 06:46:49, falken wrote: > typo? CreateRenderProcessHost() Done.
On 2017/06/15 08:14:59, mattcary wrote: > On 2017/06/14 23:51:55, Charlie Reis (slow) wrote: > > Thanks-- I'll try to give this a full review tomorrow, but would it be > possible > > to include a caller outside content for the new public method? Or point to a > CL > > that will use it? > > > > (We generally don't allow new public methods in the content module unless they > > have a caller, which helps understand how they will be used and helps avoids > > dead code like you're removing in your other CL.) :) > > > > Gotta run. > > Thanks! > > It will be called from the omnibox. Where exactly to do that has been worked in > on parallel, but I'll add it to this CL when I can. I've uploaded the hook we'll likely use from the WarmupManager, so you can see why the public interface is needed. Still working on exactly where to call from the omnibox.
Thanks, looks good. A few comments. https://codereview.chromium.org/2929113002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:252: * Warms up a spare, empty renderer that may be used for subsequent navigations. Can this be named "RenderProcessHost"? That would hopefully lessen confusion between this and the other one. Also, could you discard the spare WebContents (if any) when this is called? To avoid avoid several empty renderers. https://codereview.chromium.org/2929113002/diff/140001/content/browser/render... File content/browser/renderer_host/render_process_host_unittest.cc (right): https://codereview.chromium.org/2929113002/diff/140001/content/browser/render... content/browser/renderer_host/render_process_host_unittest.cc:521: // TODO(mattcary): test storage partition shutdown Can you also test the case where the process dies?
https://codereview.chromium.org/2929113002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:252: * Warms up a spare, empty renderer that may be used for subsequent navigations. On 2017/06/15 22:17:11, Benoit L wrote: > Can this be named "RenderProcessHost"? > > That would hopefully lessen confusion between this and the other one. > Also, could you discard the spare WebContents (if any) when this is called? To > avoid avoid several empty renderers. Done. https://codereview.chromium.org/2929113002/diff/140001/content/browser/render... File content/browser/renderer_host/render_process_host_unittest.cc (right): https://codereview.chromium.org/2929113002/diff/140001/content/browser/render... content/browser/renderer_host/render_process_host_unittest.cc:521: // TODO(mattcary): test storage partition shutdown On 2017/06/15 22:17:11, Benoit L wrote: > Can you also test the case where the process dies? That seems to be hard to do in the unittests, but I think I've figured out how to hold the browser tests and will add those to this patch today.
> On 2017/06/15 22:17:11, Benoit L wrote: > > Can this be named "RenderProcessHost"? > > > > That would hopefully lessen confusion between this and the other one. > > Also, could you discard the spare WebContents (if any) when this is called? To > > avoid avoid several empty renderers. > > Done. > Note that there's currently no policy on when to destroy a spare RenderProcessHost. We didn't really discuss this; I'm not sure if having a timeout is appropriate although it's probably the most straightforward.
Added browsertests. I suspect there are more corner cases/race conditions around RPHs dying, but it wasn't clear how to exercise them in the browser test. Any hints or suggestions would be appreciated.
https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:150: int32_t spare_key = FirstRenderProcessHostKey(); nit: EXPECT_NE(0, spare_key); ? https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:167: int32_t spare_key = FirstRenderProcessHostKey(); Question: Actually, should we warmup a spare renderprocesshost at all for incognito? The potential to reuse it seems lower than for a regular profile. https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:511: return rph; Shouldn't we stop observing the RenderProcessHost lifecycle at the time we hand it over?
https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:150: int32_t spare_key = FirstRenderProcessHostKey(); On 2017/06/19 11:30:35, Benoit L wrote: > nit: > EXPECT_NE(0, spare_key); > ? Done. https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:167: int32_t spare_key = FirstRenderProcessHostKey(); On 2017/06/19 11:30:35, Benoit L wrote: > Question: Actually, should we warmup a spare renderprocesshost at all for > incognito? > > The potential to reuse it seems lower than for a regular profile. That's a good question, although shouldn't that be handled at the layer above this one? The caller has a profile and knows whether it's incognito or not, and so can make an informed decision. For example, the omnibox use case probably will work equally well for incognito as for cognito. https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:511: return rph; On 2017/06/19 11:30:35, Benoit L wrote: > Shouldn't we stop observing the RenderProcessHost lifecycle at the time we hand > it over? Good point, thanks.
Thanks for the response! Looks good to me (non-owner, of course). https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:167: int32_t spare_key = FirstRenderProcessHostKey(); On 2017/06/19 12:55:10, mattcary wrote: > On 2017/06/19 11:30:35, Benoit L wrote: > > Question: Actually, should we warmup a spare renderprocesshost at all for > > incognito? > > > > The potential to reuse it seems lower than for a regular profile. > > That's a good question, although shouldn't that be handled at the layer above > this one? The caller has a profile and knows whether it's incognito or not, and > so can make an informed decision. For example, the omnibox use case probably > will work equally well for incognito as for cognito. Acknowledged. https://codereview.chromium.org/2929113002/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:551: if (host && host == spare_render_process_host_) { nit: Since we are only observing the RPH we currently hold, shouldn't the first and second condition always be true? If so, can they be turned into DCHECK()s?
Sorry for the delay in the review! I spent most of today on the other ServiceWorker proposal in https://docs.google.com/document/d/1iT8xvo18-zmlsU2iHqH2RCIXrWoGJOIaZ6aDASubi.... I'll try to get to this before my trip to MTV tomorrow night.
Apologies that this took so long! Seems like a reasonable API to add-- some thoughts and questions below. https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:257: * This uses a different mechanism than createSpareWebContents, below, and is subject What is createSpareWebContents used for? Do you think this new spare RPH approach could be sufficient for its needs in the long term? I also notice that createSpareWebContents is a no-op on low-end devices. Should we be doing the same here? https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:158: WarmupManager.getInstance().createSpareRenderProcessHost(profile); Worth having a comment here saying how the spare process will be used (i.e., for either subsequent ServiceWorkers or navigations likely to result from the upcoming search?). https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:150: int32_t spare_key = FirstRenderProcessHostKey(); I don't understand why we need FirstRenderProcessHostKey(), since it's kind of indirect and could be affected by other factors. Can we use GetSpareRenderProcessHostForTesting() instead, and then compare RPHs directly? https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:467: &kDefaultSubframeProcessHostHolderKey; nit: Please keep this and the DefaultSubframeProcessHostHolder class next to each other. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:469: // This class manages spare RenderProcessHosts. 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.) https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:474: void WarmupSpareRenderProcessHost(BrowserContext* browser_context) { This should return early if we're over the process limit, right? https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:477: BrowserContext::GetStoragePartition(browser_context, nullptr)); Do we only allow spare processes in the default StoragePartition? That seems reasonable, but we should document that if so. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:491: browser_context, current_partition, false /* is_for_guests_only */); If we can't use spare processes for is_for_guests_only==true, we should also document that. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:515: if (partition == matching_storage_partition_) { nit: No braces needed. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:542: spare_render_process_host_->Cleanup(); 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:547: // Remove |host| as a possible spare renderer. Does not shut it down cleanly, nit: semicolon rather than comma https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:548: // the assumption is that the host has been shutdown somewhere else and has nit: s/has/is/ https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:562: // spare, and not accessed. nit: Rewrap https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:604: .MaybeTakeSpareRenderProcessHost(browser_context_, partition, 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3268: // Try to use a spare process if one is found. 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.) https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3283: return render_process_host; 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:340: static void OnStoragePartitionShutdown(StoragePartition* partition); Please add comments to these. https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); 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? https://codereview.chromium.org/2929113002/diff/220001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2929113002/diff/220001/content/public/browser... content/public/browser/render_process_host.h:382: // preinitialized RenderProcessHost, improving performance. Please elaborate (as mentioned in the comment in render_process_host_impl.cc). There should be a fair amount here describing the restrictions, how it's meant to be used, etc.
Thanks for the review! https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:257: * This uses a different mechanism than createSpareWebContents, below, and is subject On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > What is createSpareWebContents used for? Do you think this new spare RPH > approach could be sufficient for its needs in the long term? > It's unclear. The spare WebContents is used to speed up a new custom tabs navigation, and because it also initializes the web contents in addition to the RPH saves about 40ms [1]. On the other hand, the spare WebContents is managed in the android world for tabs, and so won't help for the service worker startup latency problem that inspired this change. The spare WebContents is not very complicated to implement, though, so perhaps the 40ms is worth it. It hasn't been decided yet. > I also notice that createSpareWebContents is a no-op on low-end devices. Should > we be doing the same here? After some discussion we've changed when the spare RPH is created that covers this case---not on low-end devices specifically, but we're consistent about when we think it's okay to create a new process. https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:158: WarmupManager.getInstance().createSpareRenderProcessHost(profile); On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > Worth having a comment here saying how the spare process will be used (i.e., for > either subsequent ServiceWorkers or navigations likely to result from the > upcoming search?). Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:150: int32_t spare_key = FirstRenderProcessHostKey(); On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > I don't understand why we need FirstRenderProcessHostKey(), since it's kind of > indirect and could be affected by other factors. Can we use > GetSpareRenderProcessHostForTesting() instead, and then compare RPHs directly? Done. I wasn't sure what the most reliable abstraction to use for testing would be. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:467: &kDefaultSubframeProcessHostHolderKey; On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > nit: Please keep this and the DefaultSubframeProcessHostHolder class next to > each other. Whoops, I assumed it was for the preceeding class. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:469: // This class manages spare RenderProcessHosts. On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > 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.) Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:474: void WarmupSpareRenderProcessHost(BrowserContext* browser_context) { On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > This should return early if we're over the process limit, right? Yes. See also your comment about low-end devices. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:477: BrowserContext::GetStoragePartition(browser_context, nullptr)); On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > Do we only allow spare processes in the default StoragePartition? That seems > reasonable, but we should document that if so. Done, in the implementation notes above. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:491: browser_context, current_partition, false /* is_for_guests_only */); On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > If we can't use spare processes for is_for_guests_only==true, we should also > document that. Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:515: if (partition == matching_storage_partition_) { On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > nit: No braces needed. Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:542: spare_render_process_host_->Cleanup(); On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > 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. 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:547: // Remove |host| as a possible spare renderer. Does not shut it down cleanly, On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > nit: semicolon rather than comma Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:548: // the assumption is that the host has been shutdown somewhere else and has On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > nit: s/has/is/ Done (well, s/has been/was/, which maybe was your intention?) https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:562: // spare, and not accessed. On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > nit: Rewrap Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:604: .MaybeTakeSpareRenderProcessHost(browser_context_, partition, On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > 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. 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3268: // Try to use a spare process if one is found. On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > 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.) Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3283: return render_process_host; On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > 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. Done. https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > 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? 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. https://codereview.chromium.org/2929113002/diff/220001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2929113002/diff/220001/content/public/browser... content/public/browser/render_process_host.h:382: // preinitialized RenderProcessHost, improving performance. On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > Please elaborate (as mentioned in the comment in render_process_host_impl.cc). > There should be a fair amount here describing the restrictions, how it's meant > to be used, etc. Done.
Thanks! Some responses and additional questions below. https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:257: * This uses a different mechanism than createSpareWebContents, below, and is subject On 2017/06/26 14:45:05, mattcary wrote: > On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > > What is createSpareWebContents used for? Do you think this new spare RPH > > approach could be sufficient for its needs in the long term? > > > It's unclear. The spare WebContents is used to speed up a new custom tabs > navigation, and because it also initializes the web contents in addition to the > RPH saves about 40ms [1]. On the other hand, the spare WebContents is managed in > the android world for tabs, and so won't help for the service worker startup > latency problem that inspired this change. The spare WebContents is not very > complicated to implement, though, so perhaps the 40ms is worth it. It hasn't > been decided yet. Ok. Out of curiosity, what was the link you meant to include for [1]? > > I also notice that createSpareWebContents is a no-op on low-end devices. > Should > > we be doing the same here? > > After some discussion we've changed when the spare RPH is created that covers > this case---not on low-end devices specifically, but we're consistent about when > we think it's okay to create a new process. Sorry, I'm confused-- where's the change that you mention? The code in this file hasn't changed, and createSpareWebContents is still inconsistent with this one in that it calls SysUtils.isLowEndDevice(). https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:150: int32_t spare_key = FirstRenderProcessHostKey(); On 2017/06/26 14:45:06, mattcary wrote: > On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > > I don't understand why we need FirstRenderProcessHostKey(), since it's kind of > > indirect and could be affected by other factors. Can we use > > GetSpareRenderProcessHostForTesting() instead, and then compare RPHs directly? > > Done. I wasn't sure what the most reliable abstraction to use for testing would > be. Sorry, maybe I was unclear. Can we avoid FirstRenderProcessHost entirely? This is the part that bothers me-- if some other feature creates a RPH during the test, it will affect your results. Rather than getting whatever happens to be the first one in the global list, why not call GetSpareRenderProcessHostForTesting() to get exactly the one we're interested in? https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:604: .MaybeTakeSpareRenderProcessHost(browser_context_, partition, On 2017/06/26 14:45:06, mattcary wrote: > On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > > 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. > > 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. 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); On 2017/06/26 14:45:07, mattcary wrote: > On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > > 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? > > 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. 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. > > 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. 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.) > At any rate, I'm happy to implement an alternative if there is one. This is the > best I could come up with. 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. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:153: spare_renderer); We might not need this line, if we can just use GetSpareRenderProcessHostForTesting() to initialize spare_renderer. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:158: EXPECT_EQ(spare_renderer, FirstRenderProcessHost()); I would suggest checking shell()->web_contents()->GetMainFrame()->GetProcess() if you're hoping to verify that the navigation consumed the spare process. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:175: EXPECT_NE(spare_renderer, FirstRenderProcessHost()); Same here. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:503: // ShouldTryToUseExistingProcessHost in this file. Yeah, we'd kind of like to just return early if ShouldTryToUseExistingProcessHost is true, but that requires having a URL. That said, I think we do want to also return early if run_renderer_in_process() is true, right? https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:341: // RenderProcessHost is shutdown. nit: This is a static method, so "associated with the RenderProcessHost" doesn't make sense here. Can you rephrase? https://codereview.chromium.org/2929113002/diff/240001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2929113002/diff/240001/content/public/browser... content/public/browser/render_process_host.h:381: // CreateRenderProcessHost with a matching browser_context may use this This comment makes it sound like any use of CreateRenderProcessHost will use the spare if it's there. (Relevant to our discussion of whether to include it there or not.) https://codereview.chromium.org/2929113002/diff/240001/content/public/browser... content/public/browser/render_process_host.h:388: // be used if those details change (for example, the StoragePartition). Let's rephrase this last bit to avoid "probably." Something like: The spare renderer will only be used if it has a matching BrowserContext and StoragePartition.
https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:257: * This uses a different mechanism than createSpareWebContents, below, and is subject On 2017/06/26 21:22:49, Charlie Reis (slow) wrote: > On 2017/06/26 14:45:05, mattcary wrote: > > On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > > > What is createSpareWebContents used for? Do you think this new spare RPH > > > approach could be sufficient for its needs in the long term? > > > > > It's unclear. The spare WebContents is used to speed up a new custom tabs > > navigation, and because it also initializes the web contents in addition to > the > > RPH saves about 40ms [1]. On the other hand, the spare WebContents is managed > in > > the android world for tabs, and so won't help for the service worker startup > > latency problem that inspired this change. The spare WebContents is not very > > complicated to implement, though, so perhaps the 40ms is worth it. It hasn't > > been decided yet. > > Ok. Out of curiosity, what was the link you meant to include for [1]? > Oops! https://docs.google.com/document/d/1rFx8QHHydnny4T2zdiR3JPELjZRpYReE2cS18EqhJ... > > > > I also notice that createSpareWebContents is a no-op on low-end devices. > > Should > > > we be doing the same here? > > > > After some discussion we've changed when the spare RPH is created that covers > > this case---not on low-end devices specifically, but we're consistent about > when > > we think it's okay to create a new process. > > Sorry, I'm confused-- where's the change that you mention? The code in this > file hasn't changed, and createSpareWebContents is still inconsistent with this > one in that it calls SysUtils.isLowEndDevice(). The check against MaxRendererProcessCount in render_proces_host_impl.cc. It seems better to consolidate all the logic about when to create a spare renderer or not in one place (RPHImpl) rather than have it split between android and native code. That will make this a lot easier to reason about if it is used in more contexts than just the omnibox. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:150: int32_t spare_key = FirstRenderProcessHostKey(); On 2017/06/26 21:22:49, Charlie Reis (slow) wrote: > On 2017/06/26 14:45:06, mattcary wrote: > > On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > > > I don't understand why we need FirstRenderProcessHostKey(), since it's kind > of > > > indirect and could be affected by other factors. Can we use > > > GetSpareRenderProcessHostForTesting() instead, and then compare RPHs > directly? > > > > Done. I wasn't sure what the most reliable abstraction to use for testing > would > > be. > > Sorry, maybe I was unclear. Can we avoid FirstRenderProcessHost entirely? This > is the part that bothers me-- if some other feature creates a RPH during the > test, it will affect your results. Rather than getting whatever happens to be > the first one in the global list, why not call > GetSpareRenderProcessHostForTesting() to get exactly the one we're interested > in? Done, the key bit being that your shell()->web_contents()->GetMainFrame()->GetProcess() idea is really what I wanted instead of FirstRenderProcessHost(). https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:604: .MaybeTakeSpareRenderProcessHost(browser_context_, partition, On 2017/06/26 21:22:49, Charlie Reis (slow) wrote: > On 2017/06/26 14:45:06, mattcary wrote: > > On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > > > 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. > > > > 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. > > 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. 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > On 2017/06/26 14:45:07, mattcary wrote: > > On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > > > 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? > > > > 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. > > 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. > > > > > 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. > > 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.) > > > At any rate, I'm happy to implement an alternative if there is one. This is > the > > best I could come up with. > > 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. 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... https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:153: spare_renderer); On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > We might not need this line, if we can just use > GetSpareRenderProcessHostForTesting() to initialize spare_renderer. Done. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:158: EXPECT_EQ(spare_renderer, FirstRenderProcessHost()); On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > I would suggest checking shell()->web_contents()->GetMainFrame()->GetProcess() > if you're hoping to verify that the navigation consumed the spare process. Done, that's a good idea, thanks. This is what I was trying to get at with FirstRenderProcessHost() but seems to be strictly better. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:175: EXPECT_NE(spare_renderer, FirstRenderProcessHost()); On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > Same here. Done. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:503: // ShouldTryToUseExistingProcessHost in this file. On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > Yeah, we'd kind of like to just return early if > ShouldTryToUseExistingProcessHost is true, but that requires having a URL. > > That said, I think we do want to also return early if run_renderer_in_process() > is true, right? Good point, done. https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/240001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:341: // RenderProcessHost is shutdown. On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > nit: This is a static method, so "associated with the RenderProcessHost" doesn't > make sense here. Can you rephrase? Done. https://codereview.chromium.org/2929113002/diff/240001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2929113002/diff/240001/content/public/browser... content/public/browser/render_process_host.h:381: // CreateRenderProcessHost with a matching browser_context may use this On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > This comment makes it sound like any use of CreateRenderProcessHost will use the > spare if it's there. (Relevant to our discussion of whether to include it there > or not.) Rephrased, as CreateRenderProcessHost is not content/public but in RenderProcessHostImpl (see reply to other comment). https://codereview.chromium.org/2929113002/diff/240001/content/public/browser... content/public/browser/render_process_host.h:388: // be used if those details change (for example, the StoragePartition). On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > Let's rephrase this last bit to avoid "probably." Something like: > > The spare renderer will only be used if it has a matching BrowserContext and > StoragePartition. OK, but my instinct is that those are implementation details that users of the public interface should not rely on.
Thanks! A few more observations and questions below. https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:257: * This uses a different mechanism than createSpareWebContents, below, and is subject On 2017/06/28 13:14:38, mattcary wrote: > On 2017/06/26 21:22:49, Charlie Reis (slow) wrote: > > On 2017/06/26 14:45:05, mattcary wrote: > > > On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > > > > What is createSpareWebContents used for? Do you think this new spare RPH > > > > approach could be sufficient for its needs in the long term? > > > > > > > It's unclear. The spare WebContents is used to speed up a new custom tabs > > > navigation, and because it also initializes the web contents in addition to > > the > > > RPH saves about 40ms [1]. On the other hand, the spare WebContents is > managed > > in > > > the android world for tabs, and so won't help for the service worker startup > > > latency problem that inspired this change. The spare WebContents is not very > > > complicated to implement, though, so perhaps the 40ms is worth it. It hasn't > > > been decided yet. > > > > Ok. Out of curiosity, what was the link you meant to include for [1]? > > > Oops! > https://docs.google.com/document/d/1rFx8QHHydnny4T2zdiR3JPELjZRpYReE2cS18EqhJ... > > > > > > > > I also notice that createSpareWebContents is a no-op on low-end devices. > > > Should > > > > we be doing the same here? > > > > > > After some discussion we've changed when the spare RPH is created that > covers > > > this case---not on low-end devices specifically, but we're consistent about > > when > > > we think it's okay to create a new process. > > > > Sorry, I'm confused-- where's the change that you mention? The code in this > > file hasn't changed, and createSpareWebContents is still inconsistent with > this > > one in that it calls SysUtils.isLowEndDevice(). > > The check against MaxRendererProcessCount in render_proces_host_impl.cc. > > It seems better to consolidate all the logic about when to create a spare > renderer or not in one place (RPHImpl) rather than have it split between android > and native code. That will make this a lot easier to reason about if it is used > in more contexts than just the omnibox. Agreed-- that seems like a good plan. (Just to follow up, will the isLowEndDevice() call below go away at some point to be consistent with this?) https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:542: spare_render_process_host_->Cleanup(); On 2017/06/26 14:45:07, mattcary wrote: > On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > > 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. > > 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. 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 > > 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). 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. > 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. 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). https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:604: .MaybeTakeSpareRenderProcessHost(browser_context_, partition, On 2017/06/28 13:14:38, mattcary wrote: > On 2017/06/26 21:22:49, Charlie Reis (slow) wrote: > > On 2017/06/26 14:45:06, mattcary wrote: > > > On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > > > > 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. > > > > > > 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. > > > > 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. > > 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. 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 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. 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. :) https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); On 2017/06/28 13:14:38, mattcary wrote: > On 2017/06/26 21:22:50, Charlie Reis (slow) wrote: > > On 2017/06/26 14:45:07, mattcary wrote: > > > On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > > > > 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? > > > > > > 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. > > > > 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. > > > > > > > > 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. > > > > 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.) > > > > > At any rate, I'm happy to implement an alternative if there is one. This is > > the > > > best I could come up with. > > > > 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. > > 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. 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.) > 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... 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. https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:573: if (host && host == spare_render_process_host_) { nit: Let's make the null pointer check be on spare_render_process_host_ rather than host. It's equivalent given the second check, but it seems like we should never see a case where |host| is null, barring a bad caller. It's also closer to the motivation for the check, which is presumably avoiding a null deref below. https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:117: static RenderProcessHost* CreateRenderProcessHost( Let's include a comment here suggesting that callers consider using the spare RPH if it's available and matches (e.g., using the helper method I'm requesting).
https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:257: * This uses a different mechanism than createSpareWebContents, below, and is subject On 2017/06/28 22:31:34, Charlie Reis (slow) wrote: > On 2017/06/28 13:14:38, mattcary wrote: > > On 2017/06/26 21:22:49, Charlie Reis (slow) wrote: > > > On 2017/06/26 14:45:05, mattcary wrote: > > > > On 2017/06/25 23:48:27, Charlie Reis (slow) wrote: > > > > > What is createSpareWebContents used for? Do you think this new spare > RPH > > > > > approach could be sufficient for its needs in the long term? > > > > > > > > > It's unclear. The spare WebContents is used to speed up a new custom tabs > > > > navigation, and because it also initializes the web contents in addition > to > > > the > > > > RPH saves about 40ms [1]. On the other hand, the spare WebContents is > > managed > > > in > > > > the android world for tabs, and so won't help for the service worker > startup > > > > latency problem that inspired this change. The spare WebContents is not > very > > > > complicated to implement, though, so perhaps the 40ms is worth it. It > hasn't > > > > been decided yet. > > > > > > Ok. Out of curiosity, what was the link you meant to include for [1]? > > > > > Oops! > > > https://docs.google.com/document/d/1rFx8QHHydnny4T2zdiR3JPELjZRpYReE2cS18EqhJ... > > > > > > > > > > > > I also notice that createSpareWebContents is a no-op on low-end devices. > > > > > Should > > > > > we be doing the same here? > > > > > > > > After some discussion we've changed when the spare RPH is created that > > covers > > > > this case---not on low-end devices specifically, but we're consistent > about > > > when > > > > we think it's okay to create a new process. > > > > > > Sorry, I'm confused-- where's the change that you mention? The code in this > > > file hasn't changed, and createSpareWebContents is still inconsistent with > > this > > > one in that it calls SysUtils.isLowEndDevice(). > > > > The check against MaxRendererProcessCount in render_proces_host_impl.cc. > > > > It seems better to consolidate all the logic about when to create a spare > > renderer or not in one place (RPHImpl) rather than have it split between > android > > and native code. That will make this a lot easier to reason about if it is > used > > in more contexts than just the omnibox. > > Agreed-- that seems like a good plan. > > (Just to follow up, will the isLowEndDevice() call below go away at some point > to be consistent with this?) I suspect so. Possibly createSpareWebContents will just go away. I've created crbug.com/737890 to track this formally. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:542: spare_render_process_host_->Cleanup(); > > > > Cleanup() calls RenderProcessExited and/or RenderProcessHostDestroyed on all > > observers, so it seems to reduce that possibility. > > 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. > 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. > > 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). > > Is there a typo here? (Maybe you meant RenderProcessHostImpl instead of > RenderFrameHostImpl?) > Yes, sorry, I meant RPHI. > > RPHI is responsible for process assignment, but I'm not sure we realized the > issue with GetExistingProcessHost I just noted above. > > > > 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. > > 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). 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. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:604: .MaybeTakeSpareRenderProcessHost(browser_context_, partition, On 2017/06/28 22:31:35, Charlie Reis (slow) wrote: > On 2017/06/28 13:14:38, mattcary wrote: > > On 2017/06/26 21:22:49, Charlie Reis (slow) wrote: > > > On 2017/06/26 14:45:06, mattcary wrote: > > > > On 2017/06/25 23:48:28, Charlie Reis (slow) wrote: > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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 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. > > 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. :) :P point taken... https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); > > 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. > Actually, it's already done for us: NotifyWillBeDestroyed calls ForceReleaseWorkerRefCounts which calls Cleanup() which notifies observers, including my spare RPH Manager. > > 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? > 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). > I see you already removed the StoragePartition test? That's ok if we switch to > BrowserContext::NotifyWillBeDestroyed. Ack. https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:573: if (host && host == spare_render_process_host_) { On 2017/06/28 22:31:35, Charlie Reis (slow) wrote: > nit: Let's make the null pointer check be on spare_render_process_host_ rather > than host. > > It's equivalent given the second check, but it seems like we should never see a > case where |host| is null, barring a bad caller. It's also closer to the > motivation for the check, which is presumably avoiding a null deref below. Done. https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/300001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:117: static RenderProcessHost* CreateRenderProcessHost( On 2017/06/28 22:31:35, Charlie Reis (slow) wrote: > Let's include a comment here suggesting that callers consider using the spare > RPH if it's available and matches (e.g., using the helper method I'm > requesting). Done, along with the addition of the alternative constructor you suggest.
Thanks! I've got some additional suggestions below, but I think we're basically ready once those are fixed. Given the US holiday coming up, I'll give my LGTM now if you're able to make the changes below. If anything comes up that you think needs additional review, I can take another look on Friday and CQ it for you if you're otherwise ready. https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:542: spare_render_process_host_->Cleanup(); On 2017/06/29 12:57:09, mattcary wrote: > > > > > > Cleanup() calls RenderProcessExited and/or RenderProcessHostDestroyed on all > > > observers, so it seems to reduce that possibility. > > > > 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. > > > 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. > > > > 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). > > > > Is there a typo here? (Maybe you meant RenderProcessHostImpl instead of > > RenderFrameHostImpl?) > > > Yes, sorry, I meant RPHI. > > > > > RPHI is responsible for process assignment, but I'm not sure we realized the > > issue with GetExistingProcessHost I just noted above. > > > > > > > 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. > > > > 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). > > 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. Thanks! I added a few suggestions to improve these limitations in the test. https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); On 2017/06/29 12:57:09, mattcary wrote: > > > > 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. > > > Actually, it's already done for us: NotifyWillBeDestroyed calls > ForceReleaseWorkerRefCounts which calls Cleanup() which notifies observers, > including my spare RPH Manager. 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. > > > > 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? > > > 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). Ah, that's unfortunate. Thanks for checking. > > > I see you already removed the StoragePartition test? That's ok if we switch > to > > BrowserContext::NotifyWillBeDestroyed. > > Ack. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:217: // A spare RPH should be created with a max of 2 renderer process. nit: s/process/processes/ https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:226: // renderer or the existing render for the new navigation at random. nit: s/render/renderer/ https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:233: // If the spare renderer wasn't picked up for the naviation above, perform nit: navigation https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:242: RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()); This doesn't look right to me, and it's never failing for me locally without the fix. Suppose we left the spare renderer registered as a spare when we used it in the navigation on line 228. Now that we've removed the process limit on line 231, the navigation on line 240 will definitely use the spare (unexpectedly sharing with the earlier navigation), and this check will never fail. It seems like we want to check that the first navigation's process is not equal to the spare process (either because we picked the spare RPH and cleared it, or because we didn't pick it). We also want to check that the spare RPH doesn't have anything in it if it still exists. In other words, replace lines 228-243 with: Shell* new_browser = CreateBrowser(); NavigateToURL(new_browser, test_url); EXPECT_NE(RenderProcessHostImpl::GetSpareRenderProcessHostForTesting(), new_browser->web_contents()->GetMainFrame()->GetProcess()); if (RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()) { EXPECT_EQ(0, RenderProcessHostImpl::GetSpareRenderProcessHostForTesting() ->VisibleWidgetCount()); } Beyond that, I'd like to avoid having this test be non-deterministic. (Flaky tests tend to get disabled when they start failing, not fixed.) I think we can pull that off using a TestContentBrowserClient that only returns true from IsSuitableHost for the spare RPH. That should force the outcome we want to test and make it deterministic. You can see an example of using a TestContentBrowserClient in a browser test in ServiceWorkerVersionBrowserTest.FetchWithSaveData. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:502: // Don't create a spare renderer if we're running a single renderer or if nit: s/running a single renderer/using --single-process/ https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:532: RenderProcessHost* rph = spare_render_process_host_; As I mentioned earlier, I think we should include checks here for the following things now that we're handling the random reuse case: - listeners_.IsEmpty() should be true - GetWorkerRefCount() should be 0 - pending_views_ should be 0 If those fail, it means we're putting things in the spare RPH without meaning to. I'd suggest having them be CHECKs, since DCHECKs won't fire in the wild and we won't learn anything unless existing tests happen to hit them. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3315: suitable_renderers[random_index]); Sure, that seems like an ok way to handle it. Please include a comment, like: If this happened to be the spare RenderProcessHost, don't treat it as a spare anymore.
Thanks again for your review! I need to get owners approval for the java code but will go ahead and submit as you suggest due to the holiday. If I've messed anything up of course I'll fix as quickly as possible in a follow-up cl. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:217: // A spare RPH should be created with a max of 2 renderer process. On 2017/06/29 21:09:16, Charlie Reis (slow) wrote: > nit: s/process/processes/ Done. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:226: // renderer or the existing render for the new navigation at random. On 2017/06/29 21:09:16, Charlie Reis (slow) wrote: > nit: s/render/renderer/ Done. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:233: // If the spare renderer wasn't picked up for the naviation above, perform On 2017/06/29 21:09:16, Charlie Reis (slow) wrote: > nit: navigation Done. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:242: RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()); On 2017/06/29 21:09:16, Charlie Reis (slow) wrote: > This doesn't look right to me, and it's never failing for me locally without the > fix. Suppose we left the spare renderer registered as a spare when we used it > in the navigation on line 228. Now that we've removed the process limit on line > 231, the navigation on line 240 will definitely use the spare (unexpectedly > sharing with the earlier navigation), and this check will never fail. > Right, this is what I meant about not being able to tell how a spare renderer was consumed. If I remove the fix in GetExistingProcessHost, the spare renderer can get reused for the second navigation in this if statement. If that would cause things to crash, this test would pick that up. Unfortunately, currently if an RPH is reused nothing crashes. With your suggestion for checking that it's unused, that's no longer the case. I also like the idea of checking the widget count rather than just navigating and seeing if anything breaks. > It seems like we want to check that the first navigation's process is not equal > to the spare process (either because we picked the spare RPH and cleared it, or > because we didn't pick it). We also want to check that the spare RPH doesn't > have anything in it if it still exists. In other words, replace lines 228-243 > with: > > Shell* new_browser = CreateBrowser(); > NavigateToURL(new_browser, test_url); > EXPECT_NE(RenderProcessHostImpl::GetSpareRenderProcessHostForTesting(), > new_browser->web_contents()->GetMainFrame()->GetProcess()); > > if (RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()) { > EXPECT_EQ(0, RenderProcessHostImpl::GetSpareRenderProcessHostForTesting() > ->VisibleWidgetCount()); > } > > Beyond that, I'd like to avoid having this test be non-deterministic. (Flaky > tests tend to get disabled when they start failing, not fixed.) I think we can > pull that off using a TestContentBrowserClient that only returns true from > IsSuitableHost for the spare RPH. That should force the outcome we want to test > and make it deterministic. > > You can see an example of using a TestContentBrowserClient in a browser test in > ServiceWorkerVersionBrowserTest.FetchWithSaveData. That seemed to be easy enough. With this and the other changes, if I remove the DropSpare in GetExistingProcessHost, the test fails deterministically. Hooray! https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:502: // Don't create a spare renderer if we're running a single renderer or if On 2017/06/29 21:09:17, Charlie Reis (slow) wrote: > nit: s/running a single renderer/using --single-process/ Done. https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:532: RenderProcessHost* rph = spare_render_process_host_; On 2017/06/29 21:09:16, Charlie Reis (slow) wrote: > As I mentioned earlier, I think we should include checks here for the following > things now that we're handling the random reuse case: > - listeners_.IsEmpty() should be true > - GetWorkerRefCount() should be 0 > - pending_views_ should be 0 > > If those fail, it means we're putting things in the spare RPH without meaning > to. I'd suggest having them be CHECKs, since DCHECKs won't fire in the wild and > we won't learn anything unless existing tests happen to hit them. Done. I had thought that was in the context of not using a spare renderer in GetExistingProccessHost which is not what we ended up doing. I grok now. This required extending the RPHImpl public interface. I think that I properly characterized the difference between what we want to do and the existing RPH::IsUnused() flag, but can you confirm? https://codereview.chromium.org/2929113002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3315: suitable_renderers[random_index]); On 2017/06/29 21:09:16, Charlie Reis (slow) wrote: > Sure, that seems like an ok way to handle it. Please include a comment, like: > > If this happened to be the spare RenderProcessHost, don't treat it as a spare > anymore. Done.
Description was changed from ========== Enable spare RenderProcessHost to be preinitialized. Enables a preinitialization of an unbound RenderProcessHost, and refactors RenderProcessHost creation to allow that to be used where appropriate. BUG=730587 ========== to ========== Enable spare RenderProcessHost to be preinitialized. Enables a preinitialization of an unbound RenderProcessHost, and refactors RenderProcessHost creation to allow that to be used where appropriate. BUG=730587 ==========
mattcary@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for java owners review. Ted, this is the plan B for speeding up SW startup in the context of the search PWA, although it turns out to be useful generally. See go/spqa-latency for example. I think that you have all the context (given that you're on the email thread "Omnibox Heuristic for User Will Search"), but please let me know if I can answer any questions.
https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2929113002/diff/220001/content/browser/storag... content/browser/storage_partition_impl.cc:405: RenderProcessHostImpl::OnStoragePartitionShutdown(this); On 2017/06/29 21:09:16, Charlie Reis (slow) wrote: > On 2017/06/29 12:57:09, mattcary wrote: > > > > > > 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. > > > > > Actually, it's already done for us: NotifyWillBeDestroyed calls > > ForceReleaseWorkerRefCounts which calls Cleanup() which notifies observers, > > including my spare RPH Manager. > > 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. > > > > > > > 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? > > > > > 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). > > Ah, that's unfortunate. Thanks for checking. > > > > > > I see you already removed the StoragePartition test? That's ok if we switch > > to > > > BrowserContext::NotifyWillBeDestroyed. > > > > Ack. > Added the comment of (2) and (3).
the java code certainly seems fine, just some clarifying questions about how this relates to a few things https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:263: ThreadUtils.assertOnUiThread(); should we be excluding low end devices here? i.e. if (SysUtils.isLowEndDevice()) return; I thought we tried to limit the renderer count in that case to 0, and it would be sad if that caused undo pressure on the visible tab. https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:166: WarmupManager.getInstance().createSpareRenderProcessHost(profile); Is OmniboxPrerender fully disabled at this point? If not, does the prerendered contents consume this or are they separate? I'm a bit worried that we're potentially doing too much at this point if we try to do both and they are disjoint.
creis@chromium.org changed reviewers: + alexmos@chromium.org
LGTM with nits. alexmos@, can you take a look at my last question below? https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:251: RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()); nit: Might be worth including a check that the new Shell returned from CreateBrowser() now has a process equal to the previous |spare_renderer|, just to document/confirm that's where it went. https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1099: bool RenderProcessHostImpl::HostHasNotBeenUsed() { nit: This should be put in the same order as in the .h file (i.e., after GetSpareRenderProcessHostForTesting()). https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3452: if (!render_process_host) { Let's keep a comment here, like: // Otherwise, use the spare RenderProcessHost or create a new one. (I want to convey that this is meant to be the last case, unlike all the ones above which are ok with returning null.) https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:359: bool HostHasNotBeenUsed(); Hmm, it's unfortunate to have this name be effectively the same as IsUnused() but have a different meaning. I suppose we can get by for the moment so that you can land this, but we should decide whether one should be renamed or if this definition can work for both use cases. At least include a TODO(creis,alexmos) to rename or combine with IsUnused. alexmos@: Is there a problem with using this definition in your IsSuitableHost call? It's basically IsUnused() && listeners_.IsEmpty() && GetWorkerRefCount() == 0 && pending_views_ == 0. (I'm guessing that might be a problem, but there's a chance it could be better, e.g., if we have a race where both a web page and a page requiring a dedicated process decide to use the same process before either one commits?)
https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:359: bool HostHasNotBeenUsed(); On 2017/06/30 21:42:16, Charlie Reis (OOO til July 5) wrote: > Hmm, it's unfortunate to have this name be effectively the same as IsUnused() > but have a different meaning. > > I suppose we can get by for the moment so that you can land this, but we should > decide whether one should be renamed or if this definition can work for both use > cases. At least include a TODO(creis,alexmos) to rename or combine with > IsUnused. > > alexmos@: Is there a problem with using this definition in your IsSuitableHost > call? It's basically IsUnused() && listeners_.IsEmpty() && GetWorkerRefCount() > == 0 && pending_views_ == 0. (I'm guessing that might be a problem, but there's > a chance it could be better, e.g., if we have a race where both a web page and a > page requiring a dedicated process decide to use the same process before either > one commits?) I'd need to check this more carefully, but I'm guessing the listeners_.IsEmpty() might be a problem. The main use case for IsUnused() was avoiding an unnecessary process swap when a just-created blank tab with a site-less SiteInstance was navigated to a site that requires a dedicated process. It should be fine to reuse the initially created process in that case, but with HostHasNotBeenUsed, the RFH/RVH might be created and added to listeners_ before IsSuitableHost() checks IsUnused(). Good catch about the race. A page requiring a dedicated process should use a SiteInstance with its site assigned, causing IsUnused to become false right after GetProcess(), but there could still be a pending navigation for a web site with a lazily-assigned site URL that had picked that process earlier but hasn't committed yet. I've just filed https://bugs.chromium.org/p/chromium/issues/detail?id=738634 with steps that actually seem to repro this. I'm fine with adding a TODO for now and revisiting it as part of the bug above.
https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:263: ThreadUtils.assertOnUiThread(); On 2017/06/30 16:16:56, Ted C wrote: > should we be excluding low end devices here? > > i.e. > > if (SysUtils.isLowEndDevice()) return; > > I thought we tried to limit the renderer count in that case to 0, and it would > be sad if that caused undo pressure on the visible tab. We manage the logic on when to create a spare renderer in render_process_host_impl.cc rather than spread it across java and native code. We restrict spare renderer creation according to the MaxRendererProcessCount only. In the current usage, we create a spare renderer when we are almost certain a new process will be created anyway, so it will not be adding pressure that would not have been otherwise added. https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:166: WarmupManager.getInstance().createSpareRenderProcessHost(profile); On 2017/06/30 16:16:56, Ted C wrote: > Is OmniboxPrerender fully disabled at this point? > > If not, does the prerendered contents consume this or are they separate? I'm a > bit worried that we're potentially doing too much at this point if we try to do > both and they are disjoint. It's 99% disabled, but that's a good point. Added logic to coordinate. https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_browsertest.cc:251: RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()); On 2017/06/30 21:42:16, Charlie Reis (OOO til July 5) wrote: > nit: Might be worth including a check that the new Shell returned from > CreateBrowser() now has a process equal to the previous |spare_renderer|, just > to document/confirm that's where it went. Done. https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1099: bool RenderProcessHostImpl::HostHasNotBeenUsed() { On 2017/06/30 21:42:16, Charlie Reis (OOO til July 5) wrote: > nit: This should be put in the same order as in the .h file (i.e., after > GetSpareRenderProcessHostForTesting()). Done. https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3452: if (!render_process_host) { On 2017/06/30 21:42:16, Charlie Reis (OOO til July 5) wrote: > Let's keep a comment here, like: > > // Otherwise, use the spare RenderProcessHost or create a new one. > > (I want to convey that this is meant to be the last case, unlike all the ones > above which are ok with returning null.) Done. https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2929113002/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:359: bool HostHasNotBeenUsed(); On 2017/07/01 00:47:17, alexmos (OOO until Jul 5) wrote: > On 2017/06/30 21:42:16, Charlie Reis (OOO til July 5) wrote: > > Hmm, it's unfortunate to have this name be effectively the same as IsUnused() > > but have a different meaning. > > > > I suppose we can get by for the moment so that you can land this, but we > should > > decide whether one should be renamed or if this definition can work for both > use > > cases. At least include a TODO(creis,alexmos) to rename or combine with > > IsUnused. > > > > alexmos@: Is there a problem with using this definition in your IsSuitableHost > > call? It's basically IsUnused() && listeners_.IsEmpty() && > GetWorkerRefCount() > > == 0 && pending_views_ == 0. (I'm guessing that might be a problem, but > there's > > a chance it could be better, e.g., if we have a race where both a web page and > a > > page requiring a dedicated process decide to use the same process before > either > > one commits?) > > I'd need to check this more carefully, but I'm guessing the listeners_.IsEmpty() > might be a problem. The main use case for IsUnused() was avoiding an > unnecessary process swap when a just-created blank tab with a site-less > SiteInstance was navigated to a site that requires a dedicated process. It > should be fine to reuse the initially created process in that case, but with > HostHasNotBeenUsed, the RFH/RVH might be created and added to listeners_ before > IsSuitableHost() checks IsUnused(). > > Good catch about the race. A page requiring a dedicated process should use a > SiteInstance with its site assigned, causing IsUnused to become false right > after GetProcess(), but there could still be a pending navigation for a web site > with a lazily-assigned site URL that had picked that process earlier but hasn't > committed yet. I've just filed > https://bugs.chromium.org/p/chromium/issues/detail?id=738634 with steps that > actually seem to repro this. > > I'm fine with adding a TODO for now and revisiting it as part of the bug above. > Done. I assigned the TODO to you, alex, to be consistent with the bug, but I'm happy to help with the follow-up (to the best of my ability...)
https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:166: WarmupManager.getInstance().createSpareRenderProcessHost(profile); On 2017/07/03 08:03:02, mattcary wrote: > On 2017/06/30 16:16:56, Ted C wrote: > > Is OmniboxPrerender fully disabled at this point? > > > > If not, does the prerendered contents consume this or are they separate? I'm > a > > bit worried that we're potentially doing too much at this point if we try to > do > > both and they are disjoint. > > It's 99% disabled, but that's a good point. Added logic to coordinate. Actually, on further reflection, the spare renderer should just get picked up by the prerender, which is the behavior we want. I'm trying to confirm that with traces now.
On 2017/07/03 08:08:33, mattcary wrote: > https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java > (right): > > https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:166: > WarmupManager.getInstance().createSpareRenderProcessHost(profile); > On 2017/07/03 08:03:02, mattcary wrote: > > On 2017/06/30 16:16:56, Ted C wrote: > > > Is OmniboxPrerender fully disabled at this point? > > > > > > If not, does the prerendered contents consume this or are they separate? > I'm > > a > > > bit worried that we're potentially doing too much at this point if we try to > > do > > > both and they are disjoint. > > > > It's 99% disabled, but that's a good point. Added logic to coordinate. > > Actually, on further reflection, the spare renderer should just get picked up by > the prerender, which is the behavior we want. I'm trying to confirm that with > traces now. As I expected, the spare renderer is used by the prererender. Because the spare renderer is used down in the RenderProcessHostImpl level, it's completely transparent to prerender, as designed.
https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:263: ThreadUtils.assertOnUiThread(); On 2017/07/03 08:03:02, mattcary wrote: > On 2017/06/30 16:16:56, Ted C wrote: > > should we be excluding low end devices here? > > > > i.e. > > > > if (SysUtils.isLowEndDevice()) return; > > > > I thought we tried to limit the renderer count in that case to 0, and it would > > be sad if that caused undo pressure on the visible tab. > > We manage the logic on when to create a spare renderer in > render_process_host_impl.cc rather than spread it across java and native code. > We restrict spare renderer creation according to the MaxRendererProcessCount > only. In the current usage, we create a spare renderer when we are almost > certain a new process will be created anyway, so it will not be adding pressure > that would not have been otherwise added. I looked at that as well, and I'm not seeing where low end devices are limited through that code. On Android, we don't limit it based on anything ram related: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro... In fact, before this change https://codereview.chromium.org/2793623002, it was unbounded. Is there something I'm missing (again, this isn't the area I'm familiar with)? For low end devices, this functionality should be disabled (or at least that is in line with all previous behavior on low end), so I don't think this is something we want to rely on Android's killing logic to handle. It is definitely unfortunate to have this spread so many places, so I'd certainly like to find a way to make that not required.
On 2017/07/05 03:54:05, Ted C (OOO till 7.10) wrote: > https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java > (right): > > https://codereview.chromium.org/2929113002/diff/380001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:263: > ThreadUtils.assertOnUiThread(); > On 2017/07/03 08:03:02, mattcary wrote: > > On 2017/06/30 16:16:56, Ted C wrote: > > > should we be excluding low end devices here? > > > > > > i.e. > > > > > > if (SysUtils.isLowEndDevice()) return; > > > > > > I thought we tried to limit the renderer count in that case to 0, and it > would > > > be sad if that caused undo pressure on the visible tab. > > > > We manage the logic on when to create a spare renderer in > > render_process_host_impl.cc rather than spread it across java and native code. > > We restrict spare renderer creation according to the MaxRendererProcessCount > > only. In the current usage, we create a spare renderer when we are almost > > certain a new process will be created anyway, so it will not be adding > pressure > > that would not have been otherwise added. > > I looked at that as well, and I'm not seeing where low end devices are limited > through that code. > > On Android, we don't limit it based on anything ram related: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro... > > In fact, before this change https://codereview.chromium.org/2793623002, it was > unbounded. > > Is there something I'm missing (again, this isn't the area I'm familiar with)? > For low end devices, this functionality should be disabled (or at least that is > in line with all previous behavior on low end), so I don't think this is > something we want to rely on Android's killing logic to handle. It is > definitely unfortunate to have this spread so many places, so I'd certainly like > to find a way to make that not required. Yes, your comments are correct. My point was that it's not clear that we need to disable this feature on low-end devices. Essentially, the reasoning is that it does not use any more resources than would otherwise be used by a cross-site navigation, so it is not any extra work even for a low-end device. I suggest that we look at OOM and similar failures for low-end devices when this is in beta, and if it increases under this feature, then we should disable it. Does that sound right to you?
tedchoc@chromium.org changed reviewers: + mariakhomenko@chromium.org
+mariakhomenko, adding Maria for more insight into low end handling. I was under the impression that OOMs were not the only signal we are interested in this case. We want to make sure doesn't consume all the memory on the device and forcing out other apps at the larger ecosystem level. As this only happens when focusing the omnibox, it could be ok as the next renderer is likely to be consumed nearly right away. Again, my reservation is the inconsistency with other similar functionality on low end and I want to make some better reasoning on how we should be handling this across the board.
On 2017/07/05 17:23:36, Ted C (OOO till 7.10) wrote: > +mariakhomenko, adding Maria for more insight into low end handling. I was > under the impression that OOMs were not the only signal we are interested in > this case. We want to make sure doesn't consume all the memory on the device > and forcing out other apps at the larger ecosystem level. As this only happens > when focusing the omnibox, it could be ok as the next renderer is likely to be > consumed nearly right away. Again, my reservation is the inconsistency with > other similar functionality on low end and I want to make some better reasoning > on how we should be handling this across the board. Thanks everyone for the insightful discussion and reviews. Would it be time-consuming to add a UMA to track how often the spare renderer is not used? Alternatively, can we: 1. temporarily decide to disable this for low-end devices 2. move forward with the change 3. open a new bug to discuss the if/how for low-end devices Cheers,
On 2017/07/07 02:29:56, kenjibaheux wrote: > On 2017/07/05 17:23:36, Ted C (OOO till 7.10) wrote: > > +mariakhomenko, adding Maria for more insight into low end handling. I was > > under the impression that OOMs were not the only signal we are interested in > > this case. We want to make sure doesn't consume all the memory on the device > > and forcing out other apps at the larger ecosystem level. As this only happens > > when focusing the omnibox, it could be ok as the next renderer is likely to be > > consumed nearly right away. Again, my reservation is the inconsistency with > > other similar functionality on low end and I want to make some better > reasoning > > on how we should be handling this across the board. > > Thanks everyone for the insightful discussion and reviews. > > Would it be time-consuming to add a UMA to track how often the spare renderer is > not used? > > Alternatively, can we: > 1. temporarily decide to disable this for low-end devices > 2. move forward with the change > 3. open a new bug to discuss the if/how for low-end devices > > Cheers, Hi, I think the current behavior is fine, but option (3) works for me. As Ted says, I think this is fine to create a RenderProcessHost on low-end devices, as it is almost certainly going to be used right away, and there is a point in navigation where we have two renderers in memory concurrently, IIRC. This means that this change is likely not increasing peak memory usage. Furthermore, the renderer starts in the background state, meaning that's in the "background" cgroup on Android, and in the "cached" importance bucket, so this would not lead to killing applications left and right. We need to review our general management of this speculative actions, especially in the context of low-end devices, and at least add a comment to the code to explain the reasoning behind the differentiated treatment.
Description was changed from ========== Enable spare RenderProcessHost to be preinitialized. Enables a preinitialization of an unbound RenderProcessHost, and refactors RenderProcessHost creation to allow that to be used where appropriate. BUG=730587 ========== to ========== Enable spare RenderProcessHost to be preinitialized. Enables a preinitialization of an unbound RenderProcessHost, and refactors RenderProcessHost creation to allow that to be used where appropriate. BUG=730587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Makes sense. I've created crbug.com/740957 to track the low-end discussion, please CC as you see fit. A feature flag has been added so we can run a proper experiment. I also identified through manual testing a couple of instances were spare renderers were not being destroyed when unused, or (in the case of typing in the fakebox from the NTP not needed).
The CQ bit was checked by mattcary@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mattcary@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm for android-y stuff. I think we might want to tweak the logic to prevent kicking it off, but it would require some more testing. I do wonder if there is an even better way to determine whether the current renderer can be reused before attempting to spin up a spare (I believe about:blank is the same as the native pages). Also, adding the experiment makes sense to me as a way to verify this across different markets. https://codereview.chromium.org/2929113002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2929113002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:164: if (!focusedFromFakebox) { I wonder if this should instead be NewTabPage.isNTPUrl(url) instead. I suspect because the renderer of the NTP is reused for the first subsequent navigation that we don't want to do this ever from the NTP. I think tablets will be the easiest way to test this as their NTP has both the fakebox and the real omnibox so you could compare.
https://codereview.chromium.org/2929113002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java (right): https://codereview.chromium.org/2929113002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:164: if (!focusedFromFakebox) { On 2017/07/11 16:21:46, Ted C wrote: > I wonder if this should instead be NewTabPage.isNTPUrl(url) instead. I suspect > because the renderer of the NTP is reused for the first subsequent navigation > that we don't want to do this ever from the NTP. > > I think tablets will be the easiest way to test this as their NTP has both the > fakebox and the real omnibox so you could compare. Done. I tested on a tablet, and using focusedFromFakebox seemed to be equivalent to isNTPUrl. Changed anyway as the the API you suggest seems better, thanks!
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2929113002/#ps470001 (title: "git cl format added histograms.xml erroneously to the cl")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2929113002/#ps490001 (title: "bugfix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mattcary@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2929113002/#ps510001 (title: "Change creation of storage partition to not break unittests with subtle threading issues")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 510001, "attempt_start_ts": 1499952060291710,
"parent_rev": "ed644baf37747f53133478d3f8d266f66e8f2aa6", "commit_rev":
"28c8676d89927c00b9e64dfdb7681027d93a37df"}
Message was sent while issue was closed.
Description was changed from ========== Enable spare RenderProcessHost to be preinitialized. Enables a preinitialization of an unbound RenderProcessHost, and refactors RenderProcessHost creation to allow that to be used where appropriate. BUG=730587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Enable spare RenderProcessHost to be preinitialized. Enables a preinitialization of an unbound RenderProcessHost, and refactors RenderProcessHost creation to allow that to be used where appropriate. BUG=730587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2929113002 Cr-Commit-Position: refs/heads/master@{#486372} Committed: https://chromium.googlesource.com/chromium/src/+/28c8676d89927c00b9e64dfdb768... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:510001) as https://chromium.googlesource.com/chromium/src/+/28c8676d89927c00b9e64dfdb768...
Message was sent while issue was closed.
On 2017/07/13 14:26:48, commit-bot: I haz the power wrote: > Committed patchset #27 (id:510001) as > https://chromium.googlesource.com/chromium/src/+/28c8676d89927c00b9e64dfdb768... Looks like this is causing failures on RenderProcessHostTest.SpareRendererOnProcessReuse failing flakily on Linux TSan Tests; see https://bugs.chromium.org/p/chromium/issues/detail?id=742533 Does this seem right? This seems like the most likely suspect, but the failure is only on one builder; I'd like to try to revert to see if that fixes the issue on the TSan bot...
Message was sent while issue was closed.
On 2017/07/13 21:37:00, qyearsley wrote: > On 2017/07/13 14:26:48, commit-bot: I haz the power wrote: > > Committed patchset #27 (id:510001) as > > > https://chromium.googlesource.com/chromium/src/+/28c8676d89927c00b9e64dfdb768... > > Looks like this is causing failures on > RenderProcessHostTest.SpareRendererOnProcessReuse failing flakily on Linux TSan > Tests; see https://bugs.chromium.org/p/chromium/issues/detail?id=742533 > > Does this seem right? This seems like the most likely suspect, but the failure > is only on one builder; I'd like to try to revert to see if that fixes the issue > on the TSan bot... Yes the revert seems correct because the SpareRendererOnProcessReuse test was added in this patch. |
