Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache it on the browser …

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index 847f0fd5f9d82e6dd5d54a4e4db3d75360c35fde..705ff922aa4d85b6b0dd797e0beb1d82f9551f35 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -42,6 +42,7 @@
 #include "content/browser/download/download_resource_handler.h"
 #include "content/browser/download/save_file_resource_handler.h"
 #include "content/browser/frame_host/frame_tree.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/browser/frame_host/navigation_request_info.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/loader/async_resource_handler.h"
@@ -421,6 +422,23 @@ void NotifyForEachFrameFromUI(
                                      base::Passed(std::move(routing_ids))));
 }
 
+void UpdateSSLStatus(int render_process_id,
+                     int render_frame_host_id,
+                     int new_cert_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RenderFrameHostImpl* render_frame_host =
+      RenderFrameHostImpl::FromID(render_process_id, render_frame_host_id);
+  if (!render_frame_host)
+    return;
+
+  NavigationHandleImpl* navigation_handle =
+      render_frame_host->navigation_handle();
+  if (!navigation_handle)
+    return;
+
+  navigation_handle->UpdateSSLCertId(new_cert_id);

[nasko, 2016/08/25 18:09:48]

I think it is still possible to have race conditions between creation of
NavigationHandle and activity on the IO thread with non-PlzNavigate. So I think
your old approach of passing along the URL as well I think is still a good idea.

If we've made a network request for foo.com and in the meantime the user clicks
a link to bar.com, we will emit DidStartProvisionalLoad, which will delete the
old NavigationHandle and create a new one. The IO thread might not have
cancelled the request by that time and the task makes it to the UI thread and we
will put the foo.com SSLStatus on bar.com NavigationHandle.

[jam, 2016/08/25 21:56:46]

this method is just for one case (cross-site transfer). I added a URL check.

However for the other non-plz navigate case in NavigationResourceThrottle, does
anything need to be done there? It looks like it already calls
NavigationHandleImpl::WillProcessResponse without any URL checks, so I think the
race condition you mention doesn't apply there?

[clamy, 2016/08/25 23:25:14]

Regarding the possible race condition, the issue came up when I was writing the
code. Normally there shouldn't be a race condition. When we click on a link to
bar.com, we will delete the the provisional DocumentLoader for foo.com before we
send the DidStartProvisionalLoad IPC. Deleting the provisional DocumentLoader
will also cancel any request it had made, issuing an IPC to cancel the request
on the IO thread.

Since all browser IPCs are synchronized on the IO thread, this means that the
IPC to abort the current request will be received before the
DidStartProvisionalLoad IPC on the Io thread, and in particular before the
DidStartProvisionalLoad IPC is passed to the UI thread where it will create the
new NavigationHandle. Similarly, the IPC to request the main resource for the
new navigation is sent after DidStartProvisionalLoad, so by the time the new
request wants to reach the NavigationHandle on the UI thread the
NavigationHandle has already been created.

[clamy, 2016/08/26 00:06:57]

Looking at the code some more, I'm wondering if it would be possible to update
the certificate  on the UI thread first before doing it on the IO thread. We
have access to the NavigationHandle at the moment we transfer the navigation, so
we can update the certificate in the cert store at that moment (the CertStore
can be accessed from the UI thread as well). Then when we inform the IO thread
that the navigation is transferring, passing along the new cert id. This would
avoid some potential trickiness of the NavigationHandle having been transferred
to a new RFH but the current request still having ids for the old RFH.

[jam, 2016/08/26 03:36:43]

The first paragraph makes it sound like there's a race condition but the second
paragraph makes it sound like there isn't, am I reading this correctly?

[jam, 2016/08/26 03:36:43]

which method are you referring to?
by "ids" do you mean the cert id? I'm not sure what the issue is, since they
won't be used until there's another IO to UI thread hop when a commit IPC is
received from the new renderer

[clamy, 2016/08/26 18:14:30]

Hmm my first paragraph is not very clear. What I meant is that there is no race
condition due to to the ordering of IPCs on the IO thread that I explain in the
second paragraph. As long as you still have a URLRequest on the IO thread (and
we do), then the NavigationHandle on the UI thread matches the request on the IO
thread.

[clamy, 2016/08/26 18:14:30]

It was just an optimization to avoid thread hops,. Since the issue with process
id that requires updating the cert should go away, I don't think it's worth it.

+}
+
 }  // namespace
 
 ResourceDispatcherHostImpl::HeaderInterceptorInfo::HeaderInterceptorInfo() {}
@@ -1206,8 +1224,8 @@ void ResourceDispatcherHostImpl::UpdateRequestForTransfer(
   // updated to be associated with the new process.
   if (loader->transferring_response()) {
     UpdateResponseCertificateForTransfer(loader->transferring_response(),
-                                         loader->request()->ssl_info(),
-                                         child_id);
+                                         loader->request(),
+                                         info);
   }
 
   // Update maps that used the old IDs, if necessary.  Some transfers in tests
@@ -1708,7 +1726,7 @@ ResourceDispatcherHostImpl::AddStandardHandlers(
   // thread is handled by the NavigationURLloader.
   if (!IsBrowserSideNavigationEnabled() && IsResourceTypeFrame(resource_type)) {
     throttles.push_back(new NavigationResourceThrottle(
-        request, delegate(), fetch_request_context_type));
+        request, delegate_, GetCertStore(), fetch_request_context_type));
   }
 
   if (delegate_) {
@@ -2297,7 +2315,8 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
   // TODO(davidben): Attach AppCacheInterceptor.
 
   std::unique_ptr<ResourceHandler> handler(
-      new NavigationResourceHandler(new_request.get(), loader, delegate()));
+      new NavigationResourceHandler(new_request.get(), loader, delegate(),
+                                    GetCertStore()));
 
   // TODO(davidben): Pass in the appropriate appcache_service. Also fix the
   // dependency on child_id/route_id. Those are used by the ResourceScheduler;
@@ -2645,8 +2664,9 @@ int ResourceDispatcherHostImpl::BuildLoadFlagsForRequest(
 
 void ResourceDispatcherHostImpl::UpdateResponseCertificateForTransfer(
     ResourceResponse* response,
-    const net::SSLInfo& ssl_info,
-    int child_id) {
+    net::URLRequest* request,
+    ResourceRequestInfoImpl* info) {
+  const net::SSLInfo& ssl_info = request->ssl_info();
   if (!ssl_info.cert)
     return;
   SSLStatus ssl;
@@ -2659,8 +2679,21 @@ void ResourceDispatcherHostImpl::UpdateResponseCertificateForTransfer(
   bool deserialized =
       DeserializeSecurityInfo(response->head.security_info, &ssl);
   DCHECK(deserialized);
-  ssl.cert_id = GetCertStore()->StoreCert(ssl_info.cert.get(), child_id);
+  ssl.cert_id = GetCertStore()->StoreCert(ssl_info.cert.get(),
+                                          info->GetChildID());
   response->head.security_info = SerializeSecurityInfo(ssl);
+
+  if (info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME) {
+    int render_process_id, render_frame_id;
+    if (info->GetAssociatedRenderFrame(&render_process_id, &render_frame_id)) {
+      BrowserThread::PostTask(BrowserThread::UI,
+                              FROM_HERE,
+                              base::Bind(UpdateSSLStatus,
+                                         render_process_id,
+                                         render_frame_id,
+                                         ssl.cert_id));
+    }
+  }
 }
 
 CertStore* ResourceDispatcherHostImpl::GetCertStore() {