Fixing redirect through DataReductionProxy logic.

Fixing redirect through DataReductionProxy logic.

Redirects through DataReductionProxy were counting as a DRP
navigations/pageloads even when the new URL that was redirected to was
non-DRP. This affects DRP PageLoad.* UMA and the DRP pingback.

This introduces a reset_to_default method on DRPData as well as changing
using original_url from URLRequest to just URL (as DRP cares more about
the actual URL of the site rather than the original URL before
redirects).

BUG=565421
Committed: https://crrev.com/960971fdee189de4b0879017475dd7d8bca0e659
Cr-Commit-Position: refs/heads/master@{#419514}

[RyanSturm, 2016-09-08 17:50:10]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 17:50:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-08 18:37:00]

ryansturm@chromium.org changed reviewers:
+ tbansal@chromium.org

[RyanSturm, 2016-09-08 18:37:01]

tbansal: PTAL @ *

[commit-bot: I haz the power, 2016-09-08 18:38:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 18:38:39]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-09 18:42:03]

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc
(right):

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:49:
void DataReductionProxyData::reset_to_default() {
This is a very weird function. It is essentially doing the object construction.
It is possible that a new field is added to the object, and we forget to reset
it here leading to bugs.

why not instead add method:
void ResetData(URLRequest* request) 
to DataReductionProxyData class?

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:50:
void set_request_url(const GURL& request_url) { request_url_ = request_url; }
May be add a DCHECK right here that |request_url_| is not cryptographic?

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:63:
void reset_to_default();
Why not ResetToDefault()?

[RyanSturm, 2016-09-09 19:34:06]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 19:35:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-09 19:36:07]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 19:36:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-09 19:37:33]

tbansal: I made changes, but they don't exactly line up with your comments. PTAL
again, thanks.

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc
(right):

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:49:
void DataReductionProxyData::reset_to_default() {
On 2016/09/09 18:42:03, tbansal1 wrote:
> This is a very weird function. It is essentially doing the object
construction.
> It is possible that a new field is added to the object, and we forget to reset
> it here leading to bugs.
> 
> why not instead add method:
> void ResetData(URLRequest* request) 
> to DataReductionProxyData class?

Done.

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:50:
void set_request_url(const GURL& request_url) { request_url_ = request_url; }
On 2016/09/09 18:42:03, tbansal1 wrote:
> May be add a DCHECK right here that |request_url_| is not cryptographic?

The DCHECK here wouldn't have prevented the problem in the first place since
this URL was set as the last flywheel url on the url_chain before. I don't think
it's worth adding here.

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:63:
void reset_to_default();
On 2016/09/09 18:42:03, tbansal1 wrote:
> Why not ResetToDefault()?

Done.

[tbansal1, 2016-09-09 20:06:32]

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2326443004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:50:
void set_request_url(const GURL& request_url) { request_url_ = request_url; }
On 2016/09/09 19:37:33, Ryan Sturm wrote:
> On 2016/09/09 18:42:03, tbansal1 wrote:
> > May be add a DCHECK right here that |request_url_| is not cryptographic?
> 
> The DCHECK here wouldn't have prevented the problem in the first place since
> this URL was set as the last flywheel url on the url_chain before. I don't
think
> it's worth adding here.

Acknowledged.

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:48: //
The URL of the request after redirects.
May be update the method comment. See my other comment below.

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:90: //
The URL of the request after redirects.
This seems like slightly inaccurate description since at the beginning of
navigation it may be set to the original URL (at that point it is not actually
the URL after redirects). May be you want to say similar to what
navigation_handle.h says:
"/ The URL the frame is navigating to. This may change during the navigation
  // when encountering a server redirect."

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc
(right):

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:114:
TEST_F(DataReductionProxyDataTest, Reset) {
s/Reset/ClearData/

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:656:
// Simulate a redirect by calling NotifyBeforeSendHeaders again with different
If you prefer to write a more integrated test, then you can use embedded test
server, and fetch redirect301-to-https which returns 301. 

Similar to
https://cs.chromium.org/chromium/src/net/url_request/url_request_unittest.cc?....

[commit-bot: I haz the power, 2016-09-09 20:38:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 20:38:35]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-09-09 20:41:55]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[RyanSturm, 2016-09-09 20:42:20]

tbansal: PTAL

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:48: //
The URL of the request after redirects.
On 2016/09/09 20:06:32, tbansal1 wrote:
> May be update the method comment. See my other comment below.

Done.

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:90: //
The URL of the request after redirects.
On 2016/09/09 20:06:32, tbansal1 wrote:
> This seems like slightly inaccurate description since at the beginning of
> navigation it may be set to the original URL (at that point it is not actually
> the URL after redirects). May be you want to say similar to what
> navigation_handle.h says:
> "/ The URL the frame is navigating to. This may change during the navigation
>   // when encountering a server redirect."

Done.

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc
(right):

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:114:
TEST_F(DataReductionProxyDataTest, Reset) {
On 2016/09/09 20:06:32, tbansal1 wrote:
> s/Reset/ClearData/

Done.

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2326443004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:656:
// Simulate a redirect by calling NotifyBeforeSendHeaders again with different
On 2016/09/09 20:06:32, tbansal1 wrote:
> If you prefer to write a more integrated test, then you can use embedded test
> server, and fetch redirect301-to-https which returns 301. 
> 
> Similar to
>
https://cs.chromium.org/chromium/src/net/url_request/url_request_unittest.cc?....

I think from the perspective of DRPNetworkDelegate, calling it twice is
sufficient, and we don't need to actual cause a redirect with an embedded test
server.

[commit-bot: I haz the power, 2016-09-09 20:42:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-09 20:43:52]

ryansturm@chromium.org changed reviewers:
+ bmcquade@chromium.org

[RyanSturm, 2016-09-09 20:43:53]

bmcquade: PTAL @ data_reduction_proxy_metric_observer.*

[tbansal1, 2016-09-09 20:54:36]

lgtm

[RyanSturm, 2016-09-09 21:02:30]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2016-09-09 21:02:36]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-09-09 21:02:44]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-09-09 21:03:05]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2016-09-13 23:31:52]

bmcquade: PTAL @ page_load_metrics/*

[RyanSturm, 2016-09-19 16:43:33]

ryansturm@chromium.org changed reviewers:
+ csharrison@chromium.org
- bmcquade@chromium.org

[RyanSturm, 2016-09-19 16:43:34]

csharrison: PTAL at page_load_metrics/*

-bmcquade

[Charlie Harrison, 2016-09-19 16:50:22]

page_load_metrics LGTM

[RyanSturm, 2016-09-19 16:52:39]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-09-19 16:52:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 18:03:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 18:03:31]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[RyanSturm, 2016-09-19 18:07:58]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-09-19 18:08:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 18:39:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-19 18:42:36]

Description was changed from

==========
Fixing redirect through DataReductionProxy logic.

Redirects through DataReductionProxy were counting as a DRP
navigations/pageloads even when the new URL that was redirected to was
non-DRP. This affects DRP PageLoad.* UMA and the DRP pingback.

This introduces a reset_to_default method on DRPData as well as changing
using original_url from URLRequest to just URL (as DRP cares more about
the actual URL of the site rather than the original URL before
redirects).

BUG=565421
==========

to

==========
Fixing redirect through DataReductionProxy logic.

Redirects through DataReductionProxy were counting as a DRP
navigations/pageloads even when the new URL that was redirected to was
non-DRP. This affects DRP PageLoad.* UMA and the DRP pingback.

This introduces a reset_to_default method on DRPData as well as changing
using original_url from URLRequest to just URL (as DRP cares more about
the actual URL of the site rather than the original URL before
redirects).

BUG=565421

Committed: https://crrev.com/960971fdee189de4b0879017475dd7d8bca0e659
Cr-Commit-Position: refs/heads/master@{#419514}
==========

[commit-bot: I haz the power, 2016-09-19 18:42:37]

Patchset 4 (id:??) landed as
https://crrev.com/960971fdee189de4b0879017475dd7d8bca0e659
Cr-Commit-Position: refs/heads/master@{#419514}