Commit cc22e605 authored by Reilly Grant's avatar Reilly Grant Committed by Commit Bot

Move signin::ProxyingURLLoaderFactory ownership to ResourceContext

This takes a hint from r588692 and moves ownership of the
ProxyingURLLoaderFactory objects a user data object that hangs off of
the content::ResourceContext. This prevents these objects from outliving
this object on the IO thread. The previous ownership model was open to a
race condition between the destruction of the BrowserContext keyed
service ProxyingURLLoaderFactoryManager on the UI thread and the tasks
posted to the IO thread to destroy each ProxyingURLLoaderFactory. These
would be scheduled after the task to destroy the ResourceContext.

Change-Id: I600b1604526b644c3ac9e010d1cb82f4b3740925
Reviewed-on: https://chromium-review.googlesource.com/1211913Reviewed-by: default avatarClark DuVall <cduvall@chromium.org>
Reviewed-by: default avatarDavid Roger <droger@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589522}
parent 54870a1e
......@@ -1396,8 +1396,6 @@ jumbo_split_static_library("browser") {
"signin/chrome_signin_helper.h",
"signin/chrome_signin_proxying_url_loader_factory.cc",
"signin/chrome_signin_proxying_url_loader_factory.h",
"signin/chrome_signin_proxying_url_loader_factory_manager.cc",
"signin/chrome_signin_proxying_url_loader_factory_manager.h",
"signin/chrome_signin_url_loader_throttle.cc",
"signin/chrome_signin_url_loader_throttle.h",
"signin/gaia_cookie_manager_service_factory.cc",
......
......@@ -100,7 +100,7 @@
#include "chrome/browser/safe_browsing/url_checker_delegate_impl.h"
#include "chrome/browser/search/search.h"
#include "chrome/browser/sessions/session_tab_helper.h"
#include "chrome/browser/signin/chrome_signin_proxying_url_loader_factory_manager.h"
#include "chrome/browser/signin/chrome_signin_proxying_url_loader_factory.h"
#include "chrome/browser/signin/chrome_signin_url_loader_throttle.h"
#include "chrome/browser/signin/header_modification_delegate_impl.h"
#include "chrome/browser/signin/signin_manager_factory.h"
......@@ -4515,13 +4515,8 @@ bool ChromeContentBrowserClient::WillCreateURLLoaderFactory(
}
#endif
auto* signin_proxy_manager =
signin::ProxyingURLLoaderFactoryManagerFactory::GetForProfile(
Profile::FromBrowserContext(browser_context));
if (signin_proxy_manager) {
use_proxy |= signin_proxy_manager->MaybeProxyURLLoaderFactory(
frame, is_navigation, url, factory_request);
}
use_proxy |= signin::ProxyingURLLoaderFactory::MaybeProxyRequest(
frame, is_navigation, url, factory_request);
return use_proxy;
}
......
......@@ -5,17 +5,80 @@
#include "chrome/browser/signin/chrome_signin_proxying_url_loader_factory.h"
#include "base/barrier_closure.h"
#include "build/buildflag.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/chrome_signin_helper.h"
#include "chrome/browser/signin/header_modification_delegate.h"
#include "chrome/browser/signin/header_modification_delegate_impl.h"
#include "components/signin/core/browser/signin_header_helper.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/resource_context.h"
#include "content/public/browser/web_contents.h"
#include "extensions/browser/guest_view/web_view/web_view_renderer_state.h"
#include "extensions/buildflags/buildflags.h"
#include "google_apis/gaia/gaia_auth_util.h"
#include "net/base/net_errors.h"
using content::BrowserThread;
namespace signin {
namespace {
// User data key for ResourceContextData.
const void* const kResourceContextUserDataKey = &kResourceContextUserDataKey;
// Owns all of the ProxyingURLLoaderFactorys for a given Profile. Since these
// live on the IO thread this is done indirectly through the
// content::ResourceContext.
class ResourceContextData : public base::SupportsUserData::Data {
public:
~ResourceContextData() override {}
static void StartProxying(
content::ResourceContext* resource_context,
content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
network::mojom::URLLoaderFactoryRequest request,
network::mojom::URLLoaderFactoryPtrInfo target_factory) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
auto* self = static_cast<ResourceContextData*>(
resource_context->GetUserData(kResourceContextUserDataKey));
if (!self) {
self = new ResourceContextData();
resource_context->SetUserData(kResourceContextUserDataKey,
base::WrapUnique(self));
}
auto delegate =
std::make_unique<HeaderModificationDelegateImpl>(resource_context);
auto proxy = std::make_unique<ProxyingURLLoaderFactory>(
std::move(delegate), std::move(web_contents_getter), std::move(request),
std::move(target_factory),
base::BindOnce(&ResourceContextData::RemoveProxy,
self->weak_factory_.GetWeakPtr()));
self->proxies_.emplace(std::move(proxy));
}
void RemoveProxy(ProxyingURLLoaderFactory* proxy) {
auto it = proxies_.find(proxy);
DCHECK(it != proxies_.end());
proxies_.erase(it);
}
private:
ResourceContextData() : weak_factory_(this) {}
std::set<std::unique_ptr<ProxyingURLLoaderFactory>, base::UniquePtrComparator>
proxies_;
base::WeakPtrFactory<ResourceContextData> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(ResourceContextData);
};
} // namespace
class ProxyingURLLoaderFactory::InProgressRequest
: public network::mojom::URLLoader,
public network::mojom::URLLoaderClient {
......@@ -346,18 +409,12 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveRedirect(
response_url_ = redirect_info.new_url;
}
ProxyingURLLoaderFactory::ProxyingURLLoaderFactory() = default;
ProxyingURLLoaderFactory::~ProxyingURLLoaderFactory() = default;
// static
void ProxyingURLLoaderFactory::StartProxying(
ProxyingURLLoaderFactory::ProxyingURLLoaderFactory(
std::unique_ptr<HeaderModificationDelegate> delegate,
content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
network::mojom::URLLoaderFactoryRequest loader_request,
network::mojom::URLLoaderFactoryPtrInfo target_factory,
base::OnceClosure on_disconnect) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
DisconnectCallback on_disconnect) {
DCHECK(proxy_bindings_.empty());
DCHECK(!target_factory_.is_bound());
DCHECK(!delegate_);
......@@ -377,6 +434,61 @@ void ProxyingURLLoaderFactory::StartProxying(
&ProxyingURLLoaderFactory::OnProxyBindingError, base::Unretained(this)));
}
ProxyingURLLoaderFactory::~ProxyingURLLoaderFactory() = default;
// static
bool ProxyingURLLoaderFactory::MaybeProxyRequest(
content::RenderFrameHost* render_frame_host,
bool is_navigation,
const GURL& url,
network::mojom::URLLoaderFactoryRequest* factory_request) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
// Navigation requests are handled using signin::URLLoaderThrottle.
if (is_navigation)
return false;
// This proxy should only be installed for subresource requests from a frame
// that is rendering the GAIA signon realm.
if (!render_frame_host || !gaia::IsGaiaSignonRealm(url.GetOrigin())) {
return false;
}
auto* web_contents =
content::WebContents::FromRenderFrameHost(render_frame_host);
auto* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
if (profile->IsOffTheRecord())
return false;
#if BUILDFLAG(ENABLE_EXTENSIONS)
// Account consistency requires the AccountReconcilor, which is only
// attached to the main request context.
// Note: InlineLoginUI uses an isolated request context and thus bypasses
// the account consistency flow here. See http://crbug.com/428396
if (extensions::WebViewRendererState::GetInstance()->IsGuest(
render_frame_host->GetProcess()->GetID())) {
return false;
}
#endif
auto proxied_request = std::move(*factory_request);
network::mojom::URLLoaderFactoryPtrInfo target_factory_info;
*factory_request = mojo::MakeRequest(&target_factory_info);
auto web_contents_getter =
base::BindRepeating(&content::WebContents::FromFrameTreeNodeId,
render_frame_host->GetFrameTreeNodeId());
content::BrowserThread::PostTask(
content::BrowserThread::IO, FROM_HERE,
base::BindOnce(&ResourceContextData::StartProxying,
profile->GetResourceContext(),
std::move(web_contents_getter), std::move(proxied_request),
std::move(target_factory_info)));
return true;
}
void ProxyingURLLoaderFactory::CreateLoaderAndStart(
network::mojom::URLLoaderRequest loader_request,
int32_t routing_id,
......@@ -392,13 +504,10 @@ void ProxyingURLLoaderFactory::CreateLoaderAndStart(
void ProxyingURLLoaderFactory::Clone(
network::mojom::URLLoaderFactoryRequest loader_request) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
proxy_bindings_.AddBinding(this, std::move(loader_request));
}
void ProxyingURLLoaderFactory::OnTargetFactoryError() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
// Stop calls to CreateLoaderAndStart() when |target_factory_| is invalid.
target_factory_.reset();
proxy_bindings_.CloseAllBindings();
......@@ -407,8 +516,6 @@ void ProxyingURLLoaderFactory::OnTargetFactoryError() {
}
void ProxyingURLLoaderFactory::OnProxyBindingError() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (proxy_bindings_.empty())
target_factory_.reset();
......@@ -424,16 +531,13 @@ void ProxyingURLLoaderFactory::RemoveRequest(InProgressRequest* request) {
}
void ProxyingURLLoaderFactory::MaybeDestroySelf() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
// Even if all URLLoaderFactory pipes connected to this object have been
// closed it has to stay alive until all active requests have completed.
if (target_factory_.is_bound() || !requests_.empty())
return;
// Deletes |this| after a roundtrip to the UI thread.
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
std::move(on_disconnect_));
// Deletes |this|.
std::move(on_disconnect_).Run(this);
}
} // namespace signin
......@@ -16,6 +16,10 @@
#include <memory>
#include <set>
namespace content {
class RenderFrameHost;
}
namespace signin {
class HeaderModificationDelegate;
......@@ -26,15 +30,28 @@ class HeaderModificationDelegate;
// Network Service to modify request and response headers.
class ProxyingURLLoaderFactory : public network::mojom::URLLoaderFactory {
public:
ProxyingURLLoaderFactory();
~ProxyingURLLoaderFactory() override;
using DisconnectCallback =
base::OnceCallback<void(ProxyingURLLoaderFactory*)>;
void StartProxying(
// Constructor public for testing purposes. New instances should be created
// by calling MaybeProxyRequest().
ProxyingURLLoaderFactory(
std::unique_ptr<HeaderModificationDelegate> delegate,
content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
network::mojom::URLLoaderFactoryRequest request,
network::mojom::URLLoaderFactoryPtrInfo target_factory,
base::OnceClosure on_disconnect);
DisconnectCallback on_disconnect);
~ProxyingURLLoaderFactory() override;
// Called when a renderer needs a URLLoaderFactory to give this module the
// opportunity to install a proxy. This is only done when
// https://accounts.google.com is loaded in non-incognito mode. Returns true
// when |factory_request| has been proxied.
static bool MaybeProxyRequest(
content::RenderFrameHost* render_frame_host,
bool is_navigation,
const GURL& url,
network::mojom::URLLoaderFactoryRequest* factory_request);
// network::mojom::URLLoaderFactory:
void CreateLoaderAndStart(network::mojom::URLLoaderRequest loader_request,
......@@ -71,7 +88,7 @@ class ProxyingURLLoaderFactory : public network::mojom::URLLoaderFactory {
std::set<std::unique_ptr<InProgressRequest>, base::UniquePtrComparator>
requests_;
network::mojom::URLLoaderFactoryPtr target_factory_;
base::OnceClosure on_disconnect_;
DisconnectCallback on_disconnect_;
DISALLOW_COPY_AND_ASSIGN(ProxyingURLLoaderFactory);
};
......
// Copyright 2018 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 "chrome/browser/signin/chrome_signin_proxying_url_loader_factory_manager.h"
#include "base/no_destructor.h"
#include "build/buildflag.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/chrome_signin_proxying_url_loader_factory.h"
#include "chrome/browser/signin/header_modification_delegate_impl.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/web_contents.h"
#include "extensions/browser/guest_view/web_view/web_view_renderer_state.h"
#include "extensions/buildflags/buildflags.h"
#include "google_apis/gaia/gaia_auth_util.h"
using content::BrowserThread;
namespace signin {
// static
ProxyingURLLoaderFactoryManager*
ProxyingURLLoaderFactoryManagerFactory::GetForProfile(Profile* profile) {
return static_cast<ProxyingURLLoaderFactoryManager*>(
GetInstance().GetServiceForBrowserContext(profile, true));
}
// static
ProxyingURLLoaderFactoryManagerFactory&
ProxyingURLLoaderFactoryManagerFactory::GetInstance() {
static base::NoDestructor<ProxyingURLLoaderFactoryManagerFactory> instance;
return *instance;
}
ProxyingURLLoaderFactoryManagerFactory::ProxyingURLLoaderFactoryManagerFactory()
: BrowserContextKeyedServiceFactory(
"ChromeSigninProxyingURLLoaderFactoryManagerFactory",
BrowserContextDependencyManager::GetInstance()) {}
ProxyingURLLoaderFactoryManagerFactory::
~ProxyingURLLoaderFactoryManagerFactory() {
// The only instance of this class should be owned by base::NoDestructor.
NOTREACHED();
}
KeyedService* ProxyingURLLoaderFactoryManagerFactory::BuildServiceInstanceFor(
content::BrowserContext* browser_context) const {
Profile* profile = static_cast<Profile*>(browser_context);
return new ProxyingURLLoaderFactoryManager(profile);
}
ProxyingURLLoaderFactoryManager::ProxyingURLLoaderFactoryManager(
Profile* profile)
: profile_(profile), weak_factory_(this) {}
ProxyingURLLoaderFactoryManager::~ProxyingURLLoaderFactoryManager() = default;
bool ProxyingURLLoaderFactoryManager::MaybeProxyURLLoaderFactory(
content::RenderFrameHost* render_frame_host,
bool is_navigation,
const GURL& url,
network::mojom::URLLoaderFactoryRequest* factory_request) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(!profile_->IsOffTheRecord());
// Navigation requests are handled using signin::URLLoaderThrottle.
if (is_navigation)
return false;
// This proxy should only be installed for subresource requests from a frame
// that is rendering the GAIA signon realm.
if (!render_frame_host || !gaia::IsGaiaSignonRealm(url.GetOrigin())) {
return false;
}
#if BUILDFLAG(ENABLE_EXTENSIONS)
// Account consistency requires the AccountReconcilor, which is only
// attached to the main request context.
// Note: InlineLoginUI uses an isolated request context and thus bypasses
// the account consistency flow here. See http://crbug.com/428396
if (extensions::WebViewRendererState::GetInstance()->IsGuest(
render_frame_host->GetProcess()->GetID())) {
return false;
}
#endif
auto proxied_request = std::move(*factory_request);
network::mojom::URLLoaderFactoryPtrInfo target_factory_info;
*factory_request = mojo::MakeRequest(&target_factory_info);
auto web_contents_getter =
base::BindRepeating(&content::WebContents::FromFrameTreeNodeId,
render_frame_host->GetFrameTreeNodeId());
std::unique_ptr<ProxyingURLLoaderFactory, BrowserThread::DeleteOnIOThread>
proxy(new ProxyingURLLoaderFactory());
auto* proxy_weak = proxy.get();
auto delegate = std::make_unique<signin::HeaderModificationDelegateImpl>(
profile_->GetResourceContext());
proxies_.emplace(std::move(proxy));
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::BindOnce(
&ProxyingURLLoaderFactory::StartProxying,
base::Unretained(proxy_weak), std::move(delegate),
std::move(web_contents_getter), std::move(proxied_request),
std::move(target_factory_info),
base::BindOnce(&ProxyingURLLoaderFactoryManager::RemoveProxy,
weak_factory_.GetWeakPtr(), proxy_weak)));
return true;
}
void ProxyingURLLoaderFactoryManager::RemoveProxy(
ProxyingURLLoaderFactory* proxy) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
auto it = proxies_.find(proxy);
DCHECK(it != proxies_.end());
proxies_.erase(it);
}
} // namespace signin
// Copyright 2018 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.
#ifndef CHROME_BROWSER_SIGNIN_CHROME_SIGNIN_PROXYING_URL_LOADER_FACTORY_MANAGER_H_
#define CHROME_BROWSER_SIGNIN_CHROME_SIGNIN_PROXYING_URL_LOADER_FACTORY_MANAGER_H_
#include "base/containers/unique_ptr_adapters.h"
#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
#include "components/keyed_service/core/keyed_service.h"
#include "content/public/browser/browser_thread.h"
#include "services/network/public/mojom/url_loader_factory.mojom.h"
class Profile;
namespace content {
class BrowserContext;
class RenderFrameHost;
} // namespace content
namespace signin {
class ProxyingURLLoaderFactory;
class ProxyingURLLoaderFactoryManager;
class ProxyingURLLoaderFactoryManagerFactory
: public BrowserContextKeyedServiceFactory {
public:
ProxyingURLLoaderFactoryManagerFactory();
~ProxyingURLLoaderFactoryManagerFactory() override;
// Returns the instance of ProxyingURLLoaderFactoryManager associated with
// this profile (creating one if none exists). Returns nullptr if this profile
// cannot have a ProxyingURLLoaderFactoryManager (for example, if it is
// incognito).
static ProxyingURLLoaderFactoryManager* GetForProfile(Profile* profile);
// Returns the instance of this factory singleton.
static ProxyingURLLoaderFactoryManagerFactory& GetInstance();
private:
// BrowserContextKeyedServiceFactory:
KeyedService* BuildServiceInstanceFor(
content::BrowserContext* browser_context) const override;
DISALLOW_COPY_AND_ASSIGN(ProxyingURLLoaderFactoryManagerFactory);
};
// This class owns all of the ProxyingURLLoaderFactory instances associated
// with a particular Profile. This object lives on the UI thread while the
// objects it manages live on the IO thread.
class ProxyingURLLoaderFactoryManager : public KeyedService {
public:
explicit ProxyingURLLoaderFactoryManager(Profile* profile);
~ProxyingURLLoaderFactoryManager() override;
// Called when a renderer needs a URLLoaderFactory to give this module the
// opportunity to install a proxy. This is only done when
// https://accounts.google.com is loaded in non-incognito mode. Returns true
// when |factory_request| has been proxied.
bool MaybeProxyURLLoaderFactory(
content::RenderFrameHost* render_frame_host,
bool is_navigation,
const GURL& url,
network::mojom::URLLoaderFactoryRequest* factory_request);
private:
void RemoveProxy(ProxyingURLLoaderFactory* proxy);
Profile* const profile_;
std::set<std::unique_ptr<ProxyingURLLoaderFactory,
content::BrowserThread::DeleteOnIOThread>,
base::UniquePtrComparator>
proxies_;
base::WeakPtrFactory<ProxyingURLLoaderFactoryManager> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(ProxyingURLLoaderFactoryManager);
};
} // namespace signin
#endif // CHROME_BROWSER_SIGNIN_CHROME_SIGNIN_PROXYING_URL_LOADER_FACTORY_MANAGER_H_
......@@ -81,8 +81,7 @@ class ChromeSigninProxyingURLLoaderFactoryTest : public testing::Test {
auto delegate = std::make_unique<MockDelegate>();
base::WeakPtr<MockDelegate> delegate_weak = delegate->GetWeakPtr();
proxying_factory_ = std::make_unique<ProxyingURLLoaderFactory>();
proxying_factory_->StartProxying(
proxying_factory_ = std::make_unique<ProxyingURLLoaderFactory>(
std::move(delegate), NullWebContentsGetter(),
std::move(factory_request), std::move(test_factory_ptr_info),
base::BindOnce(&ChromeSigninProxyingURLLoaderFactoryTest::OnDisconnect,
......@@ -102,7 +101,10 @@ class ChromeSigninProxyingURLLoaderFactoryTest : public testing::Test {
}
private:
void OnDisconnect() { proxying_factory_.reset(); }
void OnDisconnect(ProxyingURLLoaderFactory* factory) {
EXPECT_EQ(factory, proxying_factory_.get());
proxying_factory_.reset();
}
content::TestBrowserThreadBundle thread_bundle_;
std::unique_ptr<network::SimpleURLLoader> loader_;
......@@ -278,8 +280,7 @@ TEST_F(ChromeSigninProxyingURLLoaderFactoryTest, TargetFactoryFailure) {
auto delegate = std::make_unique<MockDelegate>();
EXPECT_CALL(*delegate, ProcessRequest(_, _)).Times(0);
auto proxying_factory = std::make_unique<ProxyingURLLoaderFactory>();
proxying_factory->StartProxying(
auto proxying_factory = std::make_unique<ProxyingURLLoaderFactory>(
std::move(delegate), NullWebContentsGetter(), std::move(factory_request),
std::move(target_factory_ptr_info), base::DoNothing());
......
......@@ -7,6 +7,7 @@
#include "chrome/browser/profiles/profile_io_data.h"
#include "chrome/browser/renderer_host/chrome_navigation_ui_data.h"
#include "chrome/browser/signin/chrome_signin_helper.h"
#include "content/public/browser/browser_thread.h"
#include "extensions/browser/extension_navigation_ui_data.h"
namespace signin {
......@@ -43,12 +44,14 @@ bool HeaderModificationDelegateImpl::ShouldInterceptNavigation(
void HeaderModificationDelegateImpl::ProcessRequest(
ChromeRequestAdapter* request_adapter,
const GURL& redirect_url) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
FixAccountConsistencyRequestHeader(request_adapter, redirect_url, io_data_);
}
void HeaderModificationDelegateImpl::ProcessResponse(
ResponseAdapter* response_adapter,
const GURL& redirect_url) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
ProcessAccountConsistencyResponseHeaders(response_adapter, redirect_url,
io_data_->IsOffTheRecord());
}
......
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