Issue 2857213005: PlzNavigate: implement process reuse for ServiceWorkers

clamy

Description was changed from ========== PlzNavigate: implement process reuse for ServiceWorkers This Cl implements a ...

3 years, 7 months ago (2017-05-04 15:54:50 UTC) #1

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-04 15:55:21 UTC) #2

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/1

3 years, 7 months ago (2017-05-04 15:55:45 UTC) #3

clamy

Description was changed from ========== PlzNavigate: implement process reuse for ServiceWorkers This Cl implements a ...

3 years, 7 months ago (2017-05-04 15:56:40 UTC) #4

clamy

clamy@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org

3 years, 7 months ago (2017-05-04 15:56:40 UTC) #5

clamy

@nasko, creis: PTAL. This is a first draft of what the process reuse policy for ...

3 years, 7 months ago (2017-05-04 16:01:01 UTC) #6

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-04 16:27:01 UTC) #7

commit-bot: I haz the power

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_chromeos_ozone_rel_ng/builds/376519)

3 years, 7 months ago (2017-05-04 16:27:02 UTC) #8

Charlie Reis

Thanks. I'm going to need more time reviewing this one.

3 years, 7 months ago (2017-05-05 00:03:43 UTC) #9

nasko

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/navigation_handle_impl.cc#newcode622 content/browser/frame_host/navigation_handle_impl.cc:622: speculative_render_process_host_id_ = kInvalidRenderProcessHostId; Shouldn't speculative_render_process_host_id_ be cleared in both ...

3 years, 7 months ago (2017-05-05 05:22:00 UTC) #10

clamy

Thanks! A few answers below (I don't have time for a new patchset before going ...

3 years, 7 months ago (2017-05-05 15:10:11 UTC) #11

Thanks! A few answers below (I don't have time for a new patchset before going
ooo).

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:622:
speculative_render_process_host_id_ = kInvalidRenderProcessHostId;
On 2017/05/05 05:21:59, nasko wrote:
> Shouldn't speculative_render_process_host_id_ be cleared in both of these
cases?
> Once old site and new site don't agree, we know we will be going to a
different
> process.

Not in renderer-initiated navigations: we match the current process model and
don't switch process in that case.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:583:
std::map<std::string, int>& counts_per_site = map_[render_process_host];
On 2017/05/05 05:22:00, nasko wrote:
> I assume you are using operator[] intentionally here to add an entry to the
map
> if one does not exist. Correct?

Yes. We want to create one if we don't have one, and in that case [] looks
cleaner IMO.

https://codereview.chromium.org/2857213005/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_process_manager.cc:224:
REUSE_CHECKING_FRAMES_AND_NAVIGATIONS);
On 2017/05/05 05:22:00, nasko wrote:
> Reading this code, I wonder if it will be more natural to pass the policy as
> parameter to GetProcess() and it can set it internally.

Except it could have already been set in other cases before we get to
GetProcess.

Maybe we could have GetProcessWithReusePolicy(policy) that sets the policy then
does GetProcess?

Charlie Reis

Ok! I've had some time to think through this, and I've got a few suggestions ...

3 years, 7 months ago (2017-05-15 03:41:51 UTC) #12

Ok!  I've had some time to think through this, and I've got a few suggestions
below for how we might address some potential issues I noticed.  On the whole,
though, I'm excited to be able to SiteInstance for the ServiceWorker case, and
there could be some additional benefits as well.

As we move forward, I would really recommend more of a test-first approach here.
 We want to think about how this logic should behave ahead of time, to be sure
that we know we're building the right thing.  Some cases to cover:
 - Sites that match the SiteInstance's site URL
 - Sites that don't match SiteInstance's site URL
 - NTP (to confirm that site URLs will make it work)
 - Hosted apps (including uninstalling while open)
We should also have tests that cover the difference between the pending and
committed cases, if we don't already.

I'd also suggest a sanity checker for balancing ref counts.  Can we have a
DCHECK somewhere saying that all the refcounts have gone to zero when a process
exits cleanly, for example?  Or something similar?

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:46: const int
kInvalidRenderProcessHostId = -1;
We should use ChildProcessHost::kInvalidUniqueID when comparing against RPH IDs.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:622:
speculative_render_process_host_id_ = kInvalidRenderProcessHostId;
On 2017/05/05 15:10:11, clamy (ooo until May 15) wrote:
> On 2017/05/05 05:21:59, nasko wrote:
> > Shouldn't speculative_render_process_host_id_ be cleared in both of these
> cases?
> > Once old site and new site don't agree, we know we will be going to a
> different
> > process.
> 
> Not in renderer-initiated navigations: we match the current process model and
> don't switch process in that case.

Yes, the confusion here is part of my concern.  See my suggestion about
listening more directly to speculative RFH changes rather than trying to infer
it here indirectly.

(We might not need most of the NavHandleImpl changes if so.)

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:727: if
(speculative_render_process_host_id_ != kInvalidRenderProcessHostId) {
nit: Redundant with should_inform_final_process

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:797: if (!process)
This seems like it should be an error, rather than an early return where we
ignore it.

Actually, why not just pass in the RenderProcessHost instead?  We have it at the
call site, so we shouldn't need to look it up again.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:388: // just inform it to
stop expecting a navigation.
The should_update_on_redirects parameter seems kind of indirect to me, and I'm
not sure if it's correct.  If I understand correctly, it's meant to indicate
whether we'll do a process swap for a redirect?

We're using may_transfer_ for that, which (from our discussion on
https://codereview.chromium.org/2768813002/) is really about whether it's
from_begin_navigation(), right?  But that's not a good predictor of whether
there will be a process swap.  First, BeginNavigation cases can still swap
processes on redirect, such as link clicks on URLs that redirect to the Chrome
Web Store.  Second, the process swap logic is fairly complex and can't be boiled
down to just whether it's cross-site.

I think this means we don't want to have to guess whether a speculative RFH has
changed by listening to redirects.  Instead, we should have a more explicit
notification that the speculative RFH has changed, and use that to update the
RPH state.  Maybe even something closer to that code should be the place where
we tell RPH, rather than NavigationHandleImpl?

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:721:
navigating_frame_host->GetProcess()->GetID(), !may_transfer_);
Yes, let's revisit https://codereview.chromium.org/2768813002/ or even land a
separate CL to rename may_transfer_, if you prefer.  I think this would be
another case where it's a bit hard to map the concepts onto each other.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:479: if
(!last_committed_url_.is_empty()) {
I wonder whether using last_committed_url_ here is safe, since it has a few
issues.

First, we update it even for error pages, which might be a bit misleading.  We
wouldn't want a ServiceWorker to join a process for site A in the case that the
process only had a blocked navigation attempt to A.

Second, I worry about the difference between tracking URLs at this level but
site URLs under the hood.  One of the main problems is the GetSiteURL
transformation can change over time-- suppose a hosted app is uninstalled after
visiting one of its URLs but before navigating away.  GetSiteURL will return a
chrome-extension:// URL when we commit it, but a normal web URL when we leave. 
That will break our site refcounting.

Maybe we should be keeping track of a last_committed_site_url_ on
RenderFrameHostImpl?  I suppose it's a bit crazy to have URL, Origin, and site
URL all on the same class, but there's legitimate reasons for knowing each of
them.  It would stay stable even if a hosted app were uninstalled, and it would
be empty for error pages.  It might also make some of the APIs we're adding here
a bit easier, since they can expect to receive site URLs rather than having to
compute them internally.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1173: if
(!last_committed_url_.is_empty()) {
On 2017/05/05 05:22:00, nasko wrote:
> Since we are registering only origins, should we extract the origin here and
> only update the RPH if the origin of the previous commit and this one differ?
> This will avoid all the lookups performed in the new code. For example, pages
> like http://twitter.com, which use pushState quite extensively will avoid the
overhead
> on each commit.
> In addition, the RPH methods expect site URL, so passing last committed URL
here
> seems incorrect.

Yes, I agree that we need to be careful about what we're passing, and when it
gets converted to site URLs (as I noted above).

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:571: const void* const
kFrameSiteCountPerProcessTrackerKey =
I think pending and committed are probably better terms than frames and
expecting navigations.  We already use those terms elsewhere, plus we're not
keeping a record of which frames the sites are in (as I initially thought from
the name).

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:575: class
SiteCountPerProcessTracker : public base::SupportsUserData::Data,
On 2017/05/05 05:22:00, nasko wrote:
> On 2017/05/04 16:01:01, clamy wrote:
> > This class + data structure looks a bit weird. This is because of the split
> > RenderProcessHostImpl/MockRenderProcessHost which prevents from storing this
> > kind of state in the RenderProcessHostImpl.
> 
> Yuck! Yet another reason to hate MockRenderProcessHost. I wonder if both
classes
> can just inherit a this as a base class.

If we really want per-RPH state, we should find a way to make it happen without
this indirect approach.

However, it seems to me that we don't really need that anyway.  The query API
seems to be, "hey BrowserContext, give me the processes being used for this
particular site."  We might be able to structure the data that way to make the
lookup easier (i.e., without needing to iterate over the processes).

That is, rather than:
  map( process -> map(site -> count) )

  for each process:
    if process has site, add it to candidates
  return candidates

We instead do:
  map( site -> map( process -> count) )

  look up site
  return its set of processes (i.e., keys of the map)

Then it makes sense to have the map associated with BrowserContext, and we don't
need to do any iteration over the entire list of processes for lookup.  It would
probably be about the same complexity for adds and removes, as well.

The tradeoff is that we couldn't easily ask a process which sites it contains
(e.g., for security checks).  But the truth is, we can't really use this data
structure for such security checks anyway, at least as it stands.  We're
removing the old site when a new navigation commits, even though the old site's
unload handler may continue to run in the background (for cross-process
navigations).  That means the browser process may see actions from the old site
after this data structure no longer has a record of it being there.  That's fine
for process-per-site logic, but not for determining whether the process is
allowed to access data from the old site.  (In fact, we might want to mention
this limitation in comments on the data structure declaration.)

Does that sound like a worthwhile change?

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:317: const GURL&
site_url);
Note: If we call these parameters |site_url|, then we should expect them to
already be site URLs when passed in (rather than calling GetSiteURL internally).
 That might be easier if we track last_committed_site_url_ on RFHI.

https://codereview.chromium.org/2857213005/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_process_manager.cc:224:
REUSE_CHECKING_FRAMES_AND_NAVIGATIONS);
On 2017/05/05 15:10:11, clamy (ooo until May 15) wrote:
> On 2017/05/05 05:22:00, nasko wrote:
> > Reading this code, I wonder if it will be more natural to pass the policy as
> > parameter to GetProcess() and it can set it internally.
> 
> Except it could have already been set in other cases before we get to
> GetProcess.
> 
> Maybe we could have GetProcessWithReusePolicy(policy) that sets the policy
then
> does GetProcess?

Just curious, what's the advantage of either of these approaches?  I think
having a separate set_process_reuse_policy seems ok.

https://codereview.chromium.org/2857213005/diff/1/content/browser/site_instan...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/site_instan...
content/browser/site_instance_impl.h:71: REUSE_CHECKING_FRAMES_AND_NAVIGATIONS,
I'll suggest REUSE_PENDING_OR_COMMITTED_SITE.  That way "site" is in the name
(as the criteria for reuse), and pending vs committed seems like a more direct
comparison than "frames" vs "navigations" (which are independent concepts).

The comment can be slightly rephrased to convey this (and since "in priority"
wasn't explained):

In this mode, the site will be rendered in a RenderProcessHost that is already
in use for the site, either for a pending navigation or a committed navigation. 
If none exists, a new process will be created.  If multiple such processes
exist, ones that have foreground frames are given priority, and otherwise one is
selected randomly.

(As a side note, I think we might actually be able to use this exact behavior
for PROCESS_PER_SITE.  The main difference is using an existing process if a
navigation is pending, and I think that may actually fix a race in
process-per-site rather than cause problems.  That's for a followup CL, though,
and we should think it through.)

clamy

On 2017/05/15 03:41:51, Charlie Reis wrote: > Ok! I've had some time to think through ...

3 years, 7 months ago (2017-05-15 15:20:48 UTC) #13

clamy

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/navigation_handle_impl.cc#newcode622 content/browser/frame_host/navigation_handle_impl.cc:622: speculative_render_process_host_id_ = kInvalidRenderProcessHostId; On 2017/05/15 03:41:52, Charlie Reis wrote: ...

3 years, 7 months ago (2017-05-15 15:21:15 UTC) #14

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:622:
speculative_render_process_host_id_ = kInvalidRenderProcessHostId;
On 2017/05/15 03:41:52, Charlie Reis wrote:
> On 2017/05/05 15:10:11, clamy (ooo until May 15) wrote:
> > On 2017/05/05 05:21:59, nasko wrote:
> > > Shouldn't speculative_render_process_host_id_ be cleared in both of these
> > cases?
> > > Once old site and new site don't agree, we know we will be going to a
> > different
> > > process.
> > 
> > Not in renderer-initiated navigations: we match the current process model
and
> > don't switch process in that case.
> 
> Yes, the confusion here is part of my concern.  See my suggestion about
> listening more directly to speculative RFH changes rather than trying to infer
> it here indirectly.
> 
> (We might not need most of the NavHandleImpl changes if so.)

The problem with that is that we don't execute RFH change logic on redirects. I
would probably need to factor out the code that determines whether we are going
to change in RFHM so that it can be called without actually triggering the
change. Since it's a bit complicated, I think I'll aim for a first version here
that always clear the navigation tracking if we get a cross-site redirect. Then
I'll look into doing the clear more selectively.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:575: class
SiteCountPerProcessTracker : public base::SupportsUserData::Data,
On 2017/05/15 03:41:52, Charlie Reis wrote:
> On 2017/05/05 05:22:00, nasko wrote:
> > On 2017/05/04 16:01:01, clamy wrote:
> > > This class + data structure looks a bit weird. This is because of the
split
> > > RenderProcessHostImpl/MockRenderProcessHost which prevents from storing
this
> > > kind of state in the RenderProcessHostImpl.
> > 
> > Yuck! Yet another reason to hate MockRenderProcessHost. I wonder if both
> classes
> > can just inherit a this as a base class.
> 
> If we really want per-RPH state, we should find a way to make it happen
without
> this indirect approach.
> 
> However, it seems to me that we don't really need that anyway.  The query API
> seems to be, "hey BrowserContext, give me the processes being used for this
> particular site."  We might be able to structure the data that way to make the
> lookup easier (i.e., without needing to iterate over the processes).
> 
> That is, rather than:
>   map( process -> map(site -> count) )
> 
>   for each process:
>     if process has site, add it to candidates
>   return candidates
> 
> We instead do:
>   map( site -> map( process -> count) )
> 
>   look up site
>   return its set of processes (i.e., keys of the map)
> 
> Then it makes sense to have the map associated with BrowserContext, and we
don't
> need to do any iteration over the entire list of processes for lookup.  It
would
> probably be about the same complexity for adds and removes, as well.
> 
> The tradeoff is that we couldn't easily ask a process which sites it contains
> (e.g., for security checks).  But the truth is, we can't really use this data
> structure for such security checks anyway, at least as it stands.  We're
> removing the old site when a new navigation commits, even though the old
site's
> unload handler may continue to run in the background (for cross-process
> navigations).  That means the browser process may see actions from the old
site
> after this data structure no longer has a record of it being there.  That's
fine
> for process-per-site logic, but not for determining whether the process is
> allowed to access data from the old site.  (In fact, we might want to mention
> this limitation in comments on the data structure declaration.)
> 
> Does that sound like a worthwhile change?

This is how I had written it initially. However, while it makes querying
simpler, it makes the RenderProcessHostDestroyed function below more complex. We
now have to iterate over all sites in the map to remove the rph from it, instead
of just erasing the map of sites belonging to the process. I reasoned that we
were more likely to call RenderProcessHostDestroyed than
FindRenderProcessesForSite, so it made sense to make RenderProcessHostDestroyed
the simpler function. I can change it back the other way if you want.

Charlie Reis

Thanks! A few more thoughts below. https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/navigation_handle_impl.cc#newcode622 content/browser/frame_host/navigation_handle_impl.cc:622: speculative_render_process_host_id_ = kInvalidRenderProcessHostId; ...

3 years, 7 months ago (2017-05-15 23:46:18 UTC) #15

Thanks!  A few more thoughts below.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:622:
speculative_render_process_host_id_ = kInvalidRenderProcessHostId;
On 2017/05/15 15:21:15, clamy (ooo until May 25) wrote:
> On 2017/05/15 03:41:52, Charlie Reis wrote:
> > On 2017/05/05 15:10:11, clamy (ooo until May 15) wrote:
> > > On 2017/05/05 05:21:59, nasko wrote:
> > > > Shouldn't speculative_render_process_host_id_ be cleared in both of
these
> > > cases?
> > > > Once old site and new site don't agree, we know we will be going to a
> > > different
> > > > process.
> > > 
> > > Not in renderer-initiated navigations: we match the current process model
> and
> > > don't switch process in that case.
> > 
> > Yes, the confusion here is part of my concern.  See my suggestion about
> > listening more directly to speculative RFH changes rather than trying to
infer
> > it here indirectly.
> > 
> > (We might not need most of the NavHandleImpl changes if so.)
> 
> The problem with that is that we don't execute RFH change logic on redirects.
I
> would probably need to factor out the code that determines whether we are
going
> to change in RFHM so that it can be called without actually triggering the
> change. Since it's a bit complicated, I think I'll aim for a first version
here
> that always clear the navigation tracking if we get a cross-site redirect.
Then
> I'll look into doing the clear more selectively.

Can you put some more detail on this redirect case into the design doc for us to
discuss more there?  I'm curious about the timeline of when speculative RFHs
change.

For example, suppose we have a redirect chain like A -> B -> C -> D, and each
has a ServiceWorker.  Presumably we create a speculative RFH for A.  Do we ever
change it for B and C, or do we wait until OnResponseStarted for D to possibly
replace it?  And in the BeginNavigation case with no process swaps, presumably
there's no speculative RFH at all, unless we get to D and find it requires a
process swap (e.g., CWS)?

I'm curious which process we should put the B and C ServiceWorkers in, even if
we had perfect knowledge.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:575: class
SiteCountPerProcessTracker : public base::SupportsUserData::Data,
On 2017/05/15 15:21:15, clamy (ooo until May 25) wrote:
> On 2017/05/15 03:41:52, Charlie Reis wrote:
> > On 2017/05/05 05:22:00, nasko wrote:
> > > On 2017/05/04 16:01:01, clamy wrote:
> > > > This class + data structure looks a bit weird. This is because of the
> split
> > > > RenderProcessHostImpl/MockRenderProcessHost which prevents from storing
> this
> > > > kind of state in the RenderProcessHostImpl.
> > > 
> > > Yuck! Yet another reason to hate MockRenderProcessHost. I wonder if both
> > classes
> > > can just inherit a this as a base class.
> > 
> > If we really want per-RPH state, we should find a way to make it happen
> without
> > this indirect approach.
> > 
> > However, it seems to me that we don't really need that anyway.  The query
API
> > seems to be, "hey BrowserContext, give me the processes being used for this
> > particular site."  We might be able to structure the data that way to make
the
> > lookup easier (i.e., without needing to iterate over the processes).
> > 
> > That is, rather than:
> >   map( process -> map(site -> count) )
> > 
> >   for each process:
> >     if process has site, add it to candidates
> >   return candidates
> > 
> > We instead do:
> >   map( site -> map( process -> count) )
> > 
> >   look up site
> >   return its set of processes (i.e., keys of the map)
> > 
> > Then it makes sense to have the map associated with BrowserContext, and we
> don't
> > need to do any iteration over the entire list of processes for lookup.  It
> would
> > probably be about the same complexity for adds and removes, as well.
> > 
> > The tradeoff is that we couldn't easily ask a process which sites it
contains
> > (e.g., for security checks).  But the truth is, we can't really use this
data
> > structure for such security checks anyway, at least as it stands.  We're
> > removing the old site when a new navigation commits, even though the old
> site's
> > unload handler may continue to run in the background (for cross-process
> > navigations).  That means the browser process may see actions from the old
> site
> > after this data structure no longer has a record of it being there.  That's
> fine
> > for process-per-site logic, but not for determining whether the process is
> > allowed to access data from the old site.  (In fact, we might want to
mention
> > this limitation in comments on the data structure declaration.)
> > 
> > Does that sound like a worthwhile change?
> 
> This is how I had written it initially. However, while it makes querying
> simpler, it makes the RenderProcessHostDestroyed function below more complex.
We
> now have to iterate over all sites in the map to remove the rph from it,
instead
> of just erasing the map of sites belonging to the process. I reasoned that we
> were more likely to call RenderProcessHostDestroyed than
> FindRenderProcessesForSite, so it made sense to make
RenderProcessHostDestroyed
> the simpler function. I can change it back the other way if you want.

Interesting!  Yes, I hadn't thought about the process exit case.

Just trying to think of some alternatives, to help us avoid the awkwardness of
storing per-process data with BrowserContext SupportsUserData.

1) It doesn't look like there's an easy way to get to all the frames for a given
RenderProcessHost.  It knows its set of widgets, but you can't easily get the
frames for a widget.  Otherwise this would let us directly remove the process
from the relevant sites when it exits.

2) Could we avoid updating the data structure when the process exits, and
instead filter out non-live RenderProcessHosts when doing lookups?  The
RenderFrameHosts for the process are still around.  (Weird, though-- we seem to
be clearing last_committed_url_ but not last_committed_origin_ when the process
exits.  That doesn't seem great.  We should probably be consistent.)

3) Kind of like #2, what if we did clear the last_committed_site_ on RFH when
the process exits, and *that's* where we could update the data structure?

How bad would #3 be?  We can keep it as process -> site -> count if needed, but
if we can make site -> process -> count work, it seems like it would be nicer
for lookup, make more sense to have associated with BrowserContext, and
discourage people from misusing it for security checks (given the unload issue).

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-16 14:48:05 UTC) #16

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/40001

3 years, 7 months ago (2017-05-16 14:48:25 UTC) #17

clamy

Thanks! I have a new patch set up and I am working on adding unit ...

3 years, 7 months ago (2017-05-16 14:50:46 UTC) #18

Thanks! I have a new patch set up and I am working on adding unit tests. Will
upload a patch set with the unit tests when I am done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:46: const int
kInvalidRenderProcessHostId = -1;
On 2017/05/15 03:41:52, Charlie Reis wrote:
> We should use ChildProcessHost::kInvalidUniqueID when comparing against RPH
IDs.

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:622:
speculative_render_process_host_id_ = kInvalidRenderProcessHostId;
On 2017/05/15 15:21:15, clamy (ooo until May 15) wrote:
> On 2017/05/15 03:41:52, Charlie Reis wrote:
> > On 2017/05/05 15:10:11, clamy (ooo until May 15) wrote:
> > > On 2017/05/05 05:21:59, nasko wrote:
> > > > Shouldn't speculative_render_process_host_id_ be cleared in both of
these
> > > cases?
> > > > Once old site and new site don't agree, we know we will be going to a
> > > different
> > > > process.
> > > 
> > > Not in renderer-initiated navigations: we match the current process model
> and
> > > don't switch process in that case.
> > 
> > Yes, the confusion here is part of my concern.  See my suggestion about
> > listening more directly to speculative RFH changes rather than trying to
infer
> > it here indirectly.
> > 
> > (We might not need most of the NavHandleImpl changes if so.)
> 
> The problem with that is that we don't execute RFH change logic on redirects.
I
> would probably need to factor out the code that determines whether we are
going
> to change in RFHM so that it can be called without actually triggering the
> change. Since it's a bit complicated, I think I'll aim for a first version
here
> that always clear the navigation tracking if we get a cross-site redirect.
Then
> I'll look into doing the clear more selectively.

I am now tracking the site url and updating it from there. The code in
UpdateSiteURL will deal with the updating of the RPH.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:622:
speculative_render_process_host_id_ = kInvalidRenderProcessHostId;
On 2017/05/15 23:46:18, Charlie Reis wrote:
> On 2017/05/15 15:21:15, clamy (ooo until May 25) wrote:
> > On 2017/05/15 03:41:52, Charlie Reis wrote:
> > > On 2017/05/05 15:10:11, clamy (ooo until May 15) wrote:
> > > > On 2017/05/05 05:21:59, nasko wrote:
> > > > > Shouldn't speculative_render_process_host_id_ be cleared in both of
> these
> > > > cases?
> > > > > Once old site and new site don't agree, we know we will be going to a
> > > > different
> > > > > process.
> > > > 
> > > > Not in renderer-initiated navigations: we match the current process
model
> > and
> > > > don't switch process in that case.
> > > 
> > > Yes, the confusion here is part of my concern.  See my suggestion about
> > > listening more directly to speculative RFH changes rather than trying to
> infer
> > > it here indirectly.
> > > 
> > > (We might not need most of the NavHandleImpl changes if so.)
> > 
> > The problem with that is that we don't execute RFH change logic on
redirects.
> I
> > would probably need to factor out the code that determines whether we are
> going
> > to change in RFHM so that it can be called without actually triggering the
> > change. Since it's a bit complicated, I think I'll aim for a first version
> here
> > that always clear the navigation tracking if we get a cross-site redirect.
> Then
> > I'll look into doing the clear more selectively.
> 
> Can you put some more detail on this redirect case into the design doc for us
to
> discuss more there?  I'm curious about the timeline of when speculative RFHs
> change.
> 
> For example, suppose we have a redirect chain like A -> B -> C -> D, and each
> has a ServiceWorker.  Presumably we create a speculative RFH for A.  Do we
ever
> change it for B and C, or do we wait until OnResponseStarted for D to possibly
> replace it?  And in the BeginNavigation case with no process swaps, presumably
> there's no speculative RFH at all, unless we get to D and find it requires a
> process swap (e.g., CWS)?
> 
> I'm curious which process we should put the B and C ServiceWorkers in, even if
> we had perfect knowledge.

Ok I'll update the design doc so we can follow up on the redirect case.

In the case we mention, we don't change the speculative RFH for B & C. We
thought about it but decided it was probably wasteful.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:727: if
(speculative_render_process_host_id_ != kInvalidRenderProcessHostId) {
On 2017/05/15 03:41:52, Charlie Reis wrote:
> nit: Redundant with should_inform_final_process

I have removed this part in favor of calling SetSpeculativeProcessHostID again.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:797: if (!process)
On 2017/05/15 03:41:52, Charlie Reis wrote:
> This seems like it should be an error, rather than an early return where we
> ignore it.
> 
> Actually, why not just pass in the RenderProcessHost instead?  We have it at
the
> call site, so we shouldn't need to look it up again.

This function now takes the RenderProcessHost and expects to be called multiple
times so it has changed quite a bit.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:388: // just inform it to
stop expecting a navigation.
On 2017/05/15 03:41:52, Charlie Reis wrote:
> The should_update_on_redirects parameter seems kind of indirect to me, and I'm
> not sure if it's correct.  If I understand correctly, it's meant to indicate
> whether we'll do a process swap for a redirect?
> 
> We're using may_transfer_ for that, which (from our discussion on
> https://codereview.chromium.org/2768813002/) is really about whether it's
> from_begin_navigation(), right?  But that's not a good predictor of whether
> there will be a process swap.  First, BeginNavigation cases can still swap
> processes on redirect, such as link clicks on URLs that redirect to the Chrome
> Web Store.  Second, the process swap logic is fairly complex and can't be
boiled
> down to just whether it's cross-site.
> 
> I think this means we don't want to have to guess whether a speculative RFH
has
> changed by listening to redirects.  Instead, we should have a more explicit
> notification that the speculative RFH has changed, and use that to update the
> RPH state.  Maybe even something closer to that code should be the place where
> we tell RPH, rather than NavigationHandleImpl?

I have removed this part of the patch. For now, I'd prefer to keep the tracking
of navigations in NavigationhandleImpl as this is where the lifetime of
navigations is better tracked.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:721:
navigating_frame_host->GetProcess()->GetID(), !may_transfer_);
On 2017/05/15 03:41:52, Charlie Reis wrote:
> Yes, let's revisit https://codereview.chromium.org/2768813002/ or even land a
> separate CL to rename may_transfer_, if you prefer.  I think this would be
> another case where it's a bit hard to map the concepts onto each other.

The parameter was removed, though we should revisit
https://codereview.chromium.org/2768813002/ with a rename.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:479: if
(!last_committed_url_.is_empty()) {
On 2017/05/15 03:41:52, Charlie Reis wrote:
> I wonder whether using last_committed_url_ here is safe, since it has a few
> issues.
> 
> First, we update it even for error pages, which might be a bit misleading.  We
> wouldn't want a ServiceWorker to join a process for site A in the case that
the
> process only had a blocked navigation attempt to A.
> 
> Second, I worry about the difference between tracking URLs at this level but
> site URLs under the hood.  One of the main problems is the GetSiteURL
> transformation can change over time-- suppose a hosted app is uninstalled
after
> visiting one of its URLs but before navigating away.  GetSiteURL will return a
> chrome-extension:// URL when we commit it, but a normal web URL when we leave.

> That will break our site refcounting.
> 
> Maybe we should be keeping track of a last_committed_site_url_ on
> RenderFrameHostImpl?  I suppose it's a bit crazy to have URL, Origin, and site
> URL all on the same class, but there's legitimate reasons for knowing each of
> them.  It would stay stable even if a hosted app were uninstalled, and it
would
> be empty for error pages.  It might also make some of the APIs we're adding
here
> a bit easier, since they can expect to receive site URLs rather than having to
> compute them internally.

I'm now storing the last_committed_site_url_. I'm updating it at the end of
DidCommitProvisionalLoad when the navigation was successful, which should
exclude error pages. Let me know if you think it'd be best to update it
somewhere else.

https://codereview.chromium.org/2857213005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1173: if
(!last_committed_url_.is_empty()) {
On 2017/05/15 03:41:52, Charlie Reis wrote:
> On 2017/05/05 05:22:00, nasko wrote:
> > Since we are registering only origins, should we extract the origin here and
> > only update the RPH if the origin of the previous commit and this one
differ?
> > This will avoid all the lookups performed in the new code. For example,
pages
> > like http://twitter.com, which use pushState quite extensively will avoid
the
> overhead
> > on each commit.
> > In addition, the RPH methods expect site URL, so passing last committed URL
> here
> > seems incorrect.
> 
> Yes, I agree that we need to be careful about what we're passing, and when it
> gets converted to site URLs (as I noted above).

I'm now storing the site_url as suggested above, so this moved to
SetlastCommittedSiteUrl. It also only updates the RPH if the site url changes.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:571: const void* const
kFrameSiteCountPerProcessTrackerKey =
On 2017/05/15 03:41:52, Charlie Reis wrote:
> I think pending and committed are probably better terms than frames and
> expecting navigations.  We already use those terms elsewhere, plus we're not
> keeping a record of which frames the sites are in (as I initially thought from
> the name).

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:575: class
SiteCountPerProcessTracker : public base::SupportsUserData::Data,
On 2017/05/15 23:46:18, Charlie Reis wrote:
> On 2017/05/15 15:21:15, clamy (ooo until May 25) wrote:
> > On 2017/05/15 03:41:52, Charlie Reis wrote:
> > > On 2017/05/05 05:22:00, nasko wrote:
> > > > On 2017/05/04 16:01:01, clamy wrote:
> > > > > This class + data structure looks a bit weird. This is because of the
> > split
> > > > > RenderProcessHostImpl/MockRenderProcessHost which prevents from
storing
> > this
> > > > > kind of state in the RenderProcessHostImpl.
> > > > 
> > > > Yuck! Yet another reason to hate MockRenderProcessHost. I wonder if both
> > > classes
> > > > can just inherit a this as a base class.
> > > 
> > > If we really want per-RPH state, we should find a way to make it happen
> > without
> > > this indirect approach.
> > > 
> > > However, it seems to me that we don't really need that anyway.  The query
> API
> > > seems to be, "hey BrowserContext, give me the processes being used for
this
> > > particular site."  We might be able to structure the data that way to make
> the
> > > lookup easier (i.e., without needing to iterate over the processes).
> > > 
> > > That is, rather than:
> > >   map( process -> map(site -> count) )
> > > 
> > >   for each process:
> > >     if process has site, add it to candidates
> > >   return candidates
> > > 
> > > We instead do:
> > >   map( site -> map( process -> count) )
> > > 
> > >   look up site
> > >   return its set of processes (i.e., keys of the map)
> > > 
> > > Then it makes sense to have the map associated with BrowserContext, and we
> > don't
> > > need to do any iteration over the entire list of processes for lookup.  It
> > would
> > > probably be about the same complexity for adds and removes, as well.
> > > 
> > > The tradeoff is that we couldn't easily ask a process which sites it
> contains
> > > (e.g., for security checks).  But the truth is, we can't really use this
> data
> > > structure for such security checks anyway, at least as it stands.  We're
> > > removing the old site when a new navigation commits, even though the old
> > site's
> > > unload handler may continue to run in the background (for cross-process
> > > navigations).  That means the browser process may see actions from the old
> > site
> > > after this data structure no longer has a record of it being there. 
That's
> > fine
> > > for process-per-site logic, but not for determining whether the process is
> > > allowed to access data from the old site.  (In fact, we might want to
> mention
> > > this limitation in comments on the data structure declaration.)
> > > 
> > > Does that sound like a worthwhile change?
> > 
> > This is how I had written it initially. However, while it makes querying
> > simpler, it makes the RenderProcessHostDestroyed function below more
complex.
> We
> > now have to iterate over all sites in the map to remove the rph from it,
> instead
> > of just erasing the map of sites belonging to the process. I reasoned that
we
> > were more likely to call RenderProcessHostDestroyed than
> > FindRenderProcessesForSite, so it made sense to make
> RenderProcessHostDestroyed
> > the simpler function. I can change it back the other way if you want.
> 
> Interesting!  Yes, I hadn't thought about the process exit case.
> 
> Just trying to think of some alternatives, to help us avoid the awkwardness of
> storing per-process data with BrowserContext SupportsUserData.
> 
> 1) It doesn't look like there's an easy way to get to all the frames for a
given
> RenderProcessHost.  It knows its set of widgets, but you can't easily get the
> frames for a widget.  Otherwise this would let us directly remove the process
> from the relevant sites when it exits.
> 
> 2) Could we avoid updating the data structure when the process exits, and
> instead filter out non-live RenderProcessHosts when doing lookups?  The
> RenderFrameHosts for the process are still around.  (Weird, though-- we seem
to
> be clearing last_committed_url_ but not last_committed_origin_ when the
process
> exits.  That doesn't seem great.  We should probably be consistent.)
> 
> 3) Kind of like #2, what if we did clear the last_committed_site_ on RFH when
> the process exits, and *that's* where we could update the data structure?
> 
> How bad would #3 be?  We can keep it as process -> site -> count if needed,
but
> if we can make site -> process -> count work, it seems like it would be nicer
> for lookup, make more sense to have associated with BrowserContext, and
> discourage people from misusing it for security checks (given the unload
issue).

I've reversed the map as asked, and remove the observer part of the class. I was
a bit worried of storing stale pointers, so I moved to storing ids instead,
allowing to dcheck that we do get a RPH when we want to use it. Wdyt?

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:582: std::string site)
{
On 2017/05/05 05:22:00, nasko (out) wrote:
> Why accept the site as a string and not a GURL?

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:591:
std::map<std::string, int>& counts_per_site = map_[render_process_host];
On 2017/05/05 05:22:00, nasko (out) wrote:
> Shouldn't this be map_.find()?

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1753:
.possibly_invalid_spec();
On 2017/05/05 05:22:00, nasko (out) wrote:
> Why possibly_invalid_spec()? Shouldn't an non-empty GURL returned from this
> method be correctly formatted GURL?

It's now taking a GURL.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:3331: // Add the
RenderProcessHosts hosting a frame rendering site to the list of
On 2017/05/05 05:22:00, nasko (out) wrote:
> nit: "... hosting a frame for |site| to ..."

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:3352: int index =
rand() % eligible_foreground_hosts.size();
On 2017/05/05 05:22:00, nasko (out) wrote:
> base::RandInt

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:3360: int index =
rand() % eligible_background_hosts.size();
On 2017/05/05 05:22:00, nasko (out) wrote:
> base::RandInt

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:317: const GURL&
site_url);
On 2017/05/15 03:41:52, Charlie Reis wrote:
> Note: If we call these parameters |site_url|, then we should expect them to
> already be site URLs when passed in (rather than calling GetSiteURL
internally).
>  That might be easier if we track last_committed_site_url_ on RFHI.

Done.

https://codereview.chromium.org/2857213005/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_process_manager.cc:224:
REUSE_CHECKING_FRAMES_AND_NAVIGATIONS);
On 2017/05/15 03:41:52, Charlie Reis wrote:
> On 2017/05/05 15:10:11, clamy (ooo until May 15) wrote:
> > On 2017/05/05 05:22:00, nasko wrote:
> > > Reading this code, I wonder if it will be more natural to pass the policy
as
> > > parameter to GetProcess() and it can set it internally.
> > 
> > Except it could have already been set in other cases before we get to
> > GetProcess.
> > 
> > Maybe we could have GetProcessWithReusePolicy(policy) that sets the policy
> then
> > does GetProcess?
> 
> Just curious, what's the advantage of either of these approaches?  I think
> having a separate set_process_reuse_policy seems ok.

I'm leaving it as is for now.

https://codereview.chromium.org/2857213005/diff/1/content/browser/site_instan...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/2857213005/diff/1/content/browser/site_instan...
content/browser/site_instance_impl.h:71: REUSE_CHECKING_FRAMES_AND_NAVIGATIONS,
On 2017/05/15 03:41:52, Charlie Reis wrote:
> I'll suggest REUSE_PENDING_OR_COMMITTED_SITE.  That way "site" is in the name
> (as the criteria for reuse), and pending vs committed seems like a more direct
> comparison than "frames" vs "navigations" (which are independent concepts).
> 
> The comment can be slightly rephrased to convey this (and since "in priority"
> wasn't explained):
> 
> In this mode, the site will be rendered in a RenderProcessHost that is already
> in use for the site, either for a pending navigation or a committed
navigation. 
> If none exists, a new process will be created.  If multiple such processes
> exist, ones that have foreground frames are given priority, and otherwise one
is
> selected randomly.
> 
> (As a side note, I think we might actually be able to use this exact behavior
> for PROCESS_PER_SITE.  The main difference is using an existing process if a
> navigation is pending, and I think that may actually fix a race in
> process-per-site rather than cause problems.  That's for a followup CL,
though,
> and we should think it through.)

Done.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-16 15:16:37 UTC) #19

commit-bot: I haz the power

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_chromeos_ozone_rel_ng/builds/385534) linux_chromium_tsan_rel_ng on ...

3 years, 7 months ago (2017-05-16 15:16:39 UTC) #20

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 15:00:36 UTC) #21

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/60001

3 years, 7 months ago (2017-05-17 15:01:03 UTC) #22

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-17 15:05:08 UTC) #23

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/270046) mac_chromium_compile_dbg_ng on ...

3 years, 7 months ago (2017-05-17 15:05:10 UTC) #24

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 15:11:01 UTC) #25

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/80001

3 years, 7 months ago (2017-05-17 15:11:38 UTC) #26

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-17 15:23:07 UTC) #27

commit-bot: I haz the power

Dry run: 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_ng/builds/456064)

3 years, 7 months ago (2017-05-17 15:23:10 UTC) #28

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 16:03:45 UTC) #29

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/100001

3 years, 7 months ago (2017-05-17 16:04:38 UTC) #30

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-17 16:53:49 UTC) #31

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/457496)

3 years, 7 months ago (2017-05-17 16:53:50 UTC) #32

Charlie Reis

Thanks! This is looking good. I want to do another deep pass to make sure ...

3 years, 7 months ago (2017-05-18 00:58:35 UTC) #33

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-22 13:44:48 UTC) #34

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/140001

3 years, 7 months ago (2017-05-22 13:45:06 UTC) #35

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-22 14:30:22 UTC) #36

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/160001

3 years, 7 months ago (2017-05-22 14:30:45 UTC) #37

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-22 14:41:39 UTC) #38

commit-bot: I haz the power

Dry run: 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_linux/builds/373048) mac_chromium_rel_ng on ...

3 years, 7 months ago (2017-05-22 14:41:40 UTC) #39

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-22 14:44:54 UTC) #40

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/180001

3 years, 7 months ago (2017-05-22 14:45:15 UTC) #41

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-22 16:45:57 UTC) #42

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-22 16:45:58 UTC) #43

clamy

Thanks! I have uploaded a new patch set that also fixes the test issues. https://codereview.chromium.org/2857213005/diff/100001/content/browser/frame_host/navigation_handle_impl.cc ...

3 years, 7 months ago (2017-05-22 16:59:52 UTC) #44

Charlie Reis

Great! Mostly looks good with nits, with a few additional questions below. Also, the case ...

3 years, 7 months ago (2017-05-23 07:29:12 UTC) #45

Great!  Mostly looks good with nits, with a few additional questions below.

Also, the case where a hosted app gets uninstalled after commit but before a
query still seems like something we should test to be sure we got it right, but
I don't know how feasible that is.  If there's no public APIs to
ProcessReusePolicy and hosted apps aren't inside content, maybe there's no easy
way to test it?  Ideally, I suppose we'd have a hosted app test with a
ServiceWorker, but I don't know how hard that is to set up.

https://codereview.chromium.org/2857213005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_unittest.cc (right):

https://codereview.chromium.org/2857213005/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_unittest.cc:312: }
On 2017/05/22 16:59:52, clamy (ooo until May 25) wrote:
> On 2017/05/18 00:58:34, Charlie Reis wrote:
> > These are great.  Should we also add a NTP test in chrome/ to make sure
we're
> > doing the right thing for ServiceWorkers on the NTP?
> 
> I was thinking of doing this in a follow up patch, as it may need additional
> modifications to the code.

What would that mean for the NTP in the meantime, for anyone using PlzNavigate? 
That the ServiceWorker would go into a different process than the NTP?  Is that
equivalent or worse than today's PlzNavigate NTP behavior?

I can see doing NTP support in a followup CL, but I wouldn't want to regress
things for long, especially near branch point.  Maybe we can set up the NTP CL
as a dependent CL to this one to get it ready to land as soon after this one as
we can?

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:704:
SetSpeculativeProcess(render_frame_host->GetProcess());
Naming nit: This RFH/process isn't speculative anymore, is it?  When we call
from ReadyToCommitNavigation, I think it's the one we definitely plan to use,
whether it matches the earlier speculative one or not.

What if we renamed the NavigationHandle concept from SetSpeculativeProcess to
SetExpectedProcess (which parallels Add/RemoveExpectedNavigationToSite and can
be called with either the speculative or final RFH)?  Same for
expected_render_process_host_id_.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:435: // Updates the SiteURL
this navigation is navigating to. This is called on
nit: Updates the destination site URL for this navigation.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:463: // The SiteURL of this
navigation, as obtained from SiteInstance::GetSiteURL.
nit: site URL

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1464:
SetLastCommittedSiteUrl(navigation_handle->GetURL());
We're using validated_params.url everywhere else here, so I'm inclined to stick
with that unless there's a reason to differ.  (Hopefully they'll be the same,
but it's hard to be sure given things like FilterURL above.)

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3943: if (!url.is_empty()
&& url.SchemeIsHTTPOrHTTPS()) {
This doesn't sound right to me.  What's the reason for limiting this to HTTP(S)?

There's a few reasons we've found this call to generally be the wrong option:
1) It fails for filesystem: and blob: URLs, which have inner URLs that can be
HTTP(S).
2) It fails for view-source: (similar).

I'm also unclear why we would need to omit data URLs (which can be for
legitimate navigations from web pages), file URLs, even WebUI or chrome-search
URLs.  If this is about predicting which URLs might have ServiceWorkers, then I
wonder if there's a more direct way to determine that, but also whether it's
needed.  It would certainly be easier to think about last_committed_site_url_ if
it was always set for successful commits, and not just for HTTP(s) cases.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:632: // The following
object is used to keep track of which sites
nit: s/object/class/
nit: used to track the sites each RenderProcessHost is hosting frames for and
expecting navigations to.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:634: // There are two
of them per BrowserContext, one for frames and one for
nit: Colon rather than comma (to make it seem more like a clarification and less
like a list of 4).

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:635: // navigations.
Can you elaborate a bit further here, explaining how each site is mapped to a
counts_per_process map, and why we chose this direction vs the other one?  I
think it's useful context, to help others decide whether it can be useful in
other ways (e.g., not for security checks asking what's live in a given
process).

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:640: class
SiteCountPerProcessTracker : public base::SupportsUserData::Data {
nit: Maybe SiteProcessCountTracker (now that we've changed the struct)?

Same in the key names above.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:645: void
AddCountForSiteForProcess(int render_process_host_id,
nit: IncrementSiteProcessCount
nit: Swap parameter order, since the map goes from site to (process -> count).

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:651: void
RemoveCountForSiteForProcess(int render_process_host_id,
nit: DecrementSiteProcessCount and swap parameter order

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:3436: site_url,
&eligible_foreground_hosts, &eligible_background_hosts);
Should there be any preference between processes that have a committed site URL
and those that are only expecting a site URL?  Seems like they'll go into the
same set and be picked randomly.  Maybe that's what ServiceWorker does today?

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-23 11:45:10 UTC) #46

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/220001

3 years, 7 months ago (2017-05-23 11:45:21 UTC) #47

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-23 13:39:44 UTC) #48

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-23 13:39:46 UTC) #49

clamy

Thanks! Yes I don't really know how to proceed with the hosted app case. I ...

3 years, 7 months ago (2017-05-23 13:41:15 UTC) #50

Thanks!

Yes I don't really know how to proceed with the hosted app case. I think we
should file a bug to add a test if possible.

https://codereview.chromium.org/2857213005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_unittest.cc (right):

https://codereview.chromium.org/2857213005/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_unittest.cc:312: }
On 2017/05/23 07:29:13, Charlie Reis wrote:
> On 2017/05/22 16:59:52, clamy (ooo until May 25) wrote:
> > On 2017/05/18 00:58:34, Charlie Reis wrote:
> > > These are great.  Should we also add a NTP test in chrome/ to make sure
> we're
> > > doing the right thing for ServiceWorkers on the NTP?
> > 
> > I was thinking of doing this in a follow up patch, as it may need additional
> > modifications to the code.
> 
> What would that mean for the NTP in the meantime, for anyone using
PlzNavigate? 
> That the ServiceWorker would go into a different process than the NTP?  Is
that
> equivalent or worse than today's PlzNavigate NTP behavior?
> 
> I can see doing NTP support in a followup CL, but I wouldn't want to regress
> things for long, especially near branch point.  Maybe we can set up the NTP CL
> as a dependent CL to this one to get it ready to land as soon after this one
as
> we can?

This patch does not regress the situation in PlzNavigate. In fact, it should
improve the situation on all navigations that are not the NTP and don't
redirect.

For the NTP, we need the search code to overwrite the site URL for the NTP
ServiceWorker so that it becomes chrome-search://remote-ntp. Then the NTP issue
should be fixed. I'd rather land this change in a follow-up CL, as it is not
directly related to this one.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:704:
SetSpeculativeProcess(render_frame_host->GetProcess());
On 2017/05/23 07:29:13, Charlie Reis wrote:
> Naming nit: This RFH/process isn't speculative anymore, is it?  When we call
> from ReadyToCommitNavigation, I think it's the one we definitely plan to use,
> whether it matches the earlier speculative one or not.
> 
> What if we renamed the NavigationHandle concept from SetSpeculativeProcess to
> SetExpectedProcess (which parallels Add/RemoveExpectedNavigationToSite and can
> be called with either the speculative or final RFH)?  Same for
> expected_render_process_host_id_.

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:435: // Updates the SiteURL
this navigation is navigating to. This is called on
On 2017/05/23 07:29:13, Charlie Reis wrote:
> nit: Updates the destination site URL for this navigation.

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:463: // The SiteURL of this
navigation, as obtained from SiteInstance::GetSiteURL.
On 2017/05/23 07:29:13, Charlie Reis wrote:
> nit: site URL

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1464:
SetLastCommittedSiteUrl(navigation_handle->GetURL());
On 2017/05/23 07:29:13, Charlie Reis wrote:
> We're using validated_params.url everywhere else here, so I'm inclined to
stick
> with that unless there's a reason to differ.  (Hopefully they'll be the same,
> but it's hard to be sure given things like FilterURL above.)

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3943: if (!url.is_empty()
&& url.SchemeIsHTTPOrHTTPS()) {
On 2017/05/23 07:29:13, Charlie Reis wrote:
> This doesn't sound right to me.  What's the reason for limiting this to
HTTP(S)?
> 
> There's a few reasons we've found this call to generally be the wrong option:
> 1) It fails for filesystem: and blob: URLs, which have inner URLs that can be
> HTTP(S).
> 2) It fails for view-source: (similar).
> 
> I'm also unclear why we would need to omit data URLs (which can be for
> legitimate navigations from web pages), file URLs, even WebUI or chrome-search
> URLs.  If this is about predicting which URLs might have ServiceWorkers, then
I
> wonder if there's a more direct way to determine that, but also whether it's
> needed.  It would certainly be easier to think about last_committed_site_url_
if
> it was always set for successful commits, and not just for HTTP(s) cases.

The issue was with interstitials. There's a bad sequence in the destructor of
intertitials that make the code below break when called from the RFH destructor
(the InterstitialPAge is the navigator, and it has already been destroyed when
we get there). Limiting the tracking to HTTP(S) was an easy way to get around
the problem. Since it is an issue, I'm now excluding interstitials from updating
the site url as we do for error pages.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:632: // The following
object is used to keep track of which sites
On 2017/05/23 07:29:13, Charlie Reis wrote:
> nit: s/object/class/
> nit: used to track the sites each RenderProcessHost is hosting frames for and
> expecting navigations to.

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:634: // There are two
of them per BrowserContext, one for frames and one for
On 2017/05/23 07:29:13, Charlie Reis wrote:
> nit: Colon rather than comma (to make it seem more like a clarification and
less
> like a list of 4).

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:635: // navigations.
On 2017/05/23 07:29:13, Charlie Reis wrote:
> Can you elaborate a bit further here, explaining how each site is mapped to a
> counts_per_process map, and why we chose this direction vs the other one?  I
> think it's useful context, to help others decide whether it can be useful in
> other ways (e.g., not for security checks asking what's live in a given
> process).

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:640: class
SiteCountPerProcessTracker : public base::SupportsUserData::Data {
On 2017/05/23 07:29:13, Charlie Reis wrote:
> nit: Maybe SiteProcessCountTracker (now that we've changed the struct)?
> 
> Same in the key names above.

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:645: void
AddCountForSiteForProcess(int render_process_host_id,
On 2017/05/23 07:29:13, Charlie Reis wrote:
> nit: IncrementSiteProcessCount
> nit: Swap parameter order, since the map goes from site to (process -> count).

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:651: void
RemoveCountForSiteForProcess(int render_process_host_id,
On 2017/05/23 07:29:13, Charlie Reis wrote:
> nit: DecrementSiteProcessCount and swap parameter order

Done.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:3436: site_url,
&eligible_foreground_hosts, &eligible_background_hosts);
On 2017/05/23 07:29:14, Charlie Reis wrote:
> Should there be any preference between processes that have a committed site
URL
> and those that are only expecting a site URL?  Seems like they'll go into the
> same set and be picked randomly.  Maybe that's what ServiceWorker does today?

I think it would prioritize the navigating ones today (they try to put it in the
process that is navigating). I have updated the code so that it does, as long as
it finds a foreground one.

Charlie Reis

Thanks for the fixes! I think one of my earlier questions might have fallen between ...

3 years, 7 months ago (2017-05-24 04:50:54 UTC) #51

Thanks for the fixes!

I think one of my earlier questions might have fallen between the cracks:
"I'd also suggest a sanity checker for balancing ref counts.  Can we have a
DCHECK somewhere saying that all the refcounts have gone to zero when a process
exits cleanly, for example?  Or something similar?"

(I'm worried about not cleaning up properly, and ending up with leaks /
unintentional process reuse bugs.)

On 2017/05/23 13:41:15, clamy (ooo until May 29) wrote:
> Thanks!
> 
> Yes I don't really know how to proceed with the hosted app case. I think we
> should file a bug to add a test if possible.

I think I'm ok landing this and filing bugs to test hosted apps and fix/test
NTP, but I would probably consider those launch blocking bugs.

Hosted apps in particular just caused a bad post-Stable regression in
https://crbug.com/715756.  They have poor test coverage for cases like this, but
they're installed by default for all users.

To test them, I think we probably will want to create a ServiceWorker in
hosted_app_browsertest.cc and make sure it ends up in the right process, even
when they get uninstalled at inconvenient times.

LGTM if you agree, and if we can add some kind of balanced ref count DCHECKs.

https://codereview.chromium.org/2857213005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_unittest.cc (right):

https://codereview.chromium.org/2857213005/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_unittest.cc:312: }
On 2017/05/23 13:41:15, clamy (ooo until May 29) wrote:
> On 2017/05/23 07:29:13, Charlie Reis wrote:
> > On 2017/05/22 16:59:52, clamy (ooo until May 25) wrote:
> > > On 2017/05/18 00:58:34, Charlie Reis wrote:
> > > > These are great.  Should we also add a NTP test in chrome/ to make sure
> > we're
> > > > doing the right thing for ServiceWorkers on the NTP?
> > > 
> > > I was thinking of doing this in a follow up patch, as it may need
additional
> > > modifications to the code.
> > 
> > What would that mean for the NTP in the meantime, for anyone using
> PlzNavigate? 
> > That the ServiceWorker would go into a different process than the NTP?  Is
> that
> > equivalent or worse than today's PlzNavigate NTP behavior?
> > 
> > I can see doing NTP support in a followup CL, but I wouldn't want to regress
> > things for long, especially near branch point.  Maybe we can set up the NTP
CL
> > as a dependent CL to this one to get it ready to land as soon after this one
> as
> > we can?
> 
> This patch does not regress the situation in PlzNavigate. In fact, it should
> improve the situation on all navigations that are not the NTP and don't
> redirect.
> 
> For the NTP, we need the search code to overwrite the site URL for the NTP
> ServiceWorker so that it becomes chrome-search://remote-ntp. Then the NTP
issue
> should be fixed. I'd rather land this change in a follow-up CL, as it is not
> directly related to this one.

Acknowledged.

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2857213005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3943: if (!url.is_empty()
&& url.SchemeIsHTTPOrHTTPS()) {
On 2017/05/23 13:41:15, clamy (ooo until May 29) wrote:
> On 2017/05/23 07:29:13, Charlie Reis wrote:
> > This doesn't sound right to me.  What's the reason for limiting this to
> HTTP(S)?
> > 
> > There's a few reasons we've found this call to generally be the wrong
option:
> > 1) It fails for filesystem: and blob: URLs, which have inner URLs that can
be
> > HTTP(S).
> > 2) It fails for view-source: (similar).
> > 
> > I'm also unclear why we would need to omit data URLs (which can be for
> > legitimate navigations from web pages), file URLs, even WebUI or
chrome-search
> > URLs.  If this is about predicting which URLs might have ServiceWorkers,
then
> I
> > wonder if there's a more direct way to determine that, but also whether it's
> > needed.  It would certainly be easier to think about
last_committed_site_url_
> if
> > it was always set for successful commits, and not just for HTTP(s) cases.
> 
> The issue was with interstitials. There's a bad sequence in the destructor of
> intertitials that make the code below break when called from the RFH
destructor
> (the InterstitialPAge is the navigator, and it has already been destroyed when
> we get there). Limiting the tracking to HTTP(S) was an easy way to get around
> the problem. Since it is an issue, I'm now excluding interstitials from
updating
> the site url as we do for error pages.

Ah, thanks.  That makes more sense.

https://codereview.chromium.org/2857213005/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/2857213005/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_process_manager.cc:205: if
(can_use_existing_process) {
Sanity check: I'm assuming we'll come back and remove this old
ServiceWorker-specific process reuse logic in a followup CL?

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-24 13:14:48 UTC) #52

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/240001

3 years, 7 months ago (2017-05-24 13:15:03 UTC) #53

clamy

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-24 13:47:19 UTC) #54

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/260001

3 years, 7 months ago (2017-05-24 13:47:37 UTC) #55

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-24 14:14:57 UTC) #56

commit-bot: I haz the power

Dry run: 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_linux/builds/374996)

3 years, 7 months ago (2017-05-24 14:14:58 UTC) #57

clamy

Thanks! I have added a check mechanism to ensure that RPH ids are not left ...

3 years, 7 months ago (2017-05-24 14:44:28 UTC) #58

clamy

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the ...

3 years, 7 months ago (2017-05-24 14:44:43 UTC) #60

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/280001

3 years, 7 months ago (2017-05-24 14:45:07 UTC) #61

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-24 15:37:09 UTC) #62

commit-bot: I haz the power

Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/301905)

3 years, 7 months ago (2017-05-24 15:37:10 UTC) #63

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/280001

3 years, 7 months ago (2017-05-24 15:40:08 UTC) #65

Charlie Reis

Great! I'm glad we caught that bug, and I feel much better with the hosted ...

3 years, 7 months ago (2017-05-24 17:18:37 UTC) #66

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-24 17:49:04 UTC) #67

commit-bot: I haz the power

Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isolation/builds/8135)

3 years, 7 months ago (2017-05-24 17:49:06 UTC) #68

clamy

Thanks! https://codereview.chromium.org/2857213005/diff/280001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2857213005/diff/280001/content/browser/frame_host/render_frame_host_manager.cc#newcode486 content/browser/frame_host/render_frame_host_manager.cc:486: if (IsBrowserSideNavigationEnabled()) { On 2017/05/24 17:18:37, Charlie Reis ...

3 years, 7 months ago (2017-05-24 18:03:04 UTC) #69

clamy

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the ...

3 years, 7 months ago (2017-05-24 18:03:15 UTC) #71

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857213005/300001

3 years, 7 months ago (2017-05-24 18:04:17 UTC) #72

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495648995352110, "parent_rev": "11f044437561f28e16d0a2549714d860310fb3fd", "commit_rev": "5d947f5a0c60d33fa16b6609978bd712a497cb00"}

3 years, 7 months ago (2017-05-24 19:51:23 UTC) #73

commit-bot: I haz the power

Description was changed from ========== PlzNavigate: implement process reuse for ServiceWorkers This Cl implements a ...

3 years, 7 months ago (2017-05-24 19:51:34 UTC) #74

commit-bot: I haz the power

3 years, 7 months ago (2017-05-24 19:51:37 UTC) #75

Message was sent while issue was closed.

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/5d947f5a0c60d33fa16b6609978b...

Issue 2857213005: PlzNavigate: implement process reuse for ServiceWorkers (Closed)

Description

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Addressed comments #

Patch Set 4 : Added unit tests #

Patch Set 5 : Rebase #

Patch Set 6 : Fixed compilation error #

Patch Set 7 : Fixed tests #

Patch Set 8 : Rebase #

Patch Set 9 : Addressed comments #

Patch Set 10 : Fix compilation error #

Patch Set 11 : Rebase #

Patch Set 12 : Addressed comments #

Patch Set 13 : Rebase + addressed comments #

Patch Set 14 : Added unit tests for site url changes #

Patch Set 15 : Removed wrong CleanupNavigation call #

Patch Set 16 : Addressed comments + fixed tests #

Messages