Attach a SecurityStyle to each request in ResourceLoader

content/browser/ssl/ssl_manager.cc

Index: content/browser/ssl/ssl_manager.cc
diff --git a/content/browser/ssl/ssl_manager.cc b/content/browser/ssl/ssl_manager.cc
index ab1048c31404dc35e1eb261f1aab427c8d76f10e..48e3aa1b559aec7341194ac034d252c62ed05bd5 100644
--- a/content/browser/ssl/ssl_manager.cc
+++ b/content/browser/ssl/ssl_manager.cc
@@ -121,7 +121,8 @@ void SSLManager::DidCommitProvisionalLoad(const LoadCommittedDetails& details) {
     }
   }
 
-  UpdateEntry(entry);
+  if (!UpdateEntry(entry))

[estark, 2015/07/21 22:25:16]

If you're wondering why this change is necessary... before, the assignment on
line 120 meant that entry->GetSSL().security_style was UNKNOWN before calling
UpdateEntry(). (Since |details.ssl_status| comes from the deserialized renderer
information, which always had a style of UNKNOWN before this CL.) So
UpdateEntry() would assign a SecurityStyle, and then observe that the SSLStatus
had changed so it was time to call DidChangeVisibleSSLState().

Now that the deserialized info contains a security style, we need to make sure
that DidChangeVisibleSSLState() gets called when navigating.

[davidben, 2015/07/22 20:56:57]

This probably wants a comment. It took me a while to realize that what you were
doing is to fire NotifyDidChangeVisibleSSLState for line 120 if UpdateEntry
*didn't* already fire it for you. (It's also not quite right if entry is NULL or
if details.is_main_frame is true, right? Though we probably don't care much
there.)

Another possibility is to call policy()->UpdateEntry directly so that
DidChangeVisibleSSLState isn't automatically notified. And then the outer
function can coalesce and notify just once; the entirety of UpdateEntry seems to
just exist to do the automatic notifying anyway.

[estark, 2015/07/22 22:56:55]

Done. (Changed it to call policy()->UpdateEntry() directly and added a comment.)

+    NotifyDidChangeVisibleSSLState();
 }
 
 void SSLManager::DidDisplayInsecureContent() {
@@ -176,20 +177,27 @@ void SSLManager::DidReceiveResourceRedirect(
   //               HTTP request to https://attacker.com/payload.js.
 }
 
-void SSLManager::UpdateEntry(NavigationEntryImpl* entry) {
+bool SSLManager::UpdateEntry(NavigationEntryImpl* entry) {
   // We don't always have a navigation entry to update, for example in the
   // case of the Web Inspector.
   if (!entry)
-    return;
+    return false;
 
   SSLStatus original_ssl_status = entry->GetSSL();  // Copy!
 
+  policy()->UpdateEntry(entry, controller_->delegate()->GetWebContents());
+
+  if (entry->GetSSL().Equals(original_ssl_status))
+    return false;
+
+  NotifyDidChangeVisibleSSLState();
+  return true;
+}
+
+void SSLManager::NotifyDidChangeVisibleSSLState() {
   WebContentsImpl* contents =
       static_cast<WebContentsImpl*>(controller_->delegate()->GetWebContents());
-  policy()->UpdateEntry(entry, contents);
-
-  if (!entry->GetSSL().Equals(original_ssl_status))
-    contents->DidChangeVisibleSSLState();
+  contents->DidChangeVisibleSSLState();
 }
 
 }  // namespace content