PlzNavigate: implement process reuse for ServiceWorkers

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_frame_host_impl.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 458 matching lines @@
 
 RenderFrameHostImpl::~RenderFrameHostImpl() {
   // Destroying navigation handle may call into delegates/observers,
   // so we do it early while |this| object is still in a sane state.
   navigation_handle_.reset();
 
   // Release the WebUI instances before all else as the WebUI may accesses the
   // 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_));
 
   if (overlay_routing_token_)
     g_token_frame_map.Get().erase(*overlay_routing_token_);
 
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(&NotifyRenderFrameDetachedOnIO,
                                      GetProcess()->GetID(), routing_id_));
@@ skipping 664 matching lines @@
   frame_tree_->AddFrame(frame_tree_node_, GetProcess()->GetID(), new_routing_id,
                         scope, frame_name, frame_unique_name, sandbox_flags,
                         container_policy, frame_owner_properties);
 }
 
 void RenderFrameHostImpl::SetLastCommittedOrigin(const url::Origin& origin) {
   last_committed_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_);
 }
 
 void RenderFrameHostImpl::OnFrameFocused() {
   delegate_->SetFocusedFrame(frame_tree_node_, GetSiteInstance());
 }
 
 void RenderFrameHostImpl::OnOpenURL(const FrameHostMsg_OpenURL_Params& params) {
   GURL validated_url(params.url);
@@ skipping 2729 matching lines @@
 }
 
 void RenderFrameHostImpl::ForwardGetInterfaceToRenderFrame(
     const std::string& interface_name,
     mojo::ScopedMessagePipeHandle pipe) {
   GetRemoteInterfaces()->GetInterface(interface_name, std::move(pipe));
 }
 #endif
 
 }  // namespace content