Commit 27f8891c authored by Antonio Gomes's avatar Antonio Gomes Committed by Commit Bot

Migrate ContentHashFetcher to SimpleURLLoader

This CL migrates ContentHashFetcher away from URLRequestFetcher to
SimpleURLLoader.

In order to accomplish this, a few other related changes are also performed:

1) The regular flow of the code is ContentVerifier -> ContentHash ->
   ContentHashFetcher. In ContentVerifier, for instance, UI, IO and
   "File tasks" threads take place.
   Previously, the URLRequestFetcher logic residing in ContentHashFetcher
   executed in the "file tasks" thread.

   As part of the migration from URLRequestFetcher to SimpleURLLoader
   machinery, this CL also changes the ContentHashFetcher logic to
   execute to the IO thread.
   Note that it could be possible to keep the new SimpleURLLoader logic
   in the "file tasks" thread. However, this would impose a way bigger change,
   and require unittests to be considerably rewritten as well (see for
   patchsets 7, 8 and 9.
   The issue is that mojo objects are not thread safe. We could create a new
   URLLoaderFactory for the extensions thread, but then we'd need to create a
   new one when the network service crashes (Something this SharedURLLoaderFactory
   one, already bound to the IOThread, magically handles), but that would be
   significantly more complicated.

2) content_verifier_hash_fetch_behavior_browsertest.cc was changed
   to support running with the network service feature both enabled
   and disabled. This is a pattern that is also present in various other
   unittests.

In summary, the migration to use SimpleURLLoader performed here includes
also a change in the thread that ContentHashFetcher runs on, but no
functionality change is expected from this CL.

BUG=773295,844926

Change-Id: If570f69d01ff75ac59d8d043f8687621336dddcf
Reviewed-on: https://chromium-review.googlesource.com/1056587
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: default avatarDevlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarMatt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561480}
parent 824f7e1b
......@@ -6,12 +6,14 @@
#include <string>
#include "base/macros.h"
#include "base/test/bind_test_util.h"
#include "chrome/browser/extensions/browsertest_util.h"
#include "chrome/browser/extensions/chrome_content_verifier_delegate.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/extensions/extension_service.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/test/test_utils.h"
#include "content/public/test/url_loader_interceptor.h"
#include "extensions/browser/computed_hashes.h"
#include "extensions/browser/content_verifier/test_utils.h"
#include "extensions/browser/extension_file_task_runner.h"
......@@ -20,6 +22,7 @@
#include "extensions/browser/test_extension_registry_observer.h"
#include "extensions/common/file_util.h"
#include "net/url_request/test_url_request_interceptor.h"
#include "services/network/public/cpp/features.h"
namespace extensions {
......@@ -70,6 +73,12 @@ class ContentVerifierHashTest
ChromeContentVerifierDelegate::SetDefaultModeForTesting(base::nullopt);
}
void TearDownOnMainThread() override {
if (base::FeatureList::IsEnabled(network::features::kNetworkService))
url_loader_interceptor_.reset();
ExtensionBrowserTest::TearDownOnMainThread();
}
bool uses_enforce_strict_mode() {
return GetParam() == ContentVerificationMode::kEnforceStrict;
}
......@@ -317,7 +326,7 @@ class ContentVerifierHashTest
}
bool AddInterceptor() {
if (interceptor_) {
if (interceptor_ || url_loader_interceptor_) {
ADD_FAILURE() << "Already created interceptor.";
return false;
}
......@@ -335,6 +344,27 @@ class ContentVerifierHashTest
// Mock serving |copied_verified_contents_path| for content hash, so that
// hash fetch succeeds.
if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
url_loader_interceptor_ =
std::make_unique<content::URLLoaderInterceptor>(base::BindRepeating(
[](GURL fetch_url, base::FilePath file_path,
content::URLLoaderInterceptor::RequestParams* params) {
GURL url = params->url_request.url;
if (url == fetch_url) {
base::ScopedAllowBlockingForTesting allow_io;
std::string contents;
CHECK(base::ReadFileToString(file_path, &contents));
content::URLLoaderInterceptor::WriteResponse(
std::string(), contents, params->client.get());
return true;
}
return false;
},
fetch_url, copied_verified_contents_path));
return true;
}
interceptor_ = std::make_unique<net::TestURLRequestInterceptor>(
fetch_url.scheme(), fetch_url.host(),
content::BrowserThread::GetTaskRunnerForThread(
......@@ -378,6 +408,7 @@ class ContentVerifierHashTest
}
std::unique_ptr<net::TestURLRequestInterceptor> interceptor_;
std::unique_ptr<content::URLLoaderInterceptor> url_loader_interceptor_;
base::ScopedTempDir temp_dir_;
// Information about the loaded extension.
......
......@@ -642,6 +642,7 @@ source_set("unit_tests") {
"//services/data_decoder:lib",
"//services/data_decoder/public/cpp:test_support",
"//services/device/public/mojom",
"//services/network:test_support",
"//services/service_manager/public/cpp/test:test_support",
"//storage/browser:test_support",
"//third_party/leveldatabase",
......
......@@ -27,6 +27,7 @@ include_rules = [
"+services/data_decoder/public",
"+services/network/public/cpp",
"+services/network/public/mojom",
"+services/network",
"+services/preferences/public/cpp",
"+services/service_manager/public/cpp",
"+skia/ext/image_operations.h",
......
......@@ -25,8 +25,8 @@
#include "extensions/common/file_util.h"
#include "net/base/load_flags.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
#include "net/url_request/url_fetcher.h"
#include "net/url_request/url_request_status.h"
#include "services/network/public/cpp/resource_request.h"
#include "services/network/public/cpp/simple_url_loader.h"
namespace extensions {
......@@ -39,23 +39,17 @@ ContentHashFetcher::ContentHashFetcher(
fetch_params_(fetch_params),
response_task_runner_(base::SequencedTaskRunnerHandle::Get()) {}
void ContentHashFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
void ContentHashFetcher::OnSimpleLoaderComplete(
std::unique_ptr<std::string> response_body) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
VLOG(1) << "URLFetchComplete for " << extension_key_.extension_id
<< " is_success:" << url_fetcher_->GetStatus().is_success() << " "
<< " is_success:" << !!response_body << " "
<< fetch_params_.fetch_url.possibly_invalid_spec();
DCHECK(hash_fetcher_callback_);
std::unique_ptr<net::URLFetcher> url_fetcher = std::move(url_fetcher_);
std::unique_ptr<std::string> response(new std::string);
if (!url_fetcher->GetStatus().is_success() ||
!url_fetcher->GetResponseAsString(response.get())) {
// Failed.
response = nullptr;
}
response_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(std::move(hash_fetcher_callback_), extension_key_,
fetch_params_, std::move(response)));
fetch_params_, std::move(response_body)));
delete this;
}
......@@ -88,14 +82,21 @@ void ContentHashFetcher::Start(HashFetcherCallback hash_fetcher_callback) {
"the Web Store, this feature is required to ensure the "
"extensions match what is distributed by the store."
})");
url_fetcher_ = net::URLFetcher::Create(
fetch_params_.fetch_url, net::URLFetcher::GET, this, traffic_annotation);
url_fetcher_->SetRequestContext(fetch_params_.request_context);
url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
net::LOAD_DO_NOT_SAVE_COOKIES |
net::LOAD_DISABLE_CACHE);
url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(3);
url_fetcher_->Start();
auto resource_request = std::make_unique<network::ResourceRequest>();
resource_request->url = fetch_params_.fetch_url;
resource_request->load_flags = net::LOAD_DO_NOT_SEND_COOKIES |
net::LOAD_DO_NOT_SAVE_COOKIES |
net::LOAD_DISABLE_CACHE;
simple_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
traffic_annotation);
const int kMaxRetries = 3;
simple_loader_->SetRetryOptions(
kMaxRetries,
network::SimpleURLLoader::RetryMode::RETRY_ON_NETWORK_CHANGE);
simple_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
fetch_params_.url_loader_factory,
base::BindOnce(&ContentHashFetcher::OnSimpleLoaderComplete,
base::Unretained(this)));
}
ContentHashFetcher::~ContentHashFetcher() {
......
......@@ -16,15 +16,14 @@
#include "base/memory/weak_ptr.h"
#include "extensions/browser/content_verifier/content_hash.h"
#include "extensions/common/extension_id.h"
#include "net/url_request/url_fetcher_delegate.h"
namespace base {
class SequencedTaskRunner;
}
namespace net {
class URLFetcher;
}
namespace network {
class SimpleURLLoader;
} // namespace network
namespace extensions {
namespace internals {
......@@ -36,13 +35,13 @@ namespace internals {
// have the contents of verified_contents.json files from the webstore.
//
// Note: This class manages its own lifetime. It deletes itself when
// Start() completes at OnURLFetchComplete().
// Start() completes at OnSimpleLoaderComplete().
//
// Note: This class is an internal implementation detail of ContentHash and is
// not be used independently.
// TODO(lazyboy): Consider changing BUILD rules to enforce the above, yet
// keeping the class unit testable.
class ContentHashFetcher : public net::URLFetcherDelegate {
class ContentHashFetcher {
public:
// A callback for when fetch is complete.
// The response contents is passed through std::unique_ptr<std::string>.
......@@ -54,16 +53,15 @@ class ContentHashFetcher : public net::URLFetcherDelegate {
ContentHashFetcher(const ContentHash::ExtensionKey& extension_key,
const ContentHash::FetchParams& fetch_params);
// net::URLFetcherDelegate:
void OnURLFetchComplete(const net::URLFetcher* source) override;
// Note: |this| is deleted once OnURLFetchComplete() completes.
// Note: |this| is deleted once OnSimpleLoaderComplete() completes.
void Start(HashFetcherCallback hash_fetcher_callback);
private:
friend class base::RefCounted<ContentHashFetcher>;
~ContentHashFetcher() override;
~ContentHashFetcher();
void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body);
ContentHash::ExtensionKey extension_key_;
ContentHash::FetchParams fetch_params_;
......@@ -73,7 +71,7 @@ class ContentHashFetcher : public net::URLFetcherDelegate {
scoped_refptr<base::SequencedTaskRunner> response_task_runner_;
// Alive when url fetch is ongoing.
std::unique_ptr<net::URLFetcher> url_fetcher_;
std::unique_ptr<network::SimpleURLLoader> simple_loader_;
SEQUENCE_CHECKER(sequence_checker_);
......
......@@ -23,9 +23,11 @@
#include "extensions/common/constants.h"
#include "extensions/common/extension_paths.h"
#include "extensions/common/file_util.h"
#include "net/url_request/test_url_request_interceptor.h"
#include "net/url_request/url_request_interceptor.h"
#include "net/url_request/url_request_test_util.h"
#include "net/http/http_status_code.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
#include "services/network/public/mojom/url_loader_factory.mojom.h"
#include "services/network/test/test_url_loader_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/zlib/google/zip.h"
......@@ -95,13 +97,12 @@ class ContentHashWaiter {
class ContentHashFetcherTest : public ExtensionsTest {
public:
ContentHashFetcherTest()
// We need a real IO thread to be able to intercept the network request
// We need a real IO thread to be able to entercept the network request
// for the missing verified_contents.json file.
: ExtensionsTest(content::TestBrowserThreadBundle::REAL_IO_THREAD) {
request_context_ = new net::TestURLRequestContextGetter(
content::BrowserThread::GetTaskRunnerForThread(
content::BrowserThread::IO));
}
: ExtensionsTest(content::TestBrowserThreadBundle::REAL_IO_THREAD),
test_shared_loader_factory_(
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
&test_url_loader_factory_)) {}
~ContentHashFetcherTest() override {}
bool LoadTestExtension() {
......@@ -137,7 +138,8 @@ class ContentHashFetcherTest : public ExtensionsTest {
ContentHash::ExtensionKey(extension_->id(), extension_->path(),
extension_->version(),
delegate_->GetPublicKey()),
ContentHash::FetchParams(request_context(), fetch_url_));
ContentHash::FetchParams(test_shared_loader_factory_.get(),
fetch_url_));
delegate_.reset();
......@@ -162,20 +164,20 @@ class ContentHashFetcherTest : public ExtensionsTest {
void RegisterInterception(const GURL& url,
const base::FilePath& response_path) {
ASSERT_TRUE(base::PathExists(response_path));
ASSERT_FALSE(interceptor_);
interceptor_ = std::make_unique<net::TestURLRequestInterceptor>(
url.scheme(), url.host(),
content::BrowserThread::GetTaskRunnerForThread(
content::BrowserThread::IO),
GetExtensionFileTaskRunner());
interceptor_->SetResponse(url, response_path);
std::string data;
EXPECT_TRUE(ReadFileToString(response_path, &data));
constexpr size_t kMaxFileSize = 1024 * 2; // Using 2k file size for safety.
ASSERT_LE(data.length(), kMaxFileSize);
test_url_loader_factory_.AddResponse(url.spec(), data);
}
private:
net::URLRequestContextGetter* request_context() {
return request_context_.get();
void RegisterInterceptionWithFailure(const GURL& url, int net_error) {
test_url_loader_factory_.AddResponse(
GURL(url), network::ResourceResponseHead(), std::string(),
network::URLLoaderCompletionStatus(net_error));
}
private:
// Helper to get files from our subdirectory in the general extensions test
// data dir.
base::FilePath GetTestPath(const base::FilePath& relative_path) {
......@@ -200,8 +202,9 @@ class ContentHashFetcherTest : public ExtensionsTest {
return extension;
}
std::unique_ptr<net::TestURLRequestInterceptor> interceptor_;
scoped_refptr<net::TestURLRequestContextGetter> request_context_;
scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
network::TestURLLoaderFactory test_url_loader_factory_;
base::ScopedTempDir temp_dir_;
GURL fetch_url_;
......@@ -262,7 +265,7 @@ TEST_F(ContentHashFetcherTest, FetchInvalidVerifiedContents) {
TEST_F(ContentHashFetcherTest, Fetch404VerifiedContents) {
ASSERT_TRUE(LoadTestExtension());
// NOTE: No RegisterInterception(), hash fetch will result in 404.
RegisterInterceptionWithFailure(fetch_url(), net::HTTP_NOT_FOUND);
// Make sure the fetch was *not* successful.
std::unique_ptr<ContentHashFetcherResult> result = DoHashFetch();
......
......@@ -363,11 +363,12 @@ ContentVerifier::ContentVerifier(
std::unique_ptr<ContentVerifierDelegate> delegate)
: context_(context),
delegate_(std::move(delegate)),
request_context_getter_(
content::BrowserContext::GetDefaultStoragePartition(context)
->GetURLRequestContext()),
observer_(this),
io_data_(new ContentVerifierIOData) {}
io_data_(new ContentVerifierIOData) {
shared_url_loader_factory_info_ =
content::BrowserContext::GetDefaultStoragePartition(context_)
->GetURLLoaderFactoryForBrowserProcessIOThread();
}
ContentVerifier::~ContentVerifier() {
}
......@@ -391,6 +392,10 @@ void ContentVerifier::ShutdownOnIO() {
shutdown_on_io_ = true;
io_data_->Clear();
hash_helper_.reset();
shared_url_loader_factory_info_.reset();
scoped_refptr<network::SharedURLLoaderFactory> deleter =
std::move(shared_url_loader_factory_);
}
ContentVerifyJob* ContentVerifier::CreateJobFor(
......@@ -578,8 +583,13 @@ void ContentVerifier::OnFetchComplete(
ContentHash::FetchParams ContentVerifier::GetFetchParams(
const ExtensionId& extension_id,
const base::Version& extension_version) {
if (!shared_url_loader_factory_) {
DCHECK(shared_url_loader_factory_info_);
shared_url_loader_factory_ = network::SharedURLLoaderFactory::Create(
std::move(shared_url_loader_factory_info_));
}
return ContentHash::FetchParams(
request_context_getter_,
shared_url_loader_factory_.get(),
delegate_->GetSignatureFetchUrl(extension_id, extension_version));
}
......
......@@ -19,6 +19,7 @@
#include "extensions/browser/content_verifier_io_data.h"
#include "extensions/browser/content_verify_job.h"
#include "extensions/browser/extension_registry_observer.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
namespace base {
class FilePath;
......@@ -28,10 +29,6 @@ namespace content {
class BrowserContext;
}
namespace net {
class URLRequestContextGetter;
}
namespace extensions {
class Extension;
......@@ -164,7 +161,9 @@ class ContentVerifier : public base::RefCountedThreadSafe<ContentVerifier>,
std::unique_ptr<ContentVerifierDelegate> delegate_;
// Used by ContentHashFetcher.
net::URLRequestContextGetter* request_context_getter_;
std::unique_ptr<network::SharedURLLoaderFactoryInfo>
shared_url_loader_factory_info_;
scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
// For observing the ExtensionRegistry.
ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver> observer_;
......
......@@ -20,6 +20,7 @@
#include "extensions/common/file_util.h"
#include "net/base/load_flags.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
namespace extensions {
......@@ -79,10 +80,11 @@ ContentHash::ExtensionKey& ContentHash::ExtensionKey::operator=(
const ContentHash::ExtensionKey& other) = default;
ContentHash::FetchParams::FetchParams(
net::URLRequestContextGetter* request_context,
network::mojom::URLLoaderFactory* url_loader_factory,
const GURL& fetch_url)
: request_context(request_context), fetch_url(fetch_url) {}
: url_loader_factory(url_loader_factory), fetch_url(fetch_url) {}
ContentHash::FetchParams::~FetchParams() = default;
ContentHash::FetchParams::FetchParams(const FetchParams& other) = default;
ContentHash::FetchParams& ContentHash::FetchParams::operator=(
const FetchParams& other) = default;
......@@ -102,8 +104,11 @@ void ContentHash::Create(const ExtensionKey& key,
if (!verified_contents) {
// Fetch verified_contents.json and then respond.
FetchVerifiedContents(key, fetch_params, is_cancelled,
std::move(created_callback));
content::BrowserThread::PostTask(
content::BrowserThread::IO, FROM_HERE,
base::BindOnce(&ContentHash::FetchVerifiedContentsOnIOThread, key,
fetch_params, is_cancelled,
std::move(created_callback)));
return;
}
......@@ -152,16 +157,34 @@ ContentHash::ContentHash(
ContentHash::~ContentHash() = default;
// static
void ContentHash::FetchVerifiedContents(
void ContentHash::FetchVerifiedContentsOnIOThread(
const ContentHash::ExtensionKey& extension_key,
const ContentHash::FetchParams& fetch_params,
const ContentHash::IsCancelledCallback& is_cancelled,
ContentHash::CreatedCallback created_callback) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
// |fetcher| deletes itself when it's done.
internals::ContentHashFetcher* fetcher =
new internals::ContentHashFetcher(extension_key, fetch_params);
fetcher->Start(base::BindOnce(&ContentHash::DidFetchVerifiedContents,
std::move(created_callback), is_cancelled));
fetcher->Start(
base::BindOnce(&ContentHash::DidFetchVerifiedContentsOnIOThread,
std::move(created_callback), is_cancelled));
}
// static
void ContentHash::DidFetchVerifiedContentsOnIOThread(
ContentHash::CreatedCallback created_callback,
const ContentHash::IsCancelledCallback& is_cancelled,
const ContentHash::ExtensionKey& key,
const ContentHash::FetchParams& fetch_params,
std::unique_ptr<std::string> fetched_contents) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
GetExtensionFileTaskRunner()->PostTask(
FROM_HERE, base::BindOnce(&ContentHash::DidFetchVerifiedContents,
std::move(created_callback), is_cancelled, key,
fetch_params, std::move(fetched_contents)));
return;
}
// static
......@@ -171,6 +194,7 @@ void ContentHash::DidFetchVerifiedContents(
const ContentHash::ExtensionKey& key,
const ContentHash::FetchParams& fetch_params,
std::unique_ptr<std::string> fetched_contents) {
DCHECK(GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence());
base::AssertBlockingAllowed();
if (!fetched_contents) {
ContentHash::DispatchFetchFailure(key, std::move(created_callback),
......
......@@ -17,9 +17,11 @@
#include "extensions/common/extension_id.h"
#include "url/gurl.h"
namespace net {
class URLRequestContextGetter;
namespace network {
namespace mojom {
class URLLoaderFactory;
}
} // namespace network
namespace extensions {
......@@ -71,11 +73,12 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
// Parameters to fetch verified_contents.json.
struct FetchParams {
net::URLRequestContextGetter* request_context;
network::mojom::URLLoaderFactory* url_loader_factory;
GURL fetch_url;
FetchParams(net::URLRequestContextGetter* request_context,
FetchParams(network::mojom::URLLoaderFactory* url_loader_factory,
const GURL& fetch_url);
~FetchParams();
FetchParams(const FetchParams& other);
FetchParams& operator=(const FetchParams& other);
......@@ -145,10 +148,17 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
std::unique_ptr<ComputedHashes::Reader> computed_hashes);
~ContentHash();
static void FetchVerifiedContents(const ExtensionKey& extension_key,
const FetchParams& fetch_params,
const IsCancelledCallback& is_cancelled,
CreatedCallback created_callback);
static void FetchVerifiedContentsOnIOThread(
const ExtensionKey& extension_key,
const FetchParams& fetch_params,
const IsCancelledCallback& is_cancelled,
CreatedCallback created_callback);
static void DidFetchVerifiedContentsOnIOThread(
CreatedCallback created_callback,
const IsCancelledCallback& is_cancelled,
const ExtensionKey& key,
const FetchParams& fetch_params,
std::unique_ptr<std::string> fetched_contents);
static void DidFetchVerifiedContents(
CreatedCallback created_callback,
const IsCancelledCallback& is_cancelled,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment