Randomizing seed page id for DRP

Randomizing seed page id for DRP

The seed page id used for DRP should be randomized to avoid collisions
with cached configs.

BUG=733771
Review-Url: https://codereview.chromium.org/2940163002
Cr-Commit-Position: refs/heads/master@{#480095}
Committed: https://chromium.googlesource.com/chromium/src/+/4704a57b1bde5279f8643c2db0b2778430e43e70

[RyanSturm, 2017-06-15 20:10:52]

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

[commit-bot: I haz the power, 2017-06-15 20:11:29]

Dry run: CQ is trying da patch.

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

[RyanSturm, 2017-06-15 20:11:58]

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

[RyanSturm, 2017-06-15 20:11:58]

tbansal: PTAL

There isn't a great way to verify that a new session gets a new page id without
some flakiness (1/2^64). I'm still thinking of a test for that which will just
mock some behavior.

[RyanSturm, 2017-06-15 20:29:57]

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

[commit-bot: I haz the power, 2017-06-15 20:30:36]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-15 20:47:04]

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

https://codereview.chromium.org/2940163002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:85:
ResetPageId();
It would be preferable if we do not call class method inside the ctor.

https://codereview.chromium.org/2940163002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:236:
ResetPageId();
may be rm this now?

https://codereview.chromium.org/2940163002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:332:
current_page_id_ = base::RandGenerator(UINTMAX_MAX);
Why not just call RandUint64();??

otherwise, it seems this should be UINT64_MAX.

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

https://codereview.chromium.org/2940163002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:102:
// Resets the page ID for a new session. Virtual for testing.
Where is it called in tests?
Instead, can we rather just check in tests that it is set to a random value?

[RyanSturm, 2017-06-15 20:55:34]

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

[commit-bot: I haz the power, 2017-06-15 20:55:54]

Dry run: CQ is trying da patch.

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

[RyanSturm, 2017-06-15 23:10:55]

tbansal: PTAL, fixed relevant comments.

[tbansal1, 2017-06-16 00:23:14]

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

https://codereview.chromium.org/2940163002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1888:
page_id = io_data()->request_options()->GeneratePageId();
If you want to check if SetSecureSession generates a new page ID, then I think
it is fine to do it using a for-loop, even if there is a very small chance that
the test will fail. There are already tests in base/rand_util_unittest.cc that
have non-zero (albeit very tiny) probability of failing.

https://codereview.chromium.org/2940163002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2940163002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:236:
ResetPageId();
I think you brought up a crucial observation that it is important to
ResetPageId() when session_id changes. Mind adding that as a comment?

[RyanSturm, 2017-06-16 16:37:26]

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

[commit-bot: I haz the power, 2017-06-16 16:37:51]

Dry run: CQ is trying da patch.

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

[RyanSturm, 2017-06-16 17:05:39]

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

https://codereview.chromium.org/2940163002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1888:
page_id = io_data()->request_options()->GeneratePageId();
On 2017/06/16 00:23:14, tbansal1 wrote:
> If you want to check if SetSecureSession generates a new page ID, then I think
> it is fine to do it using a for-loop, even if there is a very small chance
that
> the test will fail. There are already tests in base/rand_util_unittest.cc that
> have non-zero (albeit very tiny) probability of failing.

Acknowledged.

https://codereview.chromium.org/2940163002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2940163002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:236:
ResetPageId();
On 2017/06/16 00:23:14, tbansal1 wrote:
> I think you brought up a crucial observation that it is important to
> ResetPageId() when session_id changes. Mind adding that as a comment?

Done.

[tbansal1, 2017-06-16 17:07:02]

lgtm

[RyanSturm, 2017-06-16 17:25:02]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2017-06-16 17:25:02]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2017-06-16 17:25:18]

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, 2017-06-16 17:44:54]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497633902369260,
"parent_rev": "9aa67aaac041b7ed58f2db22ae81ab9872bd3e07", "commit_rev":
"4704a57b1bde5279f8643c2db0b2778430e43e70"}

[commit-bot: I haz the power, 2017-06-16 17:45:06]

Description was changed from

==========
Randomizing seed page id for DRP

The seed page id used for DRP should be randomized to avoid collisions
with cached configs.

BUG=733771
==========

to

==========
Randomizing seed page id for DRP

The seed page id used for DRP should be randomized to avoid collisions
with cached configs.

BUG=733771

Review-Url: https://codereview.chromium.org/2940163002
Cr-Commit-Position: refs/heads/master@{#480095}
Committed:
https://chromium.googlesource.com/chromium/src/+/4704a57b1bde5279f8643c2db0b2...
==========

[commit-bot: I haz the power, 2017-06-16 17:45:07]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4704a57b1bde5279f8643c2db0b2...