PlzNavigate: Make ServiceWorker work with PlzNavigate.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index f5d90c62c10cfa3267f72f938a5122b6eb5c275a..6593e3645527cef4e6698269628f14022f20eb91 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -449,6 +449,8 @@ WebURLRequest CreateURLRequestForNavigation(
 
   RequestExtraData* extra_data = new RequestExtraData();
   extra_data->set_stream_override(stream_override.Pass());
+  extra_data->set_service_worker_provider_id(
+      common_params.service_worker_provider_id);
   request.setExtraData(extra_data);
 
   // Set the ui timestamp for this navigation. Currently the timestamp here is
@@ -515,7 +517,8 @@ CommonNavigationParams MakeCommonNavigationParams(
   return CommonNavigationParams(
       request->url(), referrer, extra_data->transition_type(),
       FrameMsg_Navigate_Type::NORMAL, true, should_replace_current_entry,
-      ui_timestamp, report_type, GURL(), GURL());
+      ui_timestamp, report_type, GURL(), GURL(),
+      extra_data->service_worker_provider_id());

[michaeln, 2015/09/16 00:56:42]

If i understand correctly, i think this is called in preperation of asking the
browser to do the navigation. Is that right?

* How does the value provided here get used downstream in the browser process?

* Can the browser decide to offload the nav to a different process?

[michaeln, 2015/09/18 23:03:56]

Ok, this id is definitely not useful.

The object it identifies will be deleted prior to startLoad() returning and any
other msg being processed by the loop. According to nate this is by desigin. The
original policy/provisional loader+swprovider is deleted (per the snippet below)
and a new loader+swprovider gets created if/when the browser process decides to
delegate the navigation back to the this child process.

snipped from FrameLoader::startLoad(...)

    // stopAllLoaders can detach the LocalFrame, so protect it.
    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
    if ((!m_policyDocumentLoader->shouldContinueForNavigationPolicy(...) {
        detachDocumentLoader(m_policyDocumentLoader);
        return;
    }

The MakeCommonNavigationParams() function is called inside of
m_policyDocumentLoader->shouldContinueForNavigationPolicy. The documentLoader
and its associated ServiceWorkerNetworkProvider are deleted inside of
detachDocumentLoader().

Eventually we get an ipc to RenderFrameImpl::OnCommitNavigation(). As currently
coded in the cl, the value of CommonNavigationParams.service_worker_provider_id
in that message is kInvalidServiceWorkerProviderId. Yet the stream override url
has an actual blob:url in it.

I'll look more closely at the corresponding control flow in the browser process?

[Fabrice (no longer in Chrome), 2015/09/30 17:32:08]

This has been removed.

 }
 
 #if !defined(OS_ANDROID) || defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
@@ -2574,8 +2577,19 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
         blink::WebSandboxFlags::Origin) {
       provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME;
     }
+    const WebURLRequest& original_request = datasource->originalRequest();
+
+    int service_worker_provider_id = kInvalidServiceWorkerProviderId;
+    if (original_request.extraData()) {

[michaeln, 2015/09/16 00:56:42]

I think this new bit of code is specific to PlzNavigate, if thats right please
annotate it with the // PlzNavigate comment too. I think the extraData() only
applies to PlzNavigate initiated navigations, is that right? 

I really appreciate the use of the PlzNavigate comment to annotate where the two
different modes of doing navigations differ. Otherwise we have mysteries like
"why is extra_data sometimes null and other times not and does it matter" that
nearly nobody knows the answer to.

[Fabrice (no longer in Chrome), 2015/09/30 17:32:07]

Done.
There's a bit of duplication here now but I find it easier to understand this
way since the tests are different for PlzNavigate anyway.

+      RequestExtraData* extra_data =
+          static_cast<RequestExtraData*>(original_request.extraData());
+      service_worker_provider_id = extra_data->service_worker_provider_id();
+    }
+
     scoped_ptr<ServiceWorkerNetworkProvider> network_provider(
-        new ServiceWorkerNetworkProvider(routing_id_, provider_type));
+        new ServiceWorkerNetworkProvider(routing_id_, provider_type,
+                                         service_worker_provider_id));
+
     ServiceWorkerNetworkProvider::AttachToDocumentState(
         DocumentState::FromDataSource(datasource),
         network_provider.Pass());
@@ -4837,12 +4851,30 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request) {
   if (data_source && render_view_->history_list_length_ > 0) {
     should_replace_current_entry = data_source->replacesCurrentHistoryItem();
   }
+
+  DCHECK_EQ(FETCH_REQUEST_MODE_NO_CORS,
+            GetFetchRequestModeForWebURLRequest(*request));
+  DCHECK_EQ(FETCH_CREDENTIALS_MODE_SAME_ORIGIN,
+            GetFetchCredentialsModeForWebURLRequest(*request));
+  DCHECK(GetFetchRedirectModeForWebURLRequest(*request) ==
+         FetchRedirectMode::FOLLOW_MODE);

[michaeln, 2015/09/18 23:03:56]

I've patched it in and am seeing these DCHECKs get hit on every navigation?

Is there something more I need to do in addition to running with
--enable-browser-side-navigation to test things?

[Fabrice (no longer in Chrome), 2015/09/30 17:32:08]

That's because the values for navigation requests have changed in the meantime.
I fixed them.

+  DCHECK_IMPLIES(!frame_->parent(),
+                 GetRequestContextFrameTypeForWebURLRequest(*request) ==
+                 REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL);
+  DCHECK_IMPLIES(frame_->parent(),
+                 GetRequestContextFrameTypeForWebURLRequest(*request) ==
+                 REQUEST_CONTEXT_FRAME_TYPE_NESTED);
+
   Send(new FrameHostMsg_BeginNavigation(
       routing_id_,
       MakeCommonNavigationParams(request, should_replace_current_entry),
       BeginNavigationParams(
-          request->httpMethod().latin1(), GetWebURLRequestHeaders(*request),
-          GetLoadFlagsForWebURLRequest(*request), request->hasUserGesture()),
+          request->httpMethod().latin1(),
+          GetWebURLRequestHeaders(*request),
+          GetLoadFlagsForWebURLRequest(*request),
+          request->hasUserGesture(),
+          request->skipServiceWorker(),
+          GetRequestContextTypeForWebURLRequest(*request)),
       GetRequestBodyForWebURLRequest(*request)));
 }