Domain Reliability: Don't send proxy address, other fixes

Domain Reliability: Don't send proxy address, other fixes

Make sure the monitor does not collect the proxy's address if one was used.
(In the process, refactor RequestInfo to contain the whole HttpResponseInfo,
tighten up OnRequestLegComplete, and poke at unittests a bit.)

Also, count network errors as well as HTTP errors as failures (!).

BUG=356791
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269657

[Deprecated (see juliatuttle), 2014-04-30 22:51:35]

PTAL, sleevi.

[Ryan Sleevi, 2014-04-30 23:20:14]

https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:211: beacon.server_ip = "";
beaon.server_ip.clear()? (prevents an extra copy)

https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
File components/domain_reliability/monitor_unittest.cc (right):

https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor_unittest.cc:176: 
More things to test: 
- Cache behaviour (no network access)
- Redirect chains
  - ... that end up in redirect loops
  - ... that go from an always_report to a never_report [perhaps with an
ambivalent intermediary URL between them]
  - ... that go from a never_report to an always_report [... ditto]
  - ... that cross origins
- non-whitelisted error codes
- whitelisted error codes

Just a few thoughts on things that might have surprising (security) behaviours

[Deprecated (see juliatuttle), 2014-05-02 20:55:08]

On 2014/04/30 23:20:14, Ryan Sleevi wrote:
>
https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
> File components/domain_reliability/monitor.cc (right):
> 
>
https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
> components/domain_reliability/monitor.cc:211: beacon.server_ip = "";
> beaon.server_ip.clear()? (prevents an extra copy)
> 
>
https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
> File components/domain_reliability/monitor_unittest.cc (right):
> 
>
https://codereview.chromium.org/267633002/diff/1/components/domain_reliabilit...
> components/domain_reliability/monitor_unittest.cc:176: 
> More things to test: 
> - Cache behaviour (no network access)

Done.

> - Redirect chains
>   - ... that end up in redirect loops
>   - ... that go from an always_report to a never_report [perhaps with an
> ambivalent intermediary URL between them]
>   - ... that go from a never_report to an always_report [... ditto]
>   - ... that cross origins

Those don't really make sense with the way I'm testing -- my test framework is
injecting raw "request legs", not OnBeforeRedirect / OnCompleted callbacks.

> - non-whitelisted error codes
> - whitelisted error codes


Done.

> Just a few thoughts on things that might have surprising (security) behaviours

Thanks!

[Ryan Sleevi, 2014-05-02 22:49:12]

On 2014/05/02 20:55:08, ttuttle wrote:
> On 2014/04/30 23:20:14, Ryan Sleevi wrote:
> 
> > - Redirect chains
> >   - ... that end up in redirect loops
> >   - ... that go from an always_report to a never_report [perhaps with an
> > ambivalent intermediary URL between them]
> >   - ... that go from a never_report to an always_report [... ditto]
> >   - ... that cross origins
> 
> Those don't really make sense with the way I'm testing -- my test framework is
> injecting raw "request legs", not OnBeforeRedirect / OnCompleted callbacks.
> 

Well, you'll still want to test. Tests make excellent documentation as to
expected behaviour. I honestly have no idea what the expected results would be,
having read the design doc and the implementation :)

[Ryan Sleevi, 2014-05-02 23:00:06]

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:155: response_info.network_accessed;
git-cl-format suggests you should only be indenting four spaces.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:198: request.load_flags &
net::LOAD_DO_NOT_SEND_COOKIES ||
paranoia: Might want to add () around the bitwise &. I've seen enough code where
|| becomes | that it gets... messy :)

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:211: beacon.server_ip = "";
.clear()?

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor_unittest.cc (right):

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:112: }
Interestingly, your test for beacons being sent ignores successful/failed

I'm wondering whether this would make more sense if structured as

size_t GetPendingBeacons(size_t index) {
  BeaconVector beacons;
  context_->GetQueuedDataForTesting(index, &beacons, NULL, NULL);
  return beacons.size();
}

And then in your tests, lines like 147-149, 194-196 just become

EXPECT_EQ(1U, GetPendingBeacons(kAlwaysReportIndex));

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:127:
EXPECT_TRUE(CheckNoBeacons(0));
style nit: Having read this file a few times, the (0) and (1) still surprise me.

Any concerns with adding some constants, like
static const size_t kAlwaysReportIndex = 0u;
static const size_t kNeverReportIndex = 1u;

EXPECT_TRUE(CheckNoBeacons(kAlwaysReportIndex));
EXPECT_TRUE(CheckNoBeacons(kNeverReportIndex));

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:141: 
Move this to line 125?

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:222: EXPECT_EQ("",
beacons[0].server_ip);
AFAICT, you never actually set server_ip, so it's unclear if this test is
passing because you're successful testing, or if it's because you failed to set
server_ip

It would be good if either MakeRequestInfo or your test harness here (214-216)
actually ensured things were configured properly.

[Deprecated (see juliatuttle), 2014-05-06 18:52:05]

PTAL, sleevi.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:155: response_info.network_accessed;
On 2014/05/02 23:00:07, Ryan Sleevi wrote:
> git-cl-format suggests you should only be indenting four spaces.

Done.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:198: request.load_flags &
net::LOAD_DO_NOT_SEND_COOKIES ||
On 2014/05/02 23:00:07, Ryan Sleevi wrote:
> paranoia: Might want to add () around the bitwise &. I've seen enough code
where
> || becomes | that it gets... messy :)

Done.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:211: beacon.server_ip = "";
On 2014/05/02 23:00:07, Ryan Sleevi wrote:
> .clear()?

Done.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor_unittest.cc (right):

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:112: }
On 2014/05/02 23:00:07, Ryan Sleevi wrote:
> Interestingly, your test for beacons being sent ignores successful/failed
> 
> I'm wondering whether this would make more sense if structured as
> 
> size_t GetPendingBeacons(size_t index) {
>   BeaconVector beacons;
>   context_->GetQueuedDataForTesting(index, &beacons, NULL, NULL);
>   return beacons.size();
> }
> 
> And then in your tests, lines like 147-149, 194-196 just become
> 
> EXPECT_EQ(1U, GetPendingBeacons(kAlwaysReportIndex));

Done.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:127:
EXPECT_TRUE(CheckNoBeacons(0));
On 2014/05/02 23:00:07, Ryan Sleevi wrote:
> style nit: Having read this file a few times, the (0) and (1) still surprise
me.
> 
> Any concerns with adding some constants, like
> static const size_t kAlwaysReportIndex = 0u;
> static const size_t kNeverReportIndex = 1u;
> 
> EXPECT_TRUE(CheckNoBeacons(kAlwaysReportIndex));
> EXPECT_TRUE(CheckNoBeacons(kNeverReportIndex));

Done.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:141: 
On 2014/05/02 23:00:07, Ryan Sleevi wrote:
> Move this to line 125?

Done.

https://codereview.chromium.org/267633002/diff/40001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:222: EXPECT_EQ("",
beacons[0].server_ip);
On 2014/05/02 23:00:07, Ryan Sleevi wrote:
> AFAICT, you never actually set server_ip, so it's unclear if this test is
> passing because you're successful testing, or if it's because you failed to
set
> server_ip
> 
> It would be good if either MakeRequestInfo or your test harness here (214-216)
> actually ensured things were configured properly.

Done.

[Ryan Sleevi, 2014-05-06 19:04:32]

I think you're based on the wrong branch-hash at the moment, since I'm seeing
diffs from your other CL.

[Ryan Sleevi, 2014-05-06 22:43:42]

lgtm

https://codereview.chromium.org/267633002/diff/110001/components/domain_relia...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/267633002/diff/110001/components/domain_relia...
components/domain_reliability/context.cc:144: bool success = (beacon.status ==
"ok");
unrelated?

[Deprecated (see juliatuttle), 2014-05-07 03:13:09]

https://codereview.chromium.org/267633002/diff/110001/components/domain_relia...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/267633002/diff/110001/components/domain_relia...
components/domain_reliability/context.cc:144: bool success = (beacon.status ==
"ok");
On 2014/05/06 22:43:42, Ryan Sleevi wrote:
> unrelated?

Yes, but I spotted it writing unittests :)

[Deprecated (see juliatuttle), 2014-05-09 22:27:10]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-05-09 22:29:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/267633002/210001

[Deprecated (see juliatuttle), 2014-05-10 20:07:26]

The CQ bit was unchecked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-05-10 20:07:35]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-05-10 20:07:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/267633002/210001

[commit-bot: I haz the power, 2014-05-11 00:43:52]

Change committed as 269657