Remove usage of SSLStatus in RenderFrameImpl.

content/renderer/render_frame_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.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 43 matching lines @@
 #include "content/common/frame_messages.h"
 #include "content/common/frame_owner_properties.h"
 #include "content/common/frame_replication_state.h"
 #include "content/common/gpu/client/context_provider_command_buffer.h"
 #include "content/common/input_messages.h"
 #include "content/common/navigation_params.h"
 #include "content/common/page_messages.h"
 #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"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/context_menu_params.h"
 #include "content/public/common/file_chooser_file_info.h"
 #include "content/public/common/file_chooser_params.h"
@@ skipping 703 matching lines @@
   bool useBinaryEncoding() override { return params_.mhtml_binary_encoding; }
 
  private:
   const FrameMsg_SerializeAsMHTML_Params& params_;
   std::set<std::string>* digests_of_uris_of_serialized_resources_;
 
   DISALLOW_COPY_AND_ASSIGN(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,
     const blink::WebCString& security_info) {
   blink::WebFrame* main_frame = frame->top();
 
   // If the main frame is remote, then it must be cross-site and
   // therefore this subresource's certificate errors are potentially
   // interesting to the browser (not redundant with the main frame's
   // main resource).
   if (main_frame->isWebRemoteFrame())
     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) {
   return request.httpMethod().utf8() == "POST";
 }
 
 #if defined(OS_ANDROID)
 // Returns true if WMPI should be used for playback, false otherwise.
 //
 // Note that HLS and MP4 detection are pre-redirect and path-based. It is
@@ skipping 5505 matching lines @@
   // event target. Potentially a Pepper plugin will receive the event.
   // In order to tell whether a plugin gets the last mouse event and which it
   // is, we set |pepper_last_mouse_event_target_| to null here. If a plugin gets
   // the event, it will notify us via DidReceiveMouseEvent() and set itself as
   // |pepper_last_mouse_event_target_|.
   pepper_last_mouse_event_target_ = nullptr;
 #endif
 }
 
 }  // namespace content