Remove unnecessary SSLRequestInfo class

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/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/supports_user_data.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/browser/ssl/ssl_cert_error_handler.h"
 #include "content/browser/ssl/ssl_policy.h"
-#include "content/browser/ssl/ssl_request_info.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/ssl_status_serialization.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/load_from_memory_cache_details.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/resource_request_details.h"
 #include "content/public/common/ssl_status.h"
 #include "net/url_request/url_request.h"
 
@@ skipping 108 matching lines @@
 
 void SSLManager::DidRunInsecureContent(const GURL& security_origin) {
   NavigationEntryImpl* navigation_entry = controller_->GetLastCommittedEntry();
   policy()->DidRunInsecureContent(navigation_entry, security_origin);
   UpdateEntry(navigation_entry);
 }
 
 void SSLManager::DidLoadFromMemoryCache(
     const LoadFromMemoryCacheDetails& details) {
   // Simulate loading this resource through the usual path.
-  // Note that we specify SUB_RESOURCE as the resource type as WebCore only
-  // caches sub-resources.
-  // This resource must have been loaded with no filtering because filtered
-  // resouces aren't cachable.

[felt, 2016/08/04 17:16:09]

I'm puzzled by this part. Someone, at some point, thought it was important to
note that the resource type matters for the policy.

But, obviously we are not checking the resource type now. I'd like to be sure
that isn't a bug. I can't find anything in recent history where it looks like we
would need to check the resource type though... so it seems OK.

Did you look into this?

[estark, 2016/08/04 17:25:04]

My guess is that this dates back to, oh, 2009 or so, when this class did a check
for mixed content. https://codereview.chromium.org/113391/ is what I looked at
-- in that CL, SSLPolicy checks for mixed content using the resource type. In
the history of Chrome that I've fabricated in my mind, at some point the mixed
content check moved out of here and into Blink (where it lives now) because more
context about the resource load was needed as the definition of mixed content
evolved.

[felt, 2016/08/04 17:26:24]

AHhh yes, that history makes sense.

-  scoped_refptr<SSLRequestInfo> info(new SSLRequestInfo(
-      details.url,
-      RESOURCE_TYPE_SUB_RESOURCE,
-      details.cert_id,
-      details.cert_status));
-
-  // Simulate loading this resource through the usual path.
-  policy()->OnRequestStarted(info.get());

[felt, 2016/08/04 17:16:09]

jww: Is it correct that DidLoadFromMemoryCache is triggering the logic in
OnRequestStarted? Let's say the following happens:

Monday: cert is valid. subresource is cached.
Tuesday: cert expires. user sees and clicks through error.
Wednesday: user goes to a page that loads the cached subresource.

That will remove the exception from Tuesday based on the cached subresource,
which seems kind of weird.

[jww, 2016/08/04 18:18:10]

It seems very possible that this would happen. We should probably have a browser
test to verify? Not sure how to write it, though.

I think it would be totally reasonable to have an explicit exemption of the
"revoke on good cert seen" for memory cache loaded resources, since the "good"
cert is old, and its not that we've seen the *server* issue a valid cert. Maybe
we should add a "bool cached_" member to SSLRequestInfo to track this?

+  policy()->OnRequestStarted(details.url, details.cert_id, details.cert_status);
 }
 
 void SSLManager::DidStartResourceResponse(
     const ResourceRequestDetails& details) {
-  scoped_refptr<SSLRequestInfo> info(new SSLRequestInfo(
-      details.url,
-      details.resource_type,
-      details.ssl_cert_id,
-      details.ssl_cert_status));
-
   // Notify our policy that we started a resource request.  Ideally, the
   // policy should have the ability to cancel the request, but we can't do
   // that yet.
-  policy()->OnRequestStarted(info.get());
+  policy()->OnRequestStarted(details.url, details.ssl_cert_id,
+                             details.ssl_cert_status);
 }
 
 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.
 }
@@ skipping 12 matching lines @@
     NotifyDidChangeVisibleSSLState();
 }
 
 void SSLManager::NotifyDidChangeVisibleSSLState() {
   WebContentsImpl* contents =
       static_cast<WebContentsImpl*>(controller_->delegate()->GetWebContents());
   contents->DidChangeVisibleSSLState();
 }
 
 }  // namespace content