Downgrade lock icon for broken-HTTPS subresources

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 e4c71e3dc4e0f8b2c520afe4cb9d9979d51140d9..6f08c4c80f4344eefc4008d595b81c9808887804 100644
--- a/content/browser/ssl/ssl_manager.cc
+++ b/content/browser/ssl/ssl_manager.cc
@@ -127,16 +127,34 @@ void SSLManager::DidCommitProvisionalLoad(const LoadCommittedDetails& details) {
   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);
 }
 
+bool SSLManager::IsContentWithCertificateErrorsRelevant(const GURL& url,
+                                                        const SSLStatus& ssl) {
+  // Do not handle subresource certificate errors if the main page is
+  // not loaded over HTTPS.
+  NavigationEntryImpl* entry = controller_->GetLastCommittedEntry();
+  if (!entry || !entry->GetSSL().cert_id)

[jww, 2015/11/20 01:25:08]

We talked about this a while back, and I forget all the details, but doesn't a
cert_id of 0 indicate that it's unset *or* invalid? And if the SSLStatus is
invalid, is that really "no errors"? These are really honest questions, because
I can't remember.

[estark, 2015/11/23 23:40:24]

So I ended up moving this to the renderer and changing the check to just be
SchemeIsCryptographic() on the main resource. Does that seem right to you?

[jww, 2015/11/25 19:24:02]

It doesn't seem like there's any increased security risk here, since ultimately
the renderer is deciding the state anyway, and his could all be ignored. The
only consideration is if this will someday all end up being in the Browser
anyway, but I'm not too worried about that.

+    return false;
+
+  // Do not handle subresource certificate errors if they are the same
+  // as errors that occured during the main page load. This compares
+  // most, but not all, fields of SSLStatus. For example, this check
+  // does not compare |content_status| because the navigation entry
+  // might have mixed content but also have the exact same SSL
+  // connection properties as the subresource, thereby making the
+  // subresource errors duplicative.

[jww, 2015/11/20 01:25:08]

Why is security_style not covered by this? Because all of the following bits
imply a particular style?

[estark, 2015/11/23 23:40:24]

I think just an omission on my part. (The following bits do imply a particular
style, but I think it's better not to rely on that.)

+  return (!url::Origin(entry->GetURL()).IsSameOriginWith(url::Origin(url)) ||
+          entry->GetSSL().cert_id != ssl.cert_id ||
+          entry->GetSSL().cert_status != ssl.cert_status ||
+          entry->GetSSL().security_bits != ssl.security_bits ||
+          entry->GetSSL().connection_status != ssl.connection_status);
+}
+
 void SSLManager::DidLoadFromMemoryCache(
     const LoadFromMemoryCacheDetails& details) {
   // Simulate loading this resource through the usual path.