Commit fb93bf47 authored by sclittle's avatar sclittle Committed by Commit bot

Don't report DRP request error code UMA when LOAD_BYPASS_PROXY is set.

This fixes a reporting bug with the
DataReductionProxy.RequestCompletionErrorCodes histograms. Previously,
these histograms counted requests that encountered errors when retrying
the request after bypassing the data reduction proxy (e.g. block-once).
This is incorrect because the error did not occur when trying to use the
DRP.

The fix is to ignore requests that completed with the LOAD_BYPASS_PROXY
load flag, since these requests did not use the DRP even if the
proxy_server() in the URLRequest is still set to the DRP.

BUG=454947

Review URL: https://codereview.chromium.org/896793004

Cr-Commit-Position: refs/heads/master@{#314432}
parent 36a90457
...@@ -128,8 +128,13 @@ void DataReductionProxyUsageStats::OnUrlRequestCompleted( ...@@ -128,8 +128,13 @@ void DataReductionProxyUsageStats::OnUrlRequestCompleted(
DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(thread_checker_.CalledOnValidThread());
DataReductionProxyTypeInfo proxy_info; DataReductionProxyTypeInfo proxy_info;
// Ignore requests that did not use the data reduction proxy. The check for
// LOAD_BYPASS_PROXY is necessary because the proxy_server() in the |request|
// might still be set to the data reduction proxy if |request| was retried
// over direct and a network error occurred while retrying it.
if (data_reduction_proxy_params_->WasDataReductionProxyUsed(request, if (data_reduction_proxy_params_->WasDataReductionProxyUsed(request,
&proxy_info)) { &proxy_info) &&
(request->load_flags() & net::LOAD_BYPASS_PROXY) == 0) {
if (request->status().status() == net::URLRequestStatus::SUCCESS) { if (request->status().status() == net::URLRequestStatus::SUCCESS) {
successful_requests_through_proxy_count_++; successful_requests_through_proxy_count_++;
NotifyUnavailabilityIfChanged(); NotifyUnavailabilityIfChanged();
......
...@@ -448,22 +448,25 @@ TEST_F(DataReductionProxyUsageStatsTest, RequestCompletionErrorCodes) { ...@@ -448,22 +448,25 @@ TEST_F(DataReductionProxyUsageStatsTest, RequestCompletionErrorCodes) {
struct TestCase { struct TestCase {
bool was_proxy_used; bool was_proxy_used;
bool is_load_bypass_proxy;
bool is_fallback; bool is_fallback;
bool is_main_frame; bool is_main_frame;
net::Error net_error; net::Error net_error;
}; };
const TestCase test_cases[] = { const TestCase test_cases[] = {
{false, false, true, net::OK}, {false, true, false, true, net::OK},
{false, false, false, net::ERR_TOO_MANY_REDIRECTS}, {false, true, false, false, net::ERR_TOO_MANY_REDIRECTS},
{true, false, true, net::OK}, {false, false, false, true, net::OK},
{true, false, true, net::ERR_TOO_MANY_REDIRECTS}, {false, false, false, false, net::ERR_TOO_MANY_REDIRECTS},
{true, false, false, net::OK}, {true, false, false, true, net::OK},
{true, false, false, net::ERR_TOO_MANY_REDIRECTS}, {true, false, false, true, net::ERR_TOO_MANY_REDIRECTS},
{true, true, true, net::OK}, {true, false, false, false, net::OK},
{true, true, true, net::ERR_TOO_MANY_REDIRECTS}, {true, false, false, false, net::ERR_TOO_MANY_REDIRECTS},
{true, true, false, net::OK}, {true, false, true, true, net::OK},
{true, true, false, net::ERR_TOO_MANY_REDIRECTS} {true, false, true, true, net::ERR_TOO_MANY_REDIRECTS},
{true, false, true, false, net::OK},
{true, false, true, false, net::ERR_TOO_MANY_REDIRECTS}
}; };
for (size_t i = 0; i < arraysize(test_cases); ++i) { for (size_t i = 0; i < arraysize(test_cases); ++i) {
...@@ -478,6 +481,10 @@ TEST_F(DataReductionProxyUsageStatsTest, RequestCompletionErrorCodes) { ...@@ -478,6 +481,10 @@ TEST_F(DataReductionProxyUsageStatsTest, RequestCompletionErrorCodes) {
scoped_ptr<net::URLRequest> fake_request( scoped_ptr<net::URLRequest> fake_request(
CreateURLRequestWithResponseHeaders(GURL("http://www.google.com/"), CreateURLRequestWithResponseHeaders(GURL("http://www.google.com/"),
raw_headers)); raw_headers));
if (test_cases[i].is_load_bypass_proxy) {
fake_request->SetLoadFlags(fake_request->load_flags() |
net::LOAD_BYPASS_PROXY);
}
if (test_cases[i].is_main_frame) { if (test_cases[i].is_main_frame) {
fake_request->SetLoadFlags(fake_request->load_flags() | fake_request->SetLoadFlags(fake_request->load_flags() |
net::LOAD_MAIN_FRAME); net::LOAD_MAIN_FRAME);
...@@ -497,27 +504,29 @@ TEST_F(DataReductionProxyUsageStatsTest, RequestCompletionErrorCodes) { ...@@ -497,27 +504,29 @@ TEST_F(DataReductionProxyUsageStatsTest, RequestCompletionErrorCodes) {
usage_stats->OnUrlRequestCompleted(fake_request.get(), false); usage_stats->OnUrlRequestCompleted(fake_request.get(), false);
if (test_cases[i].was_proxy_used && !test_cases[i].is_fallback) { if (test_cases[i].was_proxy_used && !test_cases[i].is_load_bypass_proxy &&
!test_cases[i].is_fallback) {
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
kPrimaryHistogramName, -net_error_int, 1); kPrimaryHistogramName, -net_error_int, 1);
} else { } else {
histogram_tester.ExpectTotalCount(kPrimaryHistogramName, 0); histogram_tester.ExpectTotalCount(kPrimaryHistogramName, 0);
} }
if (test_cases[i].was_proxy_used && test_cases[i].is_fallback) { if (test_cases[i].was_proxy_used && !test_cases[i].is_load_bypass_proxy &&
test_cases[i].is_fallback) {
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
kFallbackHistogramName, -net_error_int, 1); kFallbackHistogramName, -net_error_int, 1);
} else { } else {
histogram_tester.ExpectTotalCount(kFallbackHistogramName, 0); histogram_tester.ExpectTotalCount(kFallbackHistogramName, 0);
} }
if (test_cases[i].was_proxy_used && !test_cases[i].is_fallback && if (test_cases[i].was_proxy_used && !test_cases[i].is_load_bypass_proxy &&
test_cases[i].is_main_frame) { !test_cases[i].is_fallback && test_cases[i].is_main_frame) {
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
kPrimaryMainFrameHistogramName, -net_error_int, 1); kPrimaryMainFrameHistogramName, -net_error_int, 1);
} else { } else {
histogram_tester.ExpectTotalCount(kPrimaryMainFrameHistogramName, 0); histogram_tester.ExpectTotalCount(kPrimaryMainFrameHistogramName, 0);
} }
if (test_cases[i].was_proxy_used && test_cases[i].is_fallback && if (test_cases[i].was_proxy_used && !test_cases[i].is_load_bypass_proxy &&
test_cases[i].is_main_frame) { test_cases[i].is_fallback && test_cases[i].is_main_frame) {
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
kFallbackMainFrameHistogramName, -net_error_int, 1); kFallbackMainFrameHistogramName, -net_error_int, 1);
} else { } else {
......
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