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 466 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();
 
+  SetLastCommittedSiteUrl(GURL());
+
   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 951 matching lines @@
       frame_tree_node()->DidStartLoading(true, was_loading);
     }
     pending_commit_ = false;
   }
 
   // Find the appropriate NavigationHandle for this navigation.
   std::unique_ptr<NavigationHandleImpl> navigation_handle =
       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.
   if (!IsBrowserSideNavigationEnabled() &&
       !validated_params.searchable_form_url.is_empty()) {
     navigation_handle->set_searchable_form_url(
         validated_params.searchable_form_url);
     navigation_handle->set_searchable_form_encoding(
         validated_params.searchable_form_encoding);
 
@@ skipping 2451 matching lines @@
       false);                 // is_form_submission
 }
 
 void RenderFrameHostImpl::BeforeUnloadTimeout() {
   if (render_view_host_->GetDelegate()->ShouldIgnoreUnresponsiveRenderer())
     return;
 
   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
     : public service_manager::mojom::InterfaceProvider {
  public:
   using BindCallback =
       base::Callback<void(const std::string&, mojo::ScopedMessagePipeHandle)>;
 
   JavaInterfaceProvider(
       const BindCallback& bind_callback,
@@ skipping 44 matching lines @@
 }
 
 void RenderFrameHostImpl::ForwardGetInterfaceToRenderFrame(
     const std::string& interface_name,
     mojo::ScopedMessagePipeHandle pipe) {
   GetRemoteInterfaces()->GetInterface(interface_name, std::move(pipe));
 }
 #endif
 
 }  // namespace content