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

content/browser/loader/resource_dispatcher_host_impl.cc

 // Copyright (c) 2012 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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 24 matching lines @@
 #include "base/time/time.h"
 #include "content/browser/appcache/appcache_interceptor.h"
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/bad_message.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/cert_store_impl.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #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"
 #include "content/browser/loader/async_revalidation_manager.h"
 #include "content/browser/loader/cross_site_resource_handler.h"
 #include "content/browser/loader/detachable_resource_handler.h"
 #include "content/browser/loader/loader_delegate.h"
 #include "content/browser/loader/mime_type_resource_handler.h"
 #include "content/browser/loader/mojo_async_resource_handler.h"
 #include "content/browser/loader/navigation_resource_handler.h"
@@ skipping 359 matching lines @@
     if (frame_host)
       routing_ids->insert(frame_host->GetGlobalFrameRoutingId());
     if (pending_frame_host)
       routing_ids->insert(pending_frame_host->GetGlobalFrameRoutingId());
   }
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(&NotifyForRouteSetOnIO, frame_callback,
                                      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() {}
 
 ResourceDispatcherHostImpl::HeaderInterceptorInfo::~HeaderInterceptorInfo() {}
 
 ResourceDispatcherHostImpl::HeaderInterceptorInfo::HeaderInterceptorInfo(
     const HeaderInterceptorInfo& other) {}
 
 // static
@@ skipping 765 matching lines @@
   // ResourceRequestInfo rather than caching it locally.  This lets us update
   // the info object when a transfer occurs.
   info->UpdateForTransfer(child_id, route_id, request_data.render_frame_id,
                           request_data.origin_pid, request_id,
                           filter_->GetWeakPtr());
 
   // If a certificate is stored with the ResourceResponse, it has to be
   // 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
   // do not actually use a different ID, so not all maps need to be updated.
   pending_loaders_[new_request_id] = std::move(loader);
   IncrementOutstandingRequestsMemory(1, *info);
   if (should_update_count)
     IncrementOutstandingRequestsCount(1, info);
   if (old_routing_id != new_routing_id) {
     if (blocked_loaders_map_.find(old_routing_id) !=
@@ skipping 480 matching lines @@
   handler.reset(new MimeTypeResourceHandler(std::move(handler), this,
                                             plugin_service, request));
 
   ScopedVector<ResourceThrottle> throttles;
 
   // Add a NavigationResourceThrottle for navigations.
   // PlzNavigate: the throttle is unnecessary as communication with the UI
   // 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_) {
     delegate_->RequestBeginning(request,
                                 resource_context,
                                 appcache_service,
                                 resource_type,
                                 &throttles);
   }
 
@@ skipping 568 matching lines @@
     // Hang on to a reference to ensure the blob is not released prior
     // to the job being started.
     storage::BlobProtocolHandler::SetRequestedBlobDataHandle(
         new_request.get(),
         blob_context->GetBlobDataFromPublicURL(new_request->url()));
   }
 
   // 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;
   // currently it's a no-op.
   handler =
       AddStandardHandlers(new_request.get(), resource_type, resource_context,
                           info.begin_params.request_context_type,
                           nullptr,  // appcache_service
                           -1,       // child_id
                           -1,       // route_id
@@ skipping 327 matching lines @@
   }
 
   if (is_sync_load)
     load_flags |= net::LOAD_IGNORE_LIMITS;
 
   return load_flags;
 }
 
 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;
   // DeserializeSecurityInfo() often takes security info sent by a
   // renderer as input, in which case it's important to check that the
   // security info deserializes properly and kill the renderer if
   // not. In this case, however, the security info has been provided by
   // the ResourceLoader, so it does not need to be treated as untrusted
   // data.
   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() {
   return cert_store_for_testing_ ? cert_store_for_testing_
                                  : CertStore::GetInstance();
 }
 
 bool ResourceDispatcherHostImpl::ShouldServiceRequest(
     int process_type,
     int child_id,
@@ skipping 45 matching lines @@
                        << iter->filesystem_url().spec();
           return false;
         }
       }
     }
   }
   return true;
 }
 
 }  // namespace content