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

content/browser/loader/resource_loader.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.
 
 #include "content/browser/loader/resource_loader.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/location.h"
@@ skipping 28 matching lines @@
 #include "net/http/http_response_headers.h"
 #include "net/nqe/effective_connection_type.h"
 #include "net/nqe/network_quality_estimator.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/ssl/ssl_platform_key.h"
 #include "net/ssl/ssl_private_key.h"
 #include "net/url_request/redirect_info.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_status.h"
 
+
+#include "content/browser/frame_host/frame_tree.h"
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/frame_host/render_frame_host_manager.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_entry.h"
+
 using base::TimeDelta;
 using base::TimeTicks;
 
 namespace content {
 namespace {
 
-void GetSSLStatusForRequest(const GURL& url,
-                            const net::SSLInfo& ssl_info,
-                            int child_id,
-                            CertStore* cert_store,
-                            SSLStatus* ssl_status) {
-  DCHECK(ssl_info.cert);
-  int cert_id = cert_store->StoreCert(ssl_info.cert.get(), child_id);
+void SetSSLStatus(const base::Callback<WebContents*(void)>& wc_getter,
+                  const GURL& url,
+                  const SSLStatus& ssl_status) {
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(wc_getter.Run());
+  if (!web_contents)
+    return;
+  NavigationEntry* pending_entry =
+      web_contents->GetController().GetPendingEntry();
+  if (!pending_entry)
+    return;
 
-  *ssl_status = SSLStatus(SSLPolicy::GetSecurityStyleForResource(
-                              url, cert_id, ssl_info.cert_status),
-                          cert_id, ssl_info);
+  // First check the pending entry. This is the case for the normal navigation
+  // case without redirect.
+  if (pending_entry->GetURL() == url) {
+    pending_entry->GetSSL() = ssl_status;

[estark, 2016/08/15 08:14:08]

Does this change the behavior of the omnibox during a navigation? Currently I
think the omnibox won't get updated to a lock icon until the navigation commits
(i.e. if you navigate to https://example.com, you won't get a lock in the
omnibox while waiting for the response, only when the navigation has committed).
I'm wondering if, with this change, you'll see a lock earlier.

If so, I'm not sure that's a bad thing, but I'll want to check with some omnibox
and UI people because I think it might affect an omnibox redesign that's in
progress (https://crbug.com/622531).

(I would patch this in and check it out myself, but I'm away from my workstation
this week so it's a little hard to test subtle UI changes like this.)

[jam, 2016/08/15 19:23:58]

I just tried adding breakpoints and looking at the UI, and it doesn't appear
that the lock icon changes until the commit message arrives.

+    return;
+  }
+
+  // Handle redirect case since url won't match now. So cache the SSLStatus
+  // along with the url which we'll check when the commit happens.
+  RenderFrameHostImpl* rfh =
+      web_contents->GetFrameTree()->root()->render_manager()->
+          current_frame_host();
+  if (rfh)
+    rfh->SetSSLStatusForPendingNavigate(url, ssl_status);
 }
 
 void PopulateResourceResponse(ResourceRequestInfoImpl* info,
                               net::URLRequest* request,
                               CertStore* cert_store,
                               ResourceResponse* response) {
   response->head.request_time = request->request_time();
   response->head.response_time = request->response_time();
   response->head.headers = request->response_headers();
   request->GetCharset(&response->head.charset);
@@ skipping 31 matching lines @@
   if (service_worker_info)
     service_worker_info->GetExtraResponseInfo(&response->head);
   AppCacheInterceptor::GetExtraResponseInfo(
       request, &response->head.appcache_id,
       &response->head.appcache_manifest_url);
   if (info->is_load_timing_enabled())
     request->GetLoadTimingInfo(&response->head.load_timing);
 
   if (request->ssl_info().cert.get()) {
     SSLStatus ssl_status;
-    GetSSLStatusForRequest(request->url(), request->ssl_info(),
-                           info->GetChildID(), cert_store, &ssl_status);
+    ResourceLoader::GetSSLStatusForRequest(
+        request->url(), request->ssl_info(), info->GetChildID(),
+        cert_store, &ssl_status);
+
+    if (info->IsMainFrame()) {

[estark, 2016/08/15 08:14:08]

1. The comment on ResourceRequestInfo::IsMainFrame() makes me think that this is
true for any request issued by the main frame. What you really want here is to
somehow check if this is the main resource request for the main frame, isn't it?
Maybe `info->GetResourceType() == MAIN_FRAME` is what you want?

2. I'm not sure if this is racy or not -- maybe you or Nasko know for sure -- is
there any way the pending navigation entry could change before this makes it to
the UI thread?

[jam, 2016/08/15 19:23:58]

ah, thanks, yes that's what I wanted. fixed.
I think it's possible, that's why I send the URL to check that it matches.

+      // Store the SSLStatus in the pending NavigationEntry so that if it's
+      // committed we can store it in the SSLManager.
+      BrowserThread::PostTask(BrowserThread::UI,
+                              FROM_HERE,
+                              base::Bind(SetSSLStatus,
+                                         info->GetWebContentsGetterForRequest(),
+                                         request->url(),
+                                         ssl_status));
+    }
     response->head.security_info = SerializeSecurityInfo(ssl_status);
     response->head.has_major_certificate_errors =
         net::IsCertStatusError(ssl_status.cert_status) &&
         !net::IsCertStatusMinorError(ssl_status.cert_status);
     if (info->ShouldReportRawHeaders()) {
       // Only pass the Signed Certificate Timestamps (SCTs) when the network
       // panel of the DevTools is open, i.e. ShouldReportRawHeaders() is set.
       // These data are used to populate the requests in the security panel too.
       response->head.signed_certificate_timestamps =
           request->ssl_info().signed_certificate_timestamps;
     }
   } else {
     // We should not have any SSL state.
     DCHECK(!request->ssl_info().cert_status);
     DCHECK_EQ(request->ssl_info().security_bits, -1);
     DCHECK_EQ(request->ssl_info().key_exchange_info, 0);
     DCHECK(!request->ssl_info().connection_status);
   }
 }
 
 }  // namespace
 
+void ResourceLoader::GetSSLStatusForRequest(const GURL& url,
+                                            const net::SSLInfo& ssl_info,
+                                            int child_id,
+                                            CertStore* cert_store,
+                                            SSLStatus* ssl_status) {
+  DCHECK(ssl_info.cert);
+  int cert_id = cert_store->StoreCert(ssl_info.cert.get(), child_id);
+
+  *ssl_status = SSLStatus(SSLPolicy::GetSecurityStyleForResource(
+                              url, cert_id, ssl_info.cert_status),
+                          cert_id, ssl_info);
+}
+
 ResourceLoader::ResourceLoader(std::unique_ptr<net::URLRequest> request,
                                std::unique_ptr<ResourceHandler> handler,
                                CertStore* cert_store,
                                ResourceLoaderDelegate* delegate)
     : deferred_stage_(DEFERRED_NONE),
       request_(std::move(request)),
       handler_(std::move(handler)),
       delegate_(delegate),
       is_transferring_(false),
       times_cancelled_before_request_start_(0),
@@ skipping 587 matching lines @@
     }
 
     UMA_HISTOGRAM_ENUMERATION("Net.Prefetch.Pattern", status, STATUS_MAX);
   } else if (request_->response_info().unused_since_prefetch) {
     TimeDelta total_time = base::TimeTicks::Now() - request_->creation_time();
     UMA_HISTOGRAM_TIMES("Net.Prefetch.TimeSpentOnPrefetchHit", total_time);
   }
 }
 
 }  // namespace content