Domain Reliability: More security review.

Domain Reliability: More security review.

Change some trivial stuff so sleevi can do more security review.
Also, add rdsmith as an OWNER.

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

[Deprecated (see juliatuttle), 2014-04-24 21:30:41]

PTAL, rleevi ^.^

[Ryan Sleevi, 2014-04-25 01:00:02]

There's a lot here. I'm publishing first remarks, I got up to
context_unittest.cc.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.cc:24: net::ErrorToString(chrome_error));
I don't think we should be arbitrarily providing the error code, for two
reasons:

1) It makes our internal error codes a de-facto public API (unless this is
clearly documented as 'informative and subject to change'). Still, I have strong
fear this will end up being "user agent strings" all over again (eg: we can't
change because people expect X to happen)

2) This leaks a variety of state about the clients' local network configuration
that the remote site would otherwise be unable or unauthorized to view. If we do
this, we need to sit down and create an explicit whitelist of errors we "know"
will never contain private data or would otherwise be detectable by the server.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.h:20: class DOMAIN_RELIABILITY_EXPORT
DomainReliabilityBeacon {
Why is this a class and not a struct?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.h:42: // Okay to copy and assign. ^_^
No need for emoticons either way.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:38: }
You should have unittests that cover this - in particular the wildcard matching
issues I described.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:44: }
API design: It's worth noting that calling this multiple times for a Resource
may return non-deterministic results, which can run into TOCTTOU issues

Perhaps you should pre-calculate this for a Resource?

Also, this can return 'bad' results if !IsValid()

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:91: }
style: inconsistent bracing (compare 93/94 and 84/85... and rest of file,
really)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:100: if (valid_until == 0.0 ||
domain.empty() || version.empty() ||
What about negative values in valid_until ?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:119: base::Time valid_until_time =
base::Time::FromDoubleT(valid_until);
Note that this can fail if IsValid() isn't checked before hand.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:124: const std::string& url_string =
url.spec();
Can this potentially allow sites to perform username matching rules on their
collecting URLs?

Does it mean that they can potentially collect usernames?

I can't help but feel like |url| may need more sanitization. This is where
having a comprehensive set of unittests can easily answer that question by
"adding tests".

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.h:36: bool MatchesUrlString(const
std::string& url_string) const;
Why do you pass this as "url_string", rather than a GURL, which gives somewhat
more precise control over matching?

In particular, I'm concerned whether you can confound the matching algorithm
(which is a rather simple MatchPattern) through the use of query arguments.

Consider a match pattern of http*://mydomain.com

Now, consider a URL of
  http://victim.com/?some_query_parameter=://mydomain.com

Yes, some_query_parameter SHOULD be URL encoded as %3A%2F%2Ftest when sent over
the network. However, depending on how the caller handles/converts the URL
string, they may have allowed attacker data through. Your implementation would
allow it, seeing it as

http(stuff matching wildcard)://mydomain.com, reporting the failure of
victim.com to mydomain.com.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:27: const char* kReporter = "chrome";
layering violation: This should not be hardcoded in //components.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:61: return;
size_t & npos (aka ((size_t)~0)) for a safer approach here

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:78: << beacon.elapsed.InMilliseconds()
<< "ms";
every VLOG costs space in release. Make sure you're making the right trade-offs
here, and perhaps take a bug to clean it up after ship & stablization.

With our frequent canary releases, it's often easier to add logging in on a
case-by-case basis than leave it there "for when you need it"

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:106:
DCHECK_GT(static_cast<int>(states_.size()), resource_index);
-_-

static_cast<int> of a size_t? Aww hell naw...

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:110: std::copy(state.beacons.begin(),
state.beacons.end(), beacons_out->begin());
beacons_out->assign(state.becons.begin(), state.becons.end()) also works.
Different tradeoffs, but probably fine for testing.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:9: #include <map>
IWYU: vector (line 48)
DIWYDU: delete map - you don't use it.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:11: #include "base/memory/ref_counted.h"
IWYU: I don't see any refcounting going on here.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:16: #include
"components/domain_reliability/config.h"
Normally you'd be able to forward declare this, if ::Resource wasn't an inner
class (see http://www.chromium.org/developers/coding-style/cpp-dos-and-donts )

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:34: MockableTime* time,
nit: Should "MockableTime" be called what it is, rather than how it can be used
- eg: TimeSource (which is what we've used in things like cc/ and the
base/profiler code) or TimeProvider (used in base/ and extensions/)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:39: virtual ~DomainReliabilityContext();
Why is this virtual, when you have no virtual methods and are not implementing
any interfaces?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:43: // the request counters in any case.
Should this be called "RegisterBeacon" instead? Or something more event-notify'y
like "OnBeacon"?

The "add" in AddBeacon seems like it will take some concrete step with the
Beacon being added to something, but that's not entirely the case. Yes, you
increment the request counters, but you don't add the beacon to be uploaded,
which is the first thing I think of with "AddBeacon"

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:50: int* failed_requests_out) const;
For "counts of things" and indices, use unsigned integer types

See http://www.chromium.org/developers/coding-style#TOC-Types

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:56: static const int kMaxQueuedBeacons;
For "counts of things", use unsigned integer types

See http://www.chromium.org/developers/coding-style#TOC-Types

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:60: class ResourceState {
Because you the ScopedVector<> isn't instantiated until the ctor/dtor (in the
.cc), and you have copy and assignment disabled, you should be able to forward
declare this entire class.

That would also allow you to forward declare DomainReliabilityBeacon, since
std::deque<> is the only thing preventing it from being so.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:78: // Removes the oldest beacon.
DCHECKs if there isn't one.
nit: newline between 77/78.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:92: int uploading_failed_requests;
size_t for 85, 86, 91, 92

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:117: void RemoveOldestBeacon();
I find it weird to see 108-117 duplicated with effectively 68-79, for what it's
worth.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:129: int uploading_beacon_count_;
size_t for counts

[Ryan Sleevi, 2014-04-26 02:37:38]

Up through uploader_unittest - I've largely been skipping the unittest reviews.

At least one (potential) security bug here, see below.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/dispatcher.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/dispatcher.h:11: #include
"base/memory/scoped_vector.h"
IWYU: Unused

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/dispatcher.h:29:
DomainReliabilityDispatcher(MockableTime* time);
style: explicit

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/dispatcher.h:41: scoped_ptr<MockableTime::Timer>
timer_p,
nit: What's up with the naming here?
nit: You can also keep this class forward declared and entirely in the impl

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/domain_reliability_export.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/domain_reliability_export.h:11: // Component build
on Windows
Naturally, you won't want to commit these comments :)

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:50: };
Why have this at all? If you need a URLRequestContextGetter, why not simply
*take* a URLRequestContextGetter?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:138: : url(request.url()),
If request.url().is_valid() is false, you'll never log this, correct? So why not
filter these out in OnCompleted / OnBeforeRedirect?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:161: // Grab domain before we
config.Pass().
// Grab a copy of the domain before transferring ownership of |config|

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:206: beacon.server_ip =
request.socket_address.host();
SECURITY/PRIVACY: Is there any risk that this may disclose the users' proxy IP?
If so, bug :)

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:34: // |ChromeNetworkDelegate|, which
calls the On* methods.
You're a component now, you don't get to talk about ChromeNetworkDelegate
(that's a layering violation).

Instead, describe this class, it's requirements (eg: that it live on the same
thread as the net::URLRequestContext, which must outlive it), and how to use it
(eg: implement a net::NetworkDelegate that calls the On* methods as appropriate)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:38: // I/O thread.
NB: Pronouns in comments considered harmful :)

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...

Rather than discuss this in the constructor, document it in the class design
itself

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:45: // Adds the "baked-in"
configuration(s) for Google sites.
// Add in configurations that have been configured at compile-time.

Yes, as the reviewer who stamped this, I'm a horrible person :)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:49: // events:
Components don't get to talk about profiles either (AIUI)

Ideally, however, you also wouldn't want to talk about NetworkDelegate - it's
not an API contract of yours that users implement in terms of NetworkDelegate -
it just happens to be the most convenient way, and that has a 1:1 mapping.

Instead, describe *your* contract and expectations for these methods to be
called. What are your exact pre-conditions/post-conditions?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:65: RequestInfo(const net::URLRequest&
request);
style: explicit

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:86:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
You don't take one (because of threading), but you live on one? What's up with
that?

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.cc:40: return value;
Why Int? Why not UInt?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.cc:182: for (unsigned i = 0; i <
collectors_.size(); ++i) {
s/unsigned/size_t/ - use the proper type.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.cc:187: min_index = i;
SECURITY BUG: You just sunk my battleship :(

You coerce an unsigned to a signed here. So now we have a chance of getting a
negative number into *collector_index_out (for a large value of "i".

On line 99, you store this value into collector_index_.

If I call OnUploadComplete after this, you've now just walked backward in the
stack. And broken my heart. And my security, because you write to it in (line
119/126)

This is why you always leave a note... or always use the right integer types :)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/scheduler.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.h:53: // ScheduleUploadCallback (if all
previous beacons have been uploaded).
Comment nit: Don't describe how this *is* used, describe what it does and how to
use it.

For example, my first reaction is "Called by who?"
My next reaction is "Hrm, On* methods are usually used for delegate. Is this a
delegate? No? So when do I call this?"

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.h:57: void OnUploadStart(int*
collector_index_out);
BUG: You define |num_collectors| as a size_t, but then co-erce to an int (also
line 78). Your types should match - and, since they're indices, they should be
size_t

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.cc:20: : public
DomainReliabilityUploader, net::URLFetcherDelegate {
"git cl format" should clean this up for you, as it has my style-sense tingling.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.cc:23:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter)
const scoped_refptr<...>&

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.cc:68: delete callback_it->first;
You don't have to de-constify this? (See line 74)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/uploader.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.h:22: class DOMAIN_RELIABILITY_EXPORT
DomainReliabilityUploader {
Document.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.h:31:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter);
const scoped_refptr<...>&

Avoids an extra refcount cycle, still has the same effect.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.h:33: virtual void UploadReport(const
std::string& report_json,
Document.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader_unittest.cc:52: std::map<int, bool>
upload_successful_;
size_t

[Ryan Sleevi, 2014-04-26 02:40:27]

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/util.cc:91: } else {
Don't use "else" after a return -
http://www.chromium.org/developers/coding-style#Code_Formatting

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/util.cc:92: for (size_t i = 0; i <
arraysize(net_error_map); i++) {
*shrug* - worth mentioning http://www.cplusplus.com/reference/cstdlib/bsearch/ ?
Seems like this is going to be performance critical code.

[Deprecated (see juliatuttle), 2014-04-28 22:01:12]

PTAL, sleevi. (Note a couple of TODOs I'm working on in the change description.)

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.cc:24: net::ErrorToString(chrome_error));
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> I don't think we should be arbitrarily providing the error code, for two
> reasons:
> 
> 1) It makes our internal error codes a de-facto public API (unless this is
> clearly documented as 'informative and subject to change'). Still, I have
strong
> fear this will end up being "user agent strings" all over again (eg: we can't
> change because people expect X to happen)

We can document it as that; right now, it's not documented at all.

> 2) This leaks a variety of state about the clients' local network
configuration
> that the remote site would otherwise be unable or unauthorized to view. If we
do
> this, we need to sit down and create an explicit whitelist of errors we "know"
> will never contain private data or would otherwise be detectable by the
server.

We have that whitelist already; it's in DomainReliabilityUtil::GetBeaconStatus.
Without an entry in that mapping, we won't report the request at all.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.h:20: class DOMAIN_RELIABILITY_EXPORT
DomainReliabilityBeacon {
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Why is this a class and not a struct?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.h:42: // Okay to copy and assign. ^_^
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> No need for emoticons either way.

:(

Done.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:38: }
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> You should have unittests that cover this - in particular the wildcard
matching
> issues I described.

I don't think they're security-relevant, but I have beefed up the unittest to
try some simple wildcard examples.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:44: }
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> API design: It's worth noting that calling this multiple times for a Resource
> may return non-deterministic results, which can run into TOCTTOU issues

We shouldn't have TOCTOU issues; once the config is installed into a context,
it's const, so the probabilities can't change. (Also, it's not a
security-critical result, just a coin toss.)

> Perhaps you should pre-calculate this for a Resource?

The idea is that it's not pre-calculated; we want a random sample of the
requests.  If we precalculated it, we'd get either 100% or 0% of the beacons for
a resource.

> Also, this can return 'bad' results if !IsValid()

What's a good way to deal with this? If the probabilities are just out of range,
it'll act as if they're clamped to the nearest endpoint. I could just DCHECK
that it's a valid range before using it.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:91: }
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> style: inconsistent bracing (compare 93/94 and 84/85... and rest of file,
> really)

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:100: if (valid_until == 0.0 ||
domain.empty() || version.empty() ||
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> What about negative values in valid_until ?

They are very expired.  I would not suggest providing one.  They are not
invalid, though.

(JSONValueConverter has no way to mark a field as required, so I initialize it
to 0.0, and if it's still 0.0 when I'm done, I assume it was not there.)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:119: base::Time valid_until_time =
base::Time::FromDoubleT(valid_until);
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Note that this can fail if IsValid() isn't checked before hand.

Added DCHECK.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.cc:124: const std::string& url_string =
url.spec();
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Can this potentially allow sites to perform username matching rules on their
> collecting URLs?
> 
> Does it mean that they can potentially collect usernames?
> 
> I can't help but feel like |url| may need more sanitization. This is where
> having a comprehensive set of unittests can easily answer that question by
> "adding tests".

I think by the time the URL hits here, it doesn't have one, but that's a good
idea. I'm passing it through the same filter that's used for the referrer
header.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.h:36: bool MatchesUrlString(const
std::string& url_string) const;
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Why do you pass this as "url_string", rather than a GURL, which gives somewhat
> more precise control over matching?
> 
> In particular, I'm concerned whether you can confound the matching algorithm
> (which is a rather simple MatchPattern) through the use of query arguments.
> 
> Consider a match pattern of http*://mydomain.com
> 
> Now, consider a URL of
>   http://victim.com/?some_query_parameter=://mydomain.com
> 
> Yes, some_query_parameter SHOULD be URL encoded as %3A%2F%2Ftest when sent
over
> the network. However, depending on how the caller handles/converts the URL
> string, they may have allowed attacker data through. Your implementation would
> allow it, seeing it as
> 
> http(stuff matching http://wildcard)://mydomain.com, reporting the failure of
> http://victim.com to http://mydomain.com.

The config is only consulted once we've checked monitored_domains -- that is a
very simple map lookup, and looks directly at the hostname of the URL.  (That'll
change to origin before we allow untrusted configs, but even now it wouldn't
allow shenanigans like the above.)

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/config.h:36: bool MatchesUrlString(const
std::string& url_string) const;
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Why do you pass this as "url_string", rather than a GURL, which gives somewhat
> more precise control over matching?
> 
> In particular, I'm concerned whether you can confound the matching algorithm
> (which is a rather simple MatchPattern) through the use of query arguments.
> 
> Consider a match pattern of http*://mydomain.com
> 
> Now, consider a URL of
>   http://victim.com/?some_query_parameter=://mydomain.com
> 
> Yes, some_query_parameter SHOULD be URL encoded as %3A%2F%2Ftest when sent
over
> the network. However, depending on how the caller handles/converts the URL
> string, they may have allowed attacker data through. Your implementation would
> allow it, seeing it as
> 
> http(stuff matching http://wildcard)://mydomain.com, reporting the failure of
> http://victim.com to http://mydomain.com.

Hmm.  I was going to say I don't want to repeatedly call .spec(), but it looks
like that's a fairly low-cost operation. Lemme do that.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:27: const char* kReporter = "chrome";
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> layering violation: This should not be hardcoded in //components.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:61: return;
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> size_t & npos (aka ((size_t)~0)) for a safer approach here

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:106:
DCHECK_GT(static_cast<int>(states_.size()), resource_index);
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> -_-
> 
> static_cast<int> of a size_t? Aww hell naw...

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.cc:110: std::copy(state.beacons.begin(),
state.beacons.end(), beacons_out->begin());
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> beacons_out->assign(state.becons.begin(), state.becons.end()) also works.
> Different tradeoffs, but probably fine for testing.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:9: #include <map>
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> IWYU: vector (line 48)
> DIWYDU: delete map - you don't use it.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:11: #include "base/memory/ref_counted.h"
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> IWYU: I don't see any refcounting going on here.

Whoops. This used to take a URLRequestContextGetter, I think, which was
refcounted.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:16: #include
"components/domain_reliability/config.h"
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Normally you'd be able to forward declare this, if ::Resource wasn't an inner
> class (see http://www.chromium.org/developers/coding-style/cpp-dos-and-donts )

Are you saying I should rearrange it so I can?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:34: MockableTime* time,
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> nit: Should "MockableTime" be called what it is, rather than how it can be
used
> - eg: TimeSource (which is what we've used in things like cc/ and the
> base/profiler code) or TimeProvider (used in base/ and extensions/)

Yeah, we might be able to come up with a nicer name for it. I'll add a TODO to
rename it.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:39: virtual ~DomainReliabilityContext();
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Why is this virtual, when you have no virtual methods and are not implementing
> any interfaces?

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:43: // the request counters in any case.
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Should this be called "RegisterBeacon" instead? Or something more
event-notify'y
> like "OnBeacon"?
> 
> The "add" in AddBeacon seems like it will take some concrete step with the
> Beacon being added to something, but that's not entirely the case. Yes, you
> increment the request counters, but you don't add the beacon to be uploaded,
> which is the first thing I think of with "AddBeacon"

Yeah, I've never been happy with this name. I think OnBeacon is better enough.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:50: int* failed_requests_out) const;
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> For "counts of things" and indices, use unsigned integer types
> 
> See http://www.chromium.org/developers/coding-style#TOC-Types

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:56: static const int kMaxQueuedBeacons;
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> For "counts of things", use unsigned integer types
> 
> See http://www.chromium.org/developers/coding-style#TOC-Types

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:60: class ResourceState {
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> Because you the ScopedVector<> isn't instantiated until the ctor/dtor (in the
> .cc), and you have copy and assignment disabled, you should be able to forward
> declare this entire class.
> 
> That would also allow you to forward declare DomainReliabilityBeacon, since
> std::deque<> is the only thing preventing it from being so.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:78: // Removes the oldest beacon.
DCHECKs if there isn't one.
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> nit: newline between 77/78.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:92: int uploading_failed_requests;
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> size_t for 85, 86, 91, 92

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:117: void RemoveOldestBeacon();
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> I find it weird to see 108-117 duplicated with effectively 68-79, for what
it's
> worth.

If you want, I can just have Context::MarkUpload modify the states directly?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:129: int uploading_beacon_count_;
On 2014/04/25 01:00:02, Ryan Sleevi wrote:
> size_t for counts

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/dispatcher.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/dispatcher.h:11: #include
"base/memory/scoped_vector.h"
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> IWYU: Unused

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/dispatcher.h:29:
DomainReliabilityDispatcher(MockableTime* time);
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> style: explicit

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/dispatcher.h:41: scoped_ptr<MockableTime::Timer>
timer_p,
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> nit: What's up with the naming here?

Sorry, that's an odd (and it turns out unnecessary) habit I picked up.

> nit: You can also keep this class forward declared and entirely in the impl

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/domain_reliability_export.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/domain_reliability_export.h:11: // Component build
on Windows
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> Naturally, you won't want to commit these comments :)

Done.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:50: };
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> Why have this at all? If you need a URLRequestContextGetter, why not simply
> *take* a URLRequestContextGetter?

See .h for explanation, although I'm willing to rework it if you'd prefer.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:138: : url(request.url()),
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> If request.url().is_valid() is false, you'll never log this, correct? So why
not
> filter these out in OnCompleted / OnBeforeRedirect?

I still need the URL to match against patterns in the context.

Also, can we have a request with an invalid URL? Wouldn't the URLRequest reject
it?

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:161: // Grab domain before we
config.Pass().
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> // Grab a copy of the domain before transferring ownership of |config|

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:206: beacon.server_ip =
request.socket_address.host();
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> SECURITY/PRIVACY: Is there any risk that this may disclose the users' proxy
IP?
> If so, bug :)

Looking into it.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:34: // |ChromeNetworkDelegate|, which
calls the On* methods.
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> You're a component now, you don't get to talk about ChromeNetworkDelegate
> (that's a layering violation).
> 
> Instead, describe this class, it's requirements (eg: that it live on the same
> thread as the net::URLRequestContext, which must outlive it), and how to use
it
> (eg: implement a net::NetworkDelegate that calls the On* methods as
appropriate)
> 

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:38: // I/O thread.
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> NB: Pronouns in comments considered harmful :)
> 
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
> 
> Rather than discuss this in the constructor, document it in the class design
> itself

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:45: // Adds the "baked-in"
configuration(s) for Google sites.
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> // Add in configurations that have been configured at compile-time.
> 
> Yes, as the reviewer who stamped this, I'm a horrible person :)

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:49: // events:
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> Components don't get to talk about profiles either (AIUI)
> 
> Ideally, however, you also wouldn't want to talk about NetworkDelegate - it's
> not an API contract of yours that users implement in terms of NetworkDelegate
-
> it just happens to be the most convenient way, and that has a 1:1 mapping.
> 
> Instead, describe *your* contract and expectations for these methods to be
> called. What are your exact pre-conditions/post-conditions?

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:65: RequestInfo(const net::URLRequest&
request);
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> style: explicit

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.h:86:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> You don't take one (because of threading), but you live on one? What's up with
> that?

URLFetcher requires a URLRequestContextGetter, since (IIRC) it can be managed on
any thread. I don't want to burden *my* callers with making one, so I take a
context directly, but I do need to create a trivial one (see .cc for details) to
pass to the URLFetcher.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.cc:40: return value;
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> Why Int? Why not UInt?

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.cc:182: for (unsigned i = 0; i <
collectors_.size(); ++i) {
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> s/unsigned/size_t/ - use the proper type.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.cc:187: min_index = i;
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> SECURITY BUG: You just sunk my battleship :(
> 
> You coerce an unsigned to a signed here. So now we have a chance of getting a
> negative number into *collector_index_out (for a large value of "i".
> 
> On line 99, you store this value into collector_index_.
> 
> If I call OnUploadComplete after this, you've now just walked backward in the
> stack. And broken my heart. And my security, because you write to it in (line
> 119/126)
> 
> This is why you always leave a note... or always use the right integer types
:)

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/scheduler.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.h:53: // ScheduleUploadCallback (if all
previous beacons have been uploaded).
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> Comment nit: Don't describe how this *is* used, describe what it does and how
to
> use it.
> 
> For example, my first reaction is "Called by who?"
> My next reaction is "Hrm, On* methods are usually used for delegate. Is this a
> delegate? No? So when do I call this?"

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/scheduler.h:57: void OnUploadStart(int*
collector_index_out);
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> BUG: You define |num_collectors| as a size_t, but then co-erce to an int (also
> line 78). Your types should match - and, since they're indices, they should be
> size_t

Done.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.cc:20: : public
DomainReliabilityUploader, net::URLFetcherDelegate {
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> "git cl format" should clean this up for you, as it has my style-sense
tingling.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.cc:23:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter)
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> const scoped_refptr<...>&

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.cc:68: delete callback_it->first;
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> You don't have to de-constify this? (See line 74)

Apparently not!  Deleting a const pointer seems to be valid.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
File components/domain_reliability/uploader.h (right):

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.h:22: class DOMAIN_RELIABILITY_EXPORT
DomainReliabilityUploader {
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> Document.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.h:31:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter);
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> const scoped_refptr<...>&
> 
> Avoids an extra refcount cycle, still has the same effect.

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader.h:33: virtual void UploadReport(const
std::string& report_json,
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> Document.

Done.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/uploader_unittest.cc:52: std::map<int, bool>
upload_successful_;
On 2014/04/26 02:37:39, Ryan Sleevi wrote:
> size_t

Done.

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

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/util.cc:91: } else {
On 2014/04/26 02:40:28, Ryan Sleevi wrote:
> Don't use "else" after a return -
> http://www.chromium.org/developers/coding-style#Code_Formatting

Done.

https://codereview.chromium.org/252613002/diff/1/components/domain_reliabilit...
components/domain_reliability/util.cc:92: for (size_t i = 0; i <
arraysize(net_error_map); i++) {
On 2014/04/26 02:40:28, Ryan Sleevi wrote:
> *shrug* - worth mentioning http://www.cplusplus.com/reference/cstdlib/bsearch/
?
> Seems like this is going to be performance critical code.

I'll add a TODO, in case it's too slow?

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

PTAL, rsleevi.

[Ryan Sleevi, 2014-04-30 23:58:11]

https://codereview.chromium.org/252613002/diff/20001/components/domain_reliab...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/252613002/diff/20001/components/domain_reliab...
components/domain_reliability/config.h:42: // it |success_sample_rate| or
|failure_sample_rate| to a random number.)
you accidentally a word (compares)

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/config.cc:27: static_cast<size_t>(~0);
is the static_cast needed in this context?

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/config_unittest.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/config_unittest.cc:170: 
what about query strings? And link-fragments? (#)

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/context.cc:50:
resource_value->SetInteger("failed_requests", failed_requests);
Doesn't this imply that successful/failed should be uint32, to be constrainted
to JS's size?

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/dispatcher.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/dispatcher.cc:83:
DomainReliabilityDispatcher::Task::~Task() {}
Seems like you should move 73-83 up to 28, to match ordering.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/dispatcher.cc:106: task));
DESIGN: Not something for now, but I'm wondering if there is a better way to
aggregate these timers. MessageLoop is fairly efficient in using a scan and mark
here, and it seems like as long as the timer was the lowest possible timeout,
you could implement it as such.

Then again, this seems like it's re-implementing a lot of the existing
MessageLoop task pump stuff. Why wouldn't PostDelayedTask suffice here?

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/scheduler.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/scheduler.cc:101: VLOG(1) << "Starting upload to
collector " << collector_index_ << ".";
DVLOG? VLOG2? Seems spammy

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/uploader.h (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/uploader.h:36: net::URLRequestContextGetter>&
url_request_context_getter);
git-cl format? This doesn't look like the wrapping it would do?

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/util.h (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/util.h:21: // string that should be recorded in a
beacon. Returns true if it could.
1) Better documentation needed, since it still tripped me up in this review that
this implemented the whitelist, and not just an 'accidental omission'
2) Per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...
, this shouldn't be a static method in a class. Probably just a free-standing fn
in the domain_reliability namespace

[Deprecated (see juliatuttle), 2014-05-02 19:23:40]

PTAL, sleevi.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/config.cc:27: static_cast<size_t>(~0);
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> is the static_cast needed in this context?

Done.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/config_unittest.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/config_unittest.cc:170: 
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> what about query strings? And link-fragments? (#)

We should be okay matching query strings, but I've added a test for link
fragments.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/context.cc:50:
resource_value->SetInteger("failed_requests", failed_requests);
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> Doesn't this imply that successful/failed should be uint32, to be constrainted
> to JS's size?

Done.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/dispatcher.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/dispatcher.cc:83:
DomainReliabilityDispatcher::Task::~Task() {}
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> Seems like you should move 73-83 up to 28, to match ordering.

Done.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/dispatcher.cc:106: task));
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> DESIGN: Not something for now, but I'm wondering if there is a better way to
> aggregate these timers. MessageLoop is fairly efficient in using a scan and
mark
> here, and it seems like as long as the timer was the lowest possible timeout,
> you could implement it as such.
> 
> Then again, this seems like it's re-implementing a lot of the existing
> MessageLoop task pump stuff. Why wouldn't PostDelayedTask suffice here?

I added two layers of indirection:
MockableTime: Lets me test time-based interactions in unittests.
Dispatcher: Lets me schedule tasks with a range of times instead of a single
time.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/scheduler.cc (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/scheduler.cc:101: VLOG(1) << "Starting upload to
collector " << collector_index_ << ".";
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> DVLOG? VLOG2? Seems spammy

Uploads aren't super common, so I'm leaving this. I have, however, pruned the
ones that happen for every beacon.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/uploader.h (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/uploader.h:36: net::URLRequestContextGetter>&
url_request_context_getter);
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> git-cl format? This doesn't look like the wrapping it would do?

I seem to remember running it once already, and being surprised at this myself.

I ran it again, and it didn't change it.

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
File components/domain_reliability/util.h (right):

https://codereview.chromium.org/252613002/diff/30001/components/domain_reliab...
components/domain_reliability/util.h:21: // string that should be recorded in a
beacon. Returns true if it could.
On 2014/04/30 23:58:11, Ryan Sleevi wrote:
> 1) Better documentation needed, since it still tripped me up in this review
that
> this implemented the whitelist, and not just an 'accidental omission'
> 2) Per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...
> , this shouldn't be a static method in a class. Probably just a free-standing
fn
> in the domain_reliability namespace

Done.

[Ryan Sleevi, 2014-05-03 00:21:36]

Phew. Almost!

I need to take one last full pass - I just did the interdiff.

https://codereview.chromium.org/252613002/diff/50001/components/domain_reliab...
File components/domain_reliability/config_unittest.cc (right):

https://codereview.chromium.org/252613002/diff/50001/components/domain_reliab...
components/domain_reliability/config_unittest.cc:150: EXPECT_EQ(0,
GetIndex(&*config, "http://example/"));
Um, what's up with all the &* instead of just config.get() ?

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

On 2014/05/03 00:21:36, Ryan Sleevi wrote:
> Phew. Almost!
> 
> I need to take one last full pass - I just did the interdiff.
> 
>
https://codereview.chromium.org/252613002/diff/50001/components/domain_reliab...
> File components/domain_reliability/config_unittest.cc (right):
> 
>
https://codereview.chromium.org/252613002/diff/50001/components/domain_reliab...
> components/domain_reliability/config_unittest.cc:150: EXPECT_EQ(0,
> GetIndex(&*config, "http://example/"));
> Um, what's up with all the &* instead of just config.get() ?

Less width, but it turns out the lines fit even without it, so I switched back
to config.get().

[Ryan Sleevi, 2014-05-05 19:16:03]

A few more nits - mostly asking for more documentation.

I'm a little concerned about the test coverage - a few remarks below - but that
can be addressed later (and perhaps by !me :P)

https://codereview.chromium.org/252613002/diff/70001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/252613002/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:89: const char*
kDomainReliabilityUploadReporterString = "chrome";
1) Add a comment explaining what this constant is
2) Move to line 56 (following the advice in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
)

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/beacon.cc (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/beacon.cc:24: net::ErrorToString(chrome_error));
is "custom_error" really the right name for this? Isn't this the internal name?

That is, it doesn't go out as part of the beacon (that goes out as 'status')

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/config.cc:26: const size_t
DomainReliabilityConfig::kInvalidResourceIndex = ~0;
I suck. You can actually use SIZE_MAX here.

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/config.cc:133: return static_cast<size_t>(~0);
return kInvalidResourceIndex;

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/context.cc:61: }
API defensiveness: DCHECK_EQ(0, ...) for each of these? Or some other means to
make sure that the following API calls are treated as invalid

MarkUpload
MarkUpload
CommitUpload

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/context.h:38: scoped_ptr<const
DomainReliabilityConfig> config);
Document - in particular, you'll want to make sure to document
|upload_reporter_string|.

Are you sure const char* is correct? You're now forcing all callers to use a
string whose lifetime is kept alive (or part of static storage, what you're
doing) - true for your use, but is that the API you want?

Seems like "const std::string&" may be better? or std::string as a member?

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/dispatcher.h (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/dispatcher.h:31: void ScheduleTask(const
base::Closure& task,
Document

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/dispatcher.h:42: void RunAndDeleteTask(Task*
task);
Documentation

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

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:169: 
There's no tests for OnBeforeRedirect here, which is part of monitor's API.

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

PTAL, sleevi.

https://codereview.chromium.org/252613002/diff/70001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/252613002/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:89: const char*
kDomainReliabilityUploadReporterString = "chrome";
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> 1) Add a comment explaining what this constant is
> 2) Move to line 56 (following the advice in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> )

Done.

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/beacon.cc (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/beacon.cc:24: net::ErrorToString(chrome_error));
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> is "custom_error" really the right name for this? Isn't this the internal
name?
> 
> That is, it doesn't go out as part of the beacon (that goes out as 'status')

Yes, this is the internal name; it's partially redundant, and there just in case
we end up mapping multiple errors to the same status code.

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/config.cc:26: const size_t
DomainReliabilityConfig::kInvalidResourceIndex = ~0;
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> I suck. You can actually use SIZE_MAX here.

Done.

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/config.cc:133: return static_cast<size_t>(~0);
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> return kInvalidResourceIndex;

Done.

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/context.cc:61: }
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> API defensiveness: DCHECK_EQ(0, ...) for each of these? Or some other means to
> make sure that the following API calls are treated as invalid
> 
> MarkUpload
> MarkUpload
> CommitUpload

Hah, tricked you. Mark, Mark, Commit is valid. Mark, Commit, Commit is invalid.
(There's nothing to undo if an upload doesn't complete. I can add a call just to
track the state if you want, like UnmarkUpload.)

(I'll add a comment :P)

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/context.h:38: scoped_ptr<const
DomainReliabilityConfig> config);
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> Document - in particular, you'll want to make sure to document
> |upload_reporter_string|.
> 
> Are you sure const char* is correct? You're now forcing all callers to use a
> string whose lifetime is kept alive (or part of static storage, what you're
> doing) - true for your use, but is that the API you want?
> 
> Seems like "const std::string&" may be better? or std::string as a member?

const std::string& here, and I'll keep a std::string in the monitor.

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
File components/domain_reliability/dispatcher.h (right):

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/dispatcher.h:31: void ScheduleTask(const
base::Closure& task,
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/dispatcher.h:42: void RunAndDeleteTask(Task*
task);
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> Documentation

Done.

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

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:169: 
On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> There's no tests for OnBeforeRedirect here, which is part of monitor's API.

There aren't any for OnComplete, either! I was trying to dodge mocking out an
entire URLRequest, since it is a royal pain.

Can you think of an easier way to do that than doing the URL request filter
thing to mock out the redirects and requests?

[Ryan Sleevi, 2014-05-06 19:06:24]

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

https://codereview.chromium.org/252613002/diff/70001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:169: 
On 2014/05/06 18:52:03, ttuttle wrote:
> On 2014/05/05 19:16:03, Ryan Sleevi wrote:
> > There's no tests for OnBeforeRedirect here, which is part of monitor's API.
> 
> There aren't any for OnComplete, either! I was trying to dodge mocking out an
> entire URLRequest, since it is a royal pain.
> 
> Can you think of an easier way to do that than doing the URL request filter
> thing to mock out the redirects and requests?

Not off the top of my head.

However, what you're testing now isn't really a "good test" - that is, you're
not really testing the methods/behaviour of your class in isolation, but instead
testing it in concert with some external thing.

I definitely want to see defined behaviour for how redirects are handled and
what's the expected result. I honestly don't know what to expect, and that's the
best time to add tests (and documentation, but tests are the best kind of
documentation, since they can't lie :D)

I don't think it's essential for this CL.

[Ryan Sleevi, 2014-05-06 22:50:40]

LGTM with nits

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
File components/domain_reliability/OWNERS (right):

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
components/domain_reliability/OWNERS:3: szym@chromium.org
remove szym at some point?

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
components/domain_reliability/config.cc:7: #include <limits.h>
it's stdint.h, not limits.h, that defines SIZE_MAX (alternatively, <cstdint>)

18.4.1 of C++11, or 7.18.2 of C99

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
File components/domain_reliability/dispatcher.h (right):

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
components/domain_reliability/dispatcher.h:32: // now.
Is it a random delay, or is it just a weak guarantee that the task will not be
executed before min_delay, and will be executed before max_delay?

[Deprecated (see juliatuttle), 2014-05-07 02:36:07]

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
File components/domain_reliability/OWNERS (right):

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
components/domain_reliability/OWNERS:3: szym@chromium.org
On 2014/05/06 22:50:41, Ryan Sleevi wrote:
> remove szym at some point?

If by some stroke of luck I manage to get szym to continue to review my code, I
will not be disappointed in the least.

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
components/domain_reliability/config.cc:7: #include <limits.h>
On 2014/05/06 22:50:41, Ryan Sleevi wrote:
> it's stdint.h, not limits.h, that defines SIZE_MAX (alternatively, <cstdint>)
> 
> 18.4.1 of C++11, or 7.18.2 of C99

Done.

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
File components/domain_reliability/dispatcher.h (right):

https://codereview.chromium.org/252613002/diff/90002/components/domain_reliab...
components/domain_reliability/dispatcher.h:32: // now.
On 2014/05/06 22:50:41, Ryan Sleevi wrote:
> Is it a random delay, or is it just a weak guarantee that the task will not be
> executed before min_delay, and will be executed before max_delay?

Weak guarantee. I've updated the comment.

[Deprecated (see juliatuttle), 2014-05-07 17:15:25]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-05-07 17:19:37]

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

[DaveMoore, 2014-05-07 17:46:13]

profiles lgtm

[commit-bot: I haz the power, 2014-05-07 21:41:50]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 03:03:01]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 03:37:34]

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

[commit-bot: I haz the power, 2014-05-08 03:37:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Deprecated (see juliatuttle), 2014-05-08 15:57:10]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-05-08 16:00:51]

The CQ bit was unchecked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-05-08 17:39:30]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-05-08 17:46:07]

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

[Deprecated (see juliatuttle), 2014-05-08 21:27:39]

The CQ bit was unchecked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-05-08 21:27:50]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-05-08 21:30:27]

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

[Deprecated (see juliatuttle), 2014-05-09 12:09:59]

The CQ bit was unchecked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-05-09 12:10:21]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-05-09 12:11:47]

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

[commit-bot: I haz the power, 2014-05-09 22:18:52]

Change committed as 269434