From f1aa907921ed7fd95c8a9fb35e41aaca36a31067 Mon Sep 17 00:00:00 2001 From: mmenke Date: Mon, 18 May 2015 09:21:04 -0700 Subject: [PATCH] Remove LOAD_SUB_FRAME load flag. It's not used in net, and net/ knowing about frames is a layering violation. External consumers that depended on it now use the corresponding ResourceType instead. BUG=426442 Review URL: https://codereview.chromium.org/1134733004 Cr-Commit-Position: refs/heads/master@{#330370} --- chrome/browser/net/chrome_network_delegate.cc | 8 ++++++-- chrome/browser/net/connect_interceptor.cc | 21 ++++++++++++++++++--- .../policy/url_blacklist_manager_unittest.cc | 17 +++-------------- .../policy/core/browser/url_blacklist_manager.cc | 16 ++++------------ .../policy/core/browser/url_blacklist_manager.h | 17 ++++++++--------- .../browser/loader/resource_dispatcher_host_impl.cc | 7 +------ net/base/load_flags_list.h | 18 +++++++----------- 7 files changed, 47 insertions(+), 57 deletions(-) diff --git a/chrome/browser/net/chrome_network_delegate.cc b/chrome/browser/net/chrome_network_delegate.cc index 9348cef93a3e..4953a723c0be 100644 --- a/chrome/browser/net/chrome_network_delegate.cc +++ b/chrome/browser/net/chrome_network_delegate.cc @@ -377,9 +377,13 @@ int ChromeNetworkDelegate::OnBeforeURLRequest( // TODO(joaodasilva): This prevents extensions from seeing URLs that are // blocked. However, an extension might redirect the request to another URL, // which is not blocked. + + const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request); int error = net::ERR_BLOCKED_BY_ADMINISTRATOR; - if (url_blacklist_manager_ && - url_blacklist_manager_->IsRequestBlocked(*request, &error)) { + if (info && content::IsResourceTypeFrame(info->GetResourceType()) && + url_blacklist_manager_ && + url_blacklist_manager_->ShouldBlockRequestForFrame( + request->url(), &error)) { // URL access blocked by policy. request->net_log().AddEvent( net::NetLog::TYPE_CHROME_POLICY_ABORTED_REQUEST, diff --git a/chrome/browser/net/connect_interceptor.cc b/chrome/browser/net/connect_interceptor.cc index 494e4f57fb8e..40bf61ce8760 100644 --- a/chrome/browser/net/connect_interceptor.cc +++ b/chrome/browser/net/connect_interceptor.cc @@ -5,6 +5,8 @@ #include "chrome/browser/net/connect_interceptor.h" #include "chrome/browser/net/predictor.h" +#include "content/public/browser/resource_request_info.h" +#include "content/public/common/resource_type.h" #include "net/base/load_flags.h" #include "net/url_request/url_request.h" @@ -25,11 +27,22 @@ void ConnectInterceptor::WitnessURLRequest(net::URLRequest* request) { if (request_scheme_host == GURL::EmptyGURL()) return; + const content::ResourceRequestInfo* info = + content::ResourceRequestInfo::ForRequest(request); + bool is_main_frame = false; + bool is_sub_frame = false; + // TODO(mmenke): Should the predictor really be fed requests without a + // ResourceRequestInfo? + if (info) { + content::ResourceType resource_type = info->GetResourceType(); + is_main_frame = (resource_type == content::RESOURCE_TYPE_MAIN_FRAME); + is_sub_frame = (resource_type == content::RESOURCE_TYPE_SUB_FRAME); + } + // Learn what URLs are likely to be needed during next startup. predictor_->LearnAboutInitialNavigation(request_scheme_host); bool redirected_host = false; - bool is_subresource = !(request->load_flags() & net::LOAD_MAIN_FRAME); if (request->referrer().empty()) { if (request->url() != request->original_url()) { // This request was completed with a redirect. @@ -57,9 +70,10 @@ void ConnectInterceptor::WitnessURLRequest(net::URLRequest* request) { } else { GURL referring_scheme_host = GURL(request->referrer()).GetWithEmptyPath(); // Learn about our referring URL, for use in the future. - if (is_subresource && timed_cache_.WasRecentlySeen(referring_scheme_host)) + if (!is_main_frame && timed_cache_.WasRecentlySeen(referring_scheme_host)) { predictor_->LearnFromNavigation(referring_scheme_host, request_scheme_host); + } if (referring_scheme_host == request_scheme_host) { // We've already made any/all predictions when we navigated to the // referring host, so we can bail out here. @@ -74,9 +88,10 @@ void ConnectInterceptor::WitnessURLRequest(net::URLRequest* request) { // Subresources for main frames usually get predicted when we detected the // main frame request - way back in RenderViewHost::Navigate. So only handle // predictions now for subresources or for redirected hosts. - if ((request->load_flags() & net::LOAD_SUB_FRAME) || redirected_host) + if (is_sub_frame || redirected_host) { predictor_->PredictFrameSubresources(request_scheme_host, request->first_party_for_cookies()); + } return; } diff --git a/chrome/browser/policy/url_blacklist_manager_unittest.cc b/chrome/browser/policy/url_blacklist_manager_unittest.cc index 6bdf1bcba25a..a21df066ef5e 100644 --- a/chrome/browser/policy/url_blacklist_manager_unittest.cc +++ b/chrome/browser/policy/url_blacklist_manager_unittest.cc @@ -7,6 +7,7 @@ #include #include "base/basictypes.h" +#include "base/bind.h" #include "base/callback.h" #include "base/message_loop/message_loop.h" #include "base/prefs/pref_registry_simple.h" @@ -18,10 +19,6 @@ #include "google_apis/gaia/gaia_urls.h" #include "net/base/load_flags.h" #include "net/base/net_errors.h" -#include "net/base/request_priority.h" -#include "net/url_request/url_request.h" -#include "net/url_request/url_request_context.h" -#include "net/url_request/url_request_test_util.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/gurl.h" @@ -642,17 +639,9 @@ TEST_F(URLBlacklistManagerTest, DontBlockResources) { blacklist_manager_->SetBlacklist(blacklist.Pass()); EXPECT_TRUE(blacklist_manager_->IsURLBlocked(GURL("http://google.com"))); - net::TestURLRequestContext context; - scoped_ptr request(context.CreateRequest( - GURL("http://google.com"), net::DEFAULT_PRIORITY, NULL)); - int reason = net::ERR_UNEXPECTED; - // Background requests aren't filtered. - EXPECT_FALSE(blacklist_manager_->IsRequestBlocked(*request.get(), &reason)); - - // Main frames are filtered. - request->SetLoadFlags(net::LOAD_MAIN_FRAME); - EXPECT_TRUE(blacklist_manager_->IsRequestBlocked(*request.get(), &reason)); + EXPECT_TRUE(blacklist_manager_->ShouldBlockRequestForFrame( + GURL("http://google.com"), &reason)); EXPECT_EQ(net::ERR_BLOCKED_BY_ADMINISTRATOR, reason); } diff --git a/components/policy/core/browser/url_blacklist_manager.cc b/components/policy/core/browser/url_blacklist_manager.cc index 9f86a3c6a210..edffe0379d67 100644 --- a/components/policy/core/browser/url_blacklist_manager.cc +++ b/components/policy/core/browser/url_blacklist_manager.cc @@ -18,9 +18,7 @@ #include "components/policy/core/common/policy_pref_names.h" #include "components/pref_registry/pref_registry_syncable.h" #include "net/base/filename_util.h" -#include "net/base/load_flags.h" #include "net/base/net_errors.h" -#include "net/url_request/url_request.h" #include "url/url_constants.h" #include "url/url_parse.h" @@ -490,22 +488,16 @@ bool URLBlacklistManager::IsURLBlocked(const GURL& url) const { return blacklist_->IsURLBlocked(url); } -bool URLBlacklistManager::IsRequestBlocked( - const net::URLRequest& request, int* reason) const { +bool URLBlacklistManager::ShouldBlockRequestForFrame(const GURL& url, + int* reason) const { DCHECK(io_task_runner_->RunsTasksOnCurrentThread()); -#if !defined(OS_IOS) - // TODO(joaodasilva): iOS doesn't set these flags. http://crbug.com/338283 - int filter_flags = net::LOAD_MAIN_FRAME | net::LOAD_SUB_FRAME; - if ((request.load_flags() & filter_flags) == 0) - return false; -#endif bool block = false; - if (override_blacklist_.Run(request.url(), &block, reason)) + if (override_blacklist_.Run(url, &block, reason)) return block; *reason = net::ERR_BLOCKED_BY_ADMINISTRATOR; - return IsURLBlocked(request.url()); + return IsURLBlocked(url); } // static diff --git a/components/policy/core/browser/url_blacklist_manager.h b/components/policy/core/browser/url_blacklist_manager.h index aed642b20010..03e44ad19b79 100644 --- a/components/policy/core/browser/url_blacklist_manager.h +++ b/components/policy/core/browser/url_blacklist_manager.h @@ -27,10 +27,6 @@ class ListValue; class SequencedTaskRunner; } -namespace net { -class URLRequest; -} - namespace user_prefs { class PrefRegistrySyncable; } @@ -161,14 +157,17 @@ class POLICY_EXPORT URLBlacklistManager { // from the IO thread. bool IsURLBlocked(const GURL& url) const; - // Returns true if |request| is blocked by the current blacklist. - // Only main frame and sub frame requests may be blocked; other sub resources - // or background downloads (e.g. extensions updates, sync, etc) are not - // filtered. The sync signin page is also not filtered. + // Returns true if a request for |url| is blocked by the current blacklist. + // + // Should only be called for requests for frames (Main frames or subframes). + // Other subresources or background downloads (e.g. extensions updates, sync, + // etc) should not be filtered. The sync signin page will also not be + // filtered. + // // |reason| is populated with the exact reason for blocking the url if and // only if the return value is true otherwise it is left untouched. // Must be called from the IO thread. - bool IsRequestBlocked(const net::URLRequest& request, int* reason) const; + bool ShouldBlockRequestForFrame(const GURL& url, int* reason) const; // Replaces the current blacklist. Must be called on the IO thread. // Virtual for testing. diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc index 9828244029a1..ddbb72ad7cb9 100644 --- a/content/browser/loader/resource_dispatcher_host_impl.cc +++ b/content/browser/loader/resource_dispatcher_host_impl.cc @@ -1928,11 +1928,8 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest( int load_flags = info.begin_params.load_flags; load_flags |= net::LOAD_VERIFY_EV_CERT; - if (info.is_main_frame) { + if (info.is_main_frame) load_flags |= net::LOAD_MAIN_FRAME; - } else { - load_flags |= net::LOAD_SUB_FRAME; - } // Add a flag to selectively bypass the data reduction proxy if the resource // type is not an image. load_flags |= net::LOAD_BYPASS_DATA_REDUCTION_PROXY; @@ -2358,8 +2355,6 @@ int ResourceDispatcherHostImpl::BuildLoadFlagsForRequest( load_flags |= net::LOAD_VERIFY_EV_CERT; if (request_data.resource_type == RESOURCE_TYPE_MAIN_FRAME) { load_flags |= net::LOAD_MAIN_FRAME; - } else if (request_data.resource_type == RESOURCE_TYPE_SUB_FRAME) { - load_flags |= net::LOAD_SUB_FRAME; } else if (request_data.resource_type == RESOURCE_TYPE_PREFETCH) { load_flags |= net::LOAD_PREFETCH; } diff --git a/net/base/load_flags_list.h b/net/base/load_flags_list.h index 8f3fe0e08c4a..cdb520599a9a 100644 --- a/net/base/load_flags_list.h +++ b/net/base/load_flags_list.h @@ -59,37 +59,33 @@ LOAD_FLAG(IGNORE_ALL_CERT_ERRORS, 1 << 11) // page is loaded. LOAD_FLAG(MAIN_FRAME, 1 << 12) -// Indicate that this is a sub frame, and hence it might have subresources that -// should be speculatively resolved, or even speculatively preconnected. -LOAD_FLAG(SUB_FRAME, 1 << 13) - // If present, intercept actual request/response headers from network stack // and report them to renderer. This includes cookies, so the flag is only // respected if renderer has CanReadRawCookies capability in the security // policy. -LOAD_FLAG(REPORT_RAW_HEADERS, 1 << 14) +LOAD_FLAG(REPORT_RAW_HEADERS, 1 << 13) // Indicates that this load was motivated by the rel=prefetch feature, // and is (in theory) not intended for the current frame. -LOAD_FLAG(PREFETCH, 1 << 15) +LOAD_FLAG(PREFETCH, 1 << 14) // Indicates that this is a load that ignores limits and should complete // immediately. -LOAD_FLAG(IGNORE_LIMITS, 1 << 16) +LOAD_FLAG(IGNORE_LIMITS, 1 << 15) // Indicates that the operation is somewhat likely to be due to an // explicit user action. This can be used as a hint to treat the // request with higher priority. -LOAD_FLAG(MAYBE_USER_GESTURE, 1 << 17) +LOAD_FLAG(MAYBE_USER_GESTURE, 1 << 16) // Indicates that the username:password portion of the URL should not // be honored, but that other forms of authority may be used. -LOAD_FLAG(DO_NOT_USE_EMBEDDED_IDENTITY, 1 << 18) +LOAD_FLAG(DO_NOT_USE_EMBEDDED_IDENTITY, 1 << 17) // Send request directly to the origin if the effective proxy is the data // reduction proxy. // TODO(rcs): Remove this flag as soon as http://crbug.com/339237 is resolved. -LOAD_FLAG(BYPASS_DATA_REDUCTION_PROXY, 1 << 19) +LOAD_FLAG(BYPASS_DATA_REDUCTION_PROXY, 1 << 18) // Indicates the the request is an asynchronous revalidation. -LOAD_FLAG(ASYNC_REVALIDATION, 1 << 20) +LOAD_FLAG(ASYNC_REVALIDATION, 1 << 19) -- 2.11.4.GIT