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

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 1109 matching lines @@
     // to show chrome://bookmarks/#1 when the bookmarks webui extension changes
     // the URL.
     update_virtual_url = needs_update;
   }
 
   // Don't use the page type from the pending entry. Some interstitial page
   // may have set the type to interstitial. Once we commit, however, the page
   // type must always be normal or error.
   new_entry->set_page_type(params.url_is_unreachable ? PAGE_TYPE_ERROR
                                                      : PAGE_TYPE_NORMAL);
-  new_entry->SetURL(params.url);
+  if (params.url != new_entry->GetURL()) {

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

Is this check just an optimization? I'm concerned that two different navigations
to the same URL could have different SSL properties.

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

It wasn't; looking at it again it was poorly written. I've reworked it a bit to
make it clearer.

When the SSLState is saved on a pending NavigationEntry from ResourceLoader, we
want to use it. That's what the url check was effectively doing.
You mean that the server would return different certificates for the same url?

[mmenke, 2016/08/15 22:48:31]

It could be two different servers with the same hostname.  Server could have
updated its cert.  Or one could be a captive portal that suddenly appeared
(Along with a cert error click through).

One response could have been served out of cache (With a cached cert), while the
next could require us to go to the server (Again, may have to click through cert
error due to captive portal for the second URL, too), etc.

+    new_entry->SetURL(params.url);
+    // New URL means that we can't use the old SSLStatus.
+    new_entry->GetSSL() = SSLStatus();
+  }
+
+  if (rfh->frame_tree_node()->IsMainFrame()) {
+    // In case of redirects (same process or across different ones), the
+    // SSLStatus was cached on the pending RFH so use it now.
+    SSLStatus ssl_status;
+    if (rfh->GetSSLStatusForPendingNavigate(params.url, &ssl_status))

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

Just checking -- we're guaranteed that the returned SSLStatus corresponds to
*this* navigation? Is there any way we could call
SetSSLStatusForPendingNavigate() but then not Get() it until some later
navigation (different than the one that we called Set() for)?

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

I don't think this is guaranteed. i.e. in the
SitePerProcessIgnoreCertErrorsBrowserTest.CrossSiteRedirectCertificateStore
case, the navigation IDs aren't the same for the pending naventry and the new
process's commit message, but we do want to use this cached SSLStatus on the RFH

+      new_entry->GetSSL() = ssl_status;
+  }
+
   if (update_virtual_url)
     UpdateVirtualURLToURL(new_entry.get(), params.url);
   new_entry->SetReferrer(params.referrer);
   new_entry->SetPageID(params.page_id);
   new_entry->SetTransitionType(params.transition);
   new_entry->set_site_instance(
       static_cast<SiteInstanceImpl*>(rfh->GetSiteInstance()));
   new_entry->SetOriginalRequestURL(params.original_request_url);
   new_entry->SetIsOverridingUserAgent(params.is_overriding_user_agent);
 
@@ skipping 953 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content