Attach a SecurityStyle to each request in ResourceLoader

content/browser/ssl/ssl_manager.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/ssl/ssl_manager.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 103 matching lines @@
   NavigationEntryImpl* entry = controller_->GetLastCommittedEntry();
 
   if (details.is_main_frame) {
     if (entry) {
       // We may not have an entry if this is a navigation to an initial blank
       // page. Add the new data we have.
       entry->GetSSL() = details.ssl_status;
     }
   }
 
-  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() {
   UpdateEntry(controller_->GetLastCommittedEntry());
 }
 
 void SSLManager::DidRunInsecureContent(const std::string& security_origin) {
   NavigationEntryImpl* navigation_entry = controller_->GetLastCommittedEntry();
   policy()->DidRunInsecureContent(navigation_entry, security_origin);
   UpdateEntry(navigation_entry);
@@ skipping 34 matching lines @@
 
 void SSLManager::DidReceiveResourceRedirect(
     const ResourceRedirectDetails& details) {
   // TODO(abarth): Make sure our redirect behavior is correct.  If we ever see a
   //               non-HTTPS resource in the redirect chain, we want to trigger
   //               insecure content, even if the redirect chain goes back to
   //               HTTPS.  This is because the network attacker can redirect the
   //               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