PlzNavigate: implement process reuse for ServiceWorkers

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 4d7f92c09c1d0f73fc9e6d64ca1e6d46ae621ee2..32da13d938872c1b7a392750a9ed7e0dab748c77 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -476,6 +476,12 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   // RenderFrameHost during cleanup.
   ClearAllWebUI();
 
+  if (!last_committed_url_.is_empty()) {

[Charlie Reis, 2017/05/15 03:41:52]

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.

[clamy, 2017/05/16 14:50:45]

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.

+    RenderProcessHostImpl::RemoveFrameWithSite(
+        frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+        GetProcess(), last_committed_url_);
+  }
+
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
@@ -1160,6 +1166,21 @@ void RenderFrameHostImpl::SetLastCommittedOrigin(const url::Origin& origin) {
   CSPContext::SetSelf(origin);
 }
 
+void RenderFrameHostImpl::SetLastCommittedUrl(const GURL& url) {
+  BrowserContext* browser_context =
+      frame_tree_node_->navigator()->GetController()->GetBrowserContext();
+
+  if (!last_committed_url_.is_empty()) {

[nasko, 2017/05/05 05:22:00]

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 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.

[Charlie Reis, 2017/05/15 03:41:52]

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).

[clamy, 2017/05/16 14:50:45]

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.

+    RenderProcessHostImpl::RemoveFrameWithSite(browser_context, GetProcess(),
+                                               last_committed_url_);
+  }
+
+  last_committed_url_ = url;
+
+  RenderProcessHostImpl::AddFrameWithSite(browser_context, GetProcess(),
+                                          last_committed_url_);
+}
+
 void RenderFrameHostImpl::OnDetach() {
   frame_tree_->RemoveFrame(frame_tree_node_);
 }