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

content/browser/loader/resource_loader.cc

Index: content/browser/loader/resource_loader.cc
diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc
index d9a3b8f4ee27c37eeeec7c42877664cfab996d76..b9314d30e33da9381c7b0c3abde9b584ec4de388 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -46,23 +46,47 @@
 #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,
@@ -114,8 +138,20 @@ void PopulateResourceResponse(ResourceRequestInfoImpl* info,
 
   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) &&
@@ -138,6 +174,19 @@ void PopulateResourceResponse(ResourceRequestInfoImpl* info,
 
 }  // 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,