Skip to content

Commit

Permalink
Various format fixes for ABP redirects
Browse files Browse the repository at this point in the history
  • Loading branch information
bbondy committed Nov 19, 2019
1 parent 58f8190 commit 7e2ccbc
Show file tree
Hide file tree
Showing 17 changed files with 195 additions and 172 deletions.
6 changes: 3 additions & 3 deletions browser/net/brave_ad_block_tp_network_delegate_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,21 @@ void ShouldBlockAdOnTaskRunner(std::shared_ptr<BraveRequestInfo> ctx) {
if (!g_brave_browser_process->ad_block_service()->ShouldStartRequest(
ctx->request_url, ctx->resource_type, tab_host,
&did_match_exception, &ctx->cancel_request_explicitly,
&ctx->redirect)) {
&ctx->mock_data_url)) {
ctx->blocked_by = kAdBlocked;
} else if (!did_match_exception &&
!g_brave_browser_process->ad_block_regional_service_manager()
->ShouldStartRequest(ctx->request_url, ctx->resource_type,
tab_host, &did_match_exception,
&ctx->cancel_request_explicitly,
&ctx->redirect)) {
&ctx->mock_data_url)) {
ctx->blocked_by = kAdBlocked;
} else if (!did_match_exception &&
!g_brave_browser_process->ad_block_custom_filters_service()
->ShouldStartRequest(ctx->request_url, ctx->resource_type,
tab_host, &did_match_exception,
&ctx->cancel_request_explicitly,
&ctx->redirect)) {
&ctx->mock_data_url)) {
ctx->blocked_by = kAdBlocked;
}
}
Expand Down
23 changes: 10 additions & 13 deletions browser/net/brave_proxying_url_loader_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::RestartInternal() {

base::RepeatingCallback<void(int)> continuation =
base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders,
weak_factory_.GetWeakPtr());
weak_factory_.GetWeakPtr());
redirect_url_ = GURL();
ctx_ = std::make_shared<brave::BraveRequestInfo>();
brave::BraveRequestInfo::FillCTX(request_, render_process_id_,
Expand Down Expand Up @@ -211,8 +211,8 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::OnUploadProgress(
std::move(callback));
}

void BraveProxyingURLLoaderFactory::InProgressRequest::
OnReceiveCachedMetadata(mojo_base::BigBuffer data) {
void BraveProxyingURLLoaderFactory::InProgressRequest::OnReceiveCachedMetadata(
mojo_base::BigBuffer data) {
target_client_->OnReceiveCachedMetadata(std::move(data));
}

Expand Down Expand Up @@ -337,8 +337,8 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::
}
network::ResourceResponseHead response;
std::string response_data;
brave_shields::MakeStubResponse(ctx_->redirect, request_, &response,
&response_data);
brave_shields::MakeStubResponse(ctx_->mock_data_url, request_, &response,
&response_data);

target_client_->OnReceiveResponse(response);

Expand Down Expand Up @@ -627,10 +627,8 @@ bool BraveProxyingURLLoaderFactory::MaybeProxyRequest(
network::mojom::URLLoaderFactoryPtrInfo target_factory_info;
*factory_receiver = mojo::MakeRequest(&target_factory_info);


ResourceContextData::StartProxying(
browser_context,
render_process_id,
browser_context, render_process_id,
render_frame_host ? render_frame_host->GetFrameTreeNodeId() : 0,
std::move(proxied_receiver), std::move(target_factory_info));
return true;
Expand All @@ -652,11 +650,10 @@ void BraveProxyingURLLoaderFactory::CreateLoaderAndStart(
// unique, so we don't use it for identity here.
const uint64_t brave_request_id = request_id_generator_->Generate();

auto result = requests_.emplace(
std::make_unique<InProgressRequest>(
this, brave_request_id, request_id, routing_id, render_process_id_,
frame_tree_node_id_, options, request, browser_context_,
traffic_annotation, std::move(loader_request), std::move(client)));
auto result = requests_.emplace(std::make_unique<InProgressRequest>(
this, brave_request_id, request_id, routing_id, render_process_id_,
frame_tree_node_id_, options, request, browser_context_,
traffic_annotation, std::move(loader_request), std::move(client)));
(*result.first)->Restart();
}

Expand Down
2 changes: 1 addition & 1 deletion browser/net/url_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ struct BraveRequestInfo {
const base::ListValue* referral_headers_list = nullptr;
BlockedBy blocked_by = kNotBlocked;
bool cancel_request_explicitly = false;
std::string redirect;
std::string mock_data_url;

// Default to invalid type for resource_type, so delegate helpers
// can properly detect that the info couldn't be obtained.
Expand Down
50 changes: 24 additions & 26 deletions components/brave_shields/browser/ad_block_base_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ namespace brave_shields {
AdBlockBaseService::AdBlockBaseService(BraveComponent::Delegate* delegate)
: BaseBraveShieldsService(delegate),
ad_block_client_(new adblock::Engine()),
weak_factory_(this) {
}
weak_factory_(this) {}

AdBlockBaseService::~AdBlockBaseService() {
Cleanup();
Expand All @@ -118,22 +117,26 @@ void AdBlockBaseService::Cleanup() {
}

bool AdBlockBaseService::ShouldStartRequest(const GURL& url,
content::ResourceType resource_type, const std::string& tab_host,
bool* did_match_exception, bool* cancel_request_explicitly,
std::string* redirect) {
content::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception,
bool* cancel_request_explicitly,
std::string* mock_data_url) {
DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence());

// Determine third-party here so the library doesn't need to figure it out.
// CreateFromNormalizedTuple is needed because SameDomainOrHost needs
// a URL or origin and not a string to a host name.
bool is_third_party = !SameDomainOrHost(url,
bool is_third_party = !SameDomainOrHost(
url,
url::Origin::CreateFromNormalizedTuple("https", tab_host.c_str(), 80),
INCLUDE_PRIVATE_REGISTRIES);
bool explicit_cancel;
bool saved_from_exception;
if (ad_block_client_->matches(url.spec(), url.host(),
tab_host, is_third_party, ResourceTypeToString(resource_type),
&explicit_cancel, &saved_from_exception, redirect)) {
if (ad_block_client_->matches(
url.spec(), url.host(), tab_host, is_third_party,
ResourceTypeToString(resource_type), &explicit_cancel,
&saved_from_exception, mock_data_url)) {
if (cancel_request_explicitly) {
*cancel_request_explicitly = explicit_cancel;
}
Expand All @@ -157,10 +160,9 @@ bool AdBlockBaseService::ShouldStartRequest(const GURL& url,

void AdBlockBaseService::EnableTag(const std::string& tag, bool enabled) {
if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
GetTaskRunner()->PostTask(FROM_HERE,
base::BindOnce(&AdBlockBaseService::EnableTag,
base::Unretained(this),
tag, enabled));
GetTaskRunner()->PostTask(
FROM_HERE, base::BindOnce(&AdBlockBaseService::EnableTag,
base::Unretained(this), tag, enabled));
return;
}

Expand All @@ -179,10 +181,9 @@ void AdBlockBaseService::EnableTag(const std::string& tag, bool enabled) {

void AdBlockBaseService::AddResources(const std::string& resources) {
if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
GetTaskRunner()->PostTask(FROM_HERE,
base::BindOnce(&AdBlockBaseService::AddResources,
base::Unretained(this),
resources));
GetTaskRunner()->PostTask(
FROM_HERE, base::BindOnce(&AdBlockBaseService::AddResources,
base::Unretained(this), resources));
return;
}

Expand All @@ -197,9 +198,8 @@ bool AdBlockBaseService::TagExists(const std::string& tag) {
void AdBlockBaseService::GetDATFileData(const base::FilePath& dat_file_path) {
base::PostTaskWithTraitsAndReplyWithResult(
FROM_HERE, {base::ThreadPool(), base::MayBlock()},
base::BindOnce(
&brave_component_updater::LoadDATFileData<adblock::Engine>,
dat_file_path),
base::BindOnce(&brave_component_updater::LoadDATFileData<adblock::Engine>,
dat_file_path),
base::BindOnce(&AdBlockBaseService::OnGetDATFileData,
weak_factory_.GetWeakPtr()));
}
Expand All @@ -215,8 +215,7 @@ void AdBlockBaseService::OnGetDATFileData(GetDATFileDataResult result) {
}
GetTaskRunner()->PostTask(
FROM_HERE, base::BindOnce(&AdBlockBaseService::UpdateAdBlockClient,
base::Unretained(this),
std::move(result.first),
base::Unretained(this), std::move(result.first),
std::move(result.second)));
}

Expand All @@ -231,9 +230,8 @@ void AdBlockBaseService::UpdateAdBlockClient(
}

void AdBlockBaseService::AddKnownTagsToAdBlockInstance() {
std::for_each(tags_.begin(), tags_.end(), [&](const std::string tag) {
ad_block_client_->addTag(tag);
});
std::for_each(tags_.begin(), tags_.end(),
[&](const std::string tag) { ad_block_client_->addTag(tag); });
}

void AdBlockBaseService::AddKnownResourcesToAdBlockInstance() {
Expand All @@ -245,7 +243,7 @@ bool AdBlockBaseService::Init() {
}

void AdBlockBaseService::ResetForTest(const std::string& rules,
const std::string& resources) {
const std::string& resources) {
// This is temporary until adblock-rust supports incrementally adding
// filter rules to an existing instance. At which point the hack below
// will dissapear.
Expand Down
2 changes: 1 addition & 1 deletion components/brave_shields/browser/ad_block_base_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class AdBlockBaseService : public BaseBraveShieldsService {

bool ShouldStartRequest(const GURL &url, content::ResourceType resource_type,
const std::string& tab_host, bool* did_match_exception,
bool* cancel_request_explicitly, std::string* redirect) override;
bool* cancel_request_explicitly, std::string* mock_data_url) override;
void AddResources(const std::string& resources);
void EnableTag(const std::string& tag, bool enabled);
bool TagExists(const std::string& tag);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,12 @@ bool AdBlockRegionalServiceManager::ShouldStartRequest(
const std::string& tab_host,
bool* matching_exception_filter,
bool* cancel_request_explicitly,
std::string* redirect) {
std::string* mock_data_url) {
base::AutoLock lock(regional_services_lock_);
for (const auto& regional_service : regional_services_) {
if (!regional_service.second->ShouldStartRequest(
url, resource_type, tab_host, matching_exception_filter,
cancel_request_explicitly, redirect)) {
cancel_request_explicitly, mock_data_url)) {
return false;
}
if (matching_exception_filter && *matching_exception_filter) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class AdBlockRegionalServiceManager {
const std::string& tab_host,
bool* matching_exception_filter,
bool* cancel_request_explicitly,
std::string* redirect);
std::string* mock_data_url);
void EnableTag(const std::string& tag, bool enabled);
void AddResources(const std::string& resources);
void EnableFilterList(const std::string& uuid, bool enabled);
Expand Down
4 changes: 2 additions & 2 deletions components/brave_shields/browser/ad_block_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ std::string AdBlockService::g_ad_block_component_base64_public_key_(

AdBlockService::AdBlockService(
brave_component_updater::BraveComponent::Delegate* delegate)
: AdBlockBaseService(delegate), weak_factory_(this) {
: AdBlockBaseService(delegate) {
}

AdBlockService::~AdBlockService() {}
Expand Down Expand Up @@ -83,7 +83,7 @@ void AdBlockService::OnComponentReady(const std::string& component_id,
weak_factory_.GetWeakPtr()));
}

void AdBlockService::OnResourcesFileDataReady(std::string resources) {
void AdBlockService::OnResourcesFileDataReady(const std::string& resources) {
g_brave_browser_process->ad_block_service()->AddResources(resources);
g_brave_browser_process->ad_block_regional_service_manager()->AddResources(
resources);
Expand Down
4 changes: 2 additions & 2 deletions components/brave_shields/browser/ad_block_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class AdBlockService : public AdBlockBaseService {
void OnComponentReady(const std::string& component_id,
const base::FilePath& install_dir,
const std::string& manifest) override;
void OnResourcesFileDataReady(std::string resources);
void OnResourcesFileDataReady(const std::string& resources);

private:
friend class ::AdBlockServiceTest;
Expand All @@ -59,7 +59,7 @@ class AdBlockService : public AdBlockBaseService {
const std::string& component_id,
const std::string& component_base64_public_key);

base::WeakPtrFactory<AdBlockService> weak_factory_;
base::WeakPtrFactory<AdBlockService> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(AdBlockService);
};

Expand Down
Loading

0 comments on commit 7e2ccbc

Please sign in to comment.