Prerender: Confirm ServiceWorkers are invoked for NoState Prefetch

chrome/browser/prerender/prerender_contents.cc

Index: chrome/browser/prerender/prerender_contents.cc
diff --git a/chrome/browser/prerender/prerender_contents.cc b/chrome/browser/prerender/prerender_contents.cc
index f7c11cf153070824219bb25c50235ddffe4f808d..91a1e5f4c47b64852298ae2155a911bb1585bea0 100644
--- a/chrome/browser/prerender/prerender_contents.cc
+++ b/chrome/browser/prerender/prerender_contents.cc
@@ -38,7 +38,9 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/resource_request_details.h"
+#include "content/public/browser/service_worker_context.h"
 #include "content/public/browser/session_storage_namespace.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/frame_navigate_params.h"
@@ -208,7 +210,8 @@ PrerenderContents::PrerenderContents(
       child_id_(-1),
       route_id_(-1),
       origin_(origin),
-      network_bytes_(0) {
+      network_bytes_(0),
+      weak_factory_(this) {
   DCHECK(prerender_manager);
 }
 
@@ -316,6 +319,34 @@ void PrerenderContents::StartPrerendering(
   prerender_contents_.get()->SetUserAgentOverride(
       prerender_manager_->config().user_agent_override);
 
+  // Start URL load only after checking for a ServiceWorker if we're doing
+  // NoStatePrefetch.
+  if (prerender_manager_->IsNoStatePrefetch()) {
+    content::BrowserContext* browser_context =
+        prerender_contents_->GetBrowserContext();
+    content::StoragePartition* storage_partition =
+        content::BrowserContext::GetStoragePartition(
+            Profile::FromBrowserContext(browser_context),
+            prerender_contents_->GetSiteInstance());
+
+    storage_partition->GetServiceWorkerContext()->CheckHasServiceWorker(
+        prerender_url_, prerender_url_,
+        base::Bind(&PrerenderContents::ContinuePrerenderIfNoServiceWorker,
+                   weak_factory_.GetWeakPtr()));
+  } else {
+    // Full prerender doesn't care if we have a service worker, so we continue
+    // the prerender unconditionally.
+    ContinuePrerenderIfNoServiceWorker(false /* has_service_worker, ignored */);
+  }
+}
+
+void PrerenderContents::ContinuePrerenderIfNoServiceWorker(

[droger, 2016/12/13 12:43:01]

Naming nit (optional): there are cases where the prerender continues even if
there is a service worker. I would prefer finding a better name. I don't really
have good suggestions, but I think MaybeContinuePrerender() or even
ContinuePrerender() would be ok.

[mattcary, 2016/12/13 12:58:37]

That's a good point. The wrinkle is that this is a service worker-specific
method so it can fit with the CheckHasServiceWorker callback, I wasn't sure what
the convention was for a case like this (ie, making the context a function
should be called in very clear in the name or not).

+    bool has_service_worker) {
+  if (prerender_manager_->IsNoStatePrefetch() && has_service_worker) {
+    this->Destroy(FINAL_STATUS_SERVICE_WORKER);
+    return;

[droger, 2016/12/13 12:43:01]

Early return here means that the page will be considered as prefetched, even
though it was not. This will impact our metrics for example.

Could this code be moved in PrerenderManager::AddPrerender, specifically before
the line:

prefetches_.emplace_back(url, GetCurrentTimeTicks(), origin);

[mattcary, 2016/12/13 12:58:36]

Mmmm, good point.

As far as I can tell, we can't get at the service worker until we have a web
contents, which is created when PrerenderContents::StartPrerendering is invoked.
Currently the PrerenderContents is created in the PrerenderManager after
prefetches_ is updated. It seems cleanest to create-then-cancel, given
everything that happens when the PrerenderContents is created.

We could remove the URL from the prefetches_ list after we detect the service
worker. Would that be enough? There is a race if another navigation happens in
between.

[droger, 2016/12/13 13:25:48]

I don't see any use of WebContents in the code you are adding, are you sure the
WebContents is really needed?

Otherwise I guess we can probably keep the code as is.

[mattcary, 2016/12/13 13:30:50]

prerender_contents_ is actually a WebContents (yay for naming! not my fault...)

Anyway I just use it to get the StoragePartition. If there's a different way of
getting that without the WebContents that would be better, it would allow us to
possibly make this check before creating the PrerenderContents.

[droger, 2016/12/13 13:36:36]

prerender_contents_->GetBrowserContext() is just the profile, which we already
have in PrerenderManager.

And it seems the SiteInstance stuff can by bypassed using
GetStoragePartitionForSite, which just takes a URL. I'm not familiar with this,
but GetStoragePartition() calls GetStoragePartitionForSite() internally so that
seems sensible.

[mattcary, 2016/12/13 13:53:46]

ah, good point, will look.

+  }
+
   content::NavigationController::LoadURLParams load_url_params(
       prerender_url_);
   load_url_params.referrer = referrer_;