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 cd3a864d50d0e1a705539d3bfec517dfd39ce22b..8c9e1fdebec708e84adb729a2eb640d7848aea60 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -484,6 +484,8 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   // RenderFrameHost during cleanup.
   ClearAllWebUI();
 
+  SetLastCommittedSiteUrl(GURL());
+
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
@@ -1455,6 +1457,13 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
       TakeNavigationHandleForCommit(validated_params);
   DCHECK(navigation_handle);
 
+  // Update the site url if the navigation was successful.
+  if (validated_params.url_is_unreachable) {
+    SetLastCommittedSiteUrl(GURL());
+  } else {
+    SetLastCommittedSiteUrl(navigation_handle->GetURL());

[Charlie Reis, 2017/05/23 07:29:13]

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

[clamy, 2017/05/23 13:41:15]

Done.

+  }
+
   // PlzNavigate sends searchable form data in the BeginNavigation message
   // while non-PlzNavigate sends it in the DidCommitProvisionalLoad message.
   // Update |navigation_handle| if necessary.
@@ -3926,6 +3935,35 @@ void RenderFrameHostImpl::BeforeUnloadTimeout() {
   SimulateBeforeUnloadAck();
 }
 
+void RenderFrameHostImpl::SetLastCommittedSiteUrl(const GURL& url) {
+  GURL site_url = GURL();
+
+  // Only record the site url for http(s) schemes. In particular, do not record
+  // site urls for data urls (eg. interstitials and error pages) and WebUIs.
+  if (!url.is_empty() && url.SchemeIsHTTPOrHTTPS()) {

[Charlie Reis, 2017/05/23 07:29:13]

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.

[clamy, 2017/05/23 13:41:15]

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.

[Charlie Reis, 2017/05/24 04:50:54]

Ah, thanks.  That makes more sense.

+    site_url = SiteInstance::GetSiteForURL(
+        frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+        url);
+  }
+
+  if (last_committed_site_url_ == site_url)
+    return;
+
+  if (!last_committed_site_url_.is_empty()) {
+    RenderProcessHostImpl::RemoveFrameWithSite(
+        frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+        GetProcess(), last_committed_site_url_);
+  }
+
+  last_committed_site_url_ = site_url;
+
+  if (!last_committed_site_url_.is_empty()) {
+    RenderProcessHostImpl::AddFrameWithSite(
+        frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+        GetProcess(), last_committed_site_url_);
+  }
+}
+
 #if defined(OS_ANDROID)
 
 class RenderFrameHostImpl::JavaInterfaceProvider