Remove usage of SSLStatus in RenderFrameImpl.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 9c3d27cd842d5c8bf4f34ed516c1da89439a629a..5286b2063b454be37c27eefd04d85844f1de29f8 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -61,7 +61,6 @@
 #include "content/common/savable_subframe.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "content/common/site_isolation_policy.h"
-#include "content/common/ssl_status_serialization.h"
 #include "content/common/swapped_out_messages.h"
 #include "content/common/view_messages.h"
 #include "content/public/common/bindings_policy.h"
@@ -785,14 +784,11 @@ class MHTMLPartsGenerationDelegate
 };
 
 // Returns true if a subresource certificate error (described by |url|
-// and |security_info|) is "interesting" to the browser process. The
-// browser process is interested in certificate errors that differ from
-// certificate errors encountered while loading the main frame's main
-// resource. In other words, it would be confusing to mark a page as
-// having displayed/run insecure content when the whole page has already
-// been marked as insecure for the same reason, so subresources with the
-// same certificate errors as the main resource are not sent to the
-// browser process.
+// is "interesting" to the browser process. The browser process is interested
+// in certificate errors that differ from certificate errors encountered while
+// loading the main frame's main resource. In other words, it would be confusing
+// to mark a page as having displayed/run insecure content when the whole page
+// has already been marked as insecure for the same reason.
 bool IsContentWithCertificateErrorsRelevantToUI(
     blink::WebFrame* frame,
     const blink::WebURL& url,
@@ -807,26 +803,11 @@ bool IsContentWithCertificateErrorsRelevantToUI(
     return true;
 
   WebDataSource* main_ds = main_frame->toWebLocalFrame()->dataSource();
-  content::SSLStatus ssl_status;
-  content::SSLStatus main_resource_ssl_status;
-  CHECK(DeserializeSecurityInfo(security_info, &ssl_status));
-  CHECK(DeserializeSecurityInfo(main_ds->response().securityInfo(),
-                                &main_resource_ssl_status));
 
   // Do not send 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.
-  return (!url::Origin(GURL(url)).IsSameOriginWith(
-              url::Origin(GURL(main_ds->request().url()))) ||
-          main_resource_ssl_status.cert_id != ssl_status.cert_id ||
-          main_resource_ssl_status.cert_status != ssl_status.cert_status ||
-          main_resource_ssl_status.security_bits != ssl_status.security_bits ||
-          main_resource_ssl_status.connection_status !=
-              ssl_status.connection_status);
+  // as errors that occured during the main page load.
+  return !url::Origin(GURL(url)).IsSameOriginWith(
+             url::Origin(GURL(main_ds->request().url())));

[jam, 2016/08/03 19:33:45]

I've kept this check because otherwise three SSL browser tests
(SSLUITest.TestRefNavigation, SSLUITest.TestBadFrameNavigation,
SSLUITest.TestUnsafeContentsWithUserException) were flaking. The reason was
their last call to CheckAuthenticationBrokenState that checked for
AuthState::NONE. The favicon loading for the page was sending a
FrameHostMsg_DidDisplayContentWithCertificateErrors which made the auth_state to
be sometimes DISPLAYED_INSECURE_CONTENT if the load for the favicon finished
before the CheckAuthenticationBrokenState call.

Alternatively I can change the CheckAuthenticationBrokenState calls to not look
at the auth state for the last call in these three tests. I had that locally but
then changed my mind and left this check instead of removing the method
altogether. WDYT?

[estark, 2016/08/03 22:31:37]

Theoretically, checking only the origin could get us in to trouble here. Suppose
that https://foo.com has a certificate error that only happens on some requests
(for example only if you happen to hit certain front-end servers or something).
Suppose you click through a certificate warning on https://foo.com -- that
decision gets remembered for a week. Then later you go visit https://foo.com
again and happen to not get the certificate error, but the page loads a
same-origin subresource that does happen to get the certificate error. With this
check, the browser wouldn't get notified of the same-origin subresource
certificate error, so the user wouldn't get notified that there is broken-HTTPS
content on the page.

I fully acknowledge that this is a very theoretical situation that would
probably never happen in real life. But it makes me a little nervous, so I guess
I'd prefer changing the tests.

[jam, 2016/08/03 23:55:44]

Thanks for the example, I updated the test and removed the method.

 }
 
 bool IsHttpPost(const blink::WebURLRequest& request) {