Initial implementation for CertNetFetcher.

Initial implementation for CertNetFetcher.

CertNetFetcher is a class for issuing and cancelling network fetches, that will be used by CertVerifier. It fetches http:// AIA and CRL URLs.

This initial implementation has some remaining TODOs around:
  - Add POST parameters
  - Add cache bypass controls
  - Add maximum requests thresholds
  - Add more tests for cancellation/de-duplication

BUG=455366
NOPRESUBMIT=true

Committed: https://crrev.com/0fad62b76d552f09f8a1c90edce26fe64df9a7f9
Cr-Commit-Position: refs/heads/master@{#324261}

[eroman, 2015-02-09 18:12:27]

eroman@chromium.org changed reviewers:
+ rsleevi@chromium.org

[eroman, 2015-02-09 21:17:57]

(Adding in some other potential reviewers, to try and lighten Sleevi's load)

[mattm, 2015-02-10 05:22:13]

mattm@chromium.org changed reviewers:
+ mattm@chromium.org

[mattm, 2015-02-10 05:22:14]

Just a few comments from first look over. Nothing major jumped out, but I'll try
to take a deeper look tomorrow.

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:277: // In practice all URLs fetched are HTTPS, but
check anyway as defensive
HTTP?

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetcher.h
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:63: class NET_EXPORT CertNetFetcher {
Maybe inherit NonThreadSafe and add CalledOnValidThread dchecks?

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher_unittest.cc:93: base::MessageLoop::current()->Run();
use base::RunLoop?

[eroman, 2015-02-11 03:51:52]

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:277: // In practice all URLs fetched are HTTPS, but
check anyway as defensive
On 2015/02/10 05:22:14, mattm wrote:
> HTTP?

Done. Indeed, I managed to write the opposite of what I meant :)

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetcher.h
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:63: class NET_EXPORT CertNetFetcher {
On 2015/02/10 05:22:14, mattm wrote:
> Maybe inherit NonThreadSafe and add CalledOnValidThread dchecks?

Done.

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher_unittest.cc:93: base::MessageLoop::current()->Run();
On 2015/02/10 05:22:14, mattm wrote:
> use base::RunLoop?

I had copied this code from TestCompletionCallback. Will look into RunLoop() and
get back to you on this.

[Ryan Sleevi, 2015-02-11 08:41:54]

I'm still working through the unit tests...

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:115: const RequestParams& request_params() const {
return *request_params_.get(); }
No need for .get(), just *request_params_ will do. (operator* overload)

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:119: RequestId AddRequest(FetchCallback callback);
Pass by const-ref

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:173: std::vector<uint8_t> response_body_;
Why not use a GrowableIOBuffer? Saves you on line 177 as well.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:182: CertNetFetcher* parent_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:206: // since that may delete |this|.
Why is this necessary? |it| is already invalidated by line 211 (as are all other
iterators in |requests_|

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:217: if (requests_.empty() && parent_) {
When would |parent_| be NULL? Doesn't that indicate programmer error?

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:231: base::MessageLoop::current()->PostTask(
base::ThreadTaskRunnerHandle::Get()->PostTask

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:270: DCHECK_EQ(url_request_.get(), request);
Hopefully we can fix this with https://codereview.chromium.org/837873002/ , in
which case, the .get() would not be necessary.

But if wishes were horses...

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:279: DCHECK(request->url().SchemeIsHTTPOrHTTPS());
Why this, instead of re-using your helper method?

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:281: if (http_response_code != 200 &&
request->url().SchemeIsHTTPOrHTTPS()) {
BUG? What's up with this &&?

Does that mean if I somehow slip LDAP in here, and it fails with != 200, then
we'll call ReadBody?

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:327: request_params_->max_response_size_in_bytes) {
something something overflow

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:371: //     not iterated through.
Am I reading this right that if new jobs are attached following a failing job,
they'll be immediately failed?

Does this create an API usability concern where, as a response to a job failing,
a new request is started, except it's just attached to this existing request and
failed?

I believe we handle this elsewhere (CertVerifier, HostResolver) by having it
indirect first through the parent (to remove the job from the pending list, so
that any new requests create/are joined to a new job) and then end up here in
the Job.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:397: for (JobSet::iterator it = jobs_.begin(); it
!= jobs_.end(); ++it) {
for (auto* job : jobs) {
  job->Cancel();
}

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:448: jobs_.erase(job);
performance nit: Even though the .find() call is optimized out in a non-DCHECK
build, the .erase(job) method will still effectively invoke a find.

Because of this, you can optimize and avoid a double .find() for DCHECK builds,
without trading any of the performance for release builds, by:

auto it = jobs_.find(job);
DCHECK(it != jobs_.end());
jobs_.erase(it);

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetcher.h
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:65: : NON_EXPORTED_BASE(public base::NonThreadSafe)
{
I'm not a fan of inheriting from base::NonThreadSafe, in that it's not a
pure-virtual base class

Have you considered using a base::ThreadChecker_ instead?

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:67: struct Request;
Is it truly necessary for this to appear first? Though line 98 exists, I thought
that you could use the naming scope rules of classes to allow out-of-order
declaration (i.e. moving this to line 122 "should" still work)

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:83: struct RequestParams {
I just dinged another reviewer for using a struct to smuggle params and
potentially create overloaded methods and default arguments.

A couple issues with this design approach:
1) By having this struct public, it's part of your API for others to use as
well. To me, that does not logically fit with how it is used (as a set of
arguments for Fetch)
2) You've now made Fetch() have four possible overloads (at least)
- (url, http_method) specified, (max_response, timeout) as default values
- (url, http_method, max_response) specified, timeout as default values
- (url, http_method, timeout) specified, max_response as default values
- (url, http_method, max_response, timeout) specified, no default values

When reading code, I can no longer reason about which of these four methods is
'effectively' invoked. If refactoring Fetch(), I can no longer reason about
which arguments are required and which are optional.

3) By having operator<() be a member, rather than a comparator helper, you're
encouraging others further to store these values. Indeed, strictly speaking from
how your API is used, there's no need to expose the operator<() at all


Suggestion: If you wish to allow response_size and timeout to be optional, but
still have semi-sane defaults, then perhaps declaring public constants for these
(CertNetFetcher::kDefaultMaxResponseSize) and making Fetch() explicitly require
all four parameters be present can resolve this ambiguity.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:113: RequestId Fetch(scoped_ptr<RequestParams>
request_params,
See comments re: implicit overloads being created here.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:114: FetchCallback callback);
Pass by const-ref

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:134: Job* FindJob(const RequestParams& params);
DANGER: This API pattern generally results in "doom", in my experience, since
the result may be invalidated through some nested call.

Recommendation: return a scoped_refptr<Job>, to ensure that the returned Job
remains valid for the execution of the FullStatement.

[eroman, 2015-02-11 20:15:16]

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:371: //     not iterated through.
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Am I reading this right that if new jobs are attached following a failing job,
> they'll be immediately failed?
> 
> Does this create an API usability concern where, as a response to a job
failing,
> a new request is started, except it's just attached to this existing request
and
> failed?
> 
> I believe we handle this elsewhere (CertVerifier, HostResolver) by having it
> indirect first through the parent (to remove the job from the pending list, so
> that any new requests create/are joined to a new job) and then end up here in
> the Job.

Yes your reading is correct.

I had done it this way to easily support the "delete CertNetFetcher withing a
callback case".

However I agree with you that on error it would be more useful to re-issue the
request, and that pattern is more useful for building re-try logic.

I will change it to match your expectations. Will probably add some concept of a
"job currently being completed" to the CertNetFetcher so the deletion case is
still supported.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetcher.h
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:83: struct RequestParams {
On 2015/02/11 08:41:54, Ryan Sleevi wrote:
> I just dinged another reviewer for using a struct to smuggle params and
> potentially create overloaded methods and default arguments.
> 
> A couple issues with this design approach:
> 1) By having this struct public, it's part of your API for others to use as
> well. To me, that does not logically fit with how it is used (as a set of
> arguments for Fetch)
> 2) You've now made Fetch() have four possible overloads (at least)
> - (url, http_method) specified, (max_response, timeout) as default values
> - (url, http_method, max_response) specified, timeout as default values
> - (url, http_method, timeout) specified, max_response as default values
> - (url, http_method, max_response, timeout) specified, no default values
> 
> When reading code, I can no longer reason about which of these four methods is
> 'effectively' invoked. If refactoring Fetch(), I can no longer reason about
> which arguments are required and which are optional.
> 
> 3) By having operator<() be a member, rather than a comparator helper, you're
> encouraging others further to store these values. Indeed, strictly speaking
from
> how your API is used, there's no need to expose the operator<() at all
> 
> 
> Suggestion: If you wish to allow response_size and timeout to be optional, but
> still have semi-sane defaults, then perhaps declaring public constants for
these
> (CertNetFetcher::kDefaultMaxResponseSize) and making Fetch() explicitly
require
> all four parameters be present can resolve this ambiguity.

I will switch this over to using explicit methods, and keep the RequestParams
internal.

For the record, some properties of the RequestParams approach that I like:
  - Adding POST parameters can be expressed as a method on RequestParams
  - Fewer copying of parameters
  - Ease of testing by being able to override timeouts and max sizes without
needing to introduce test-only mechanisms, or require callers to always specify
them.

[eroman, 2015-02-11 23:18:08]

@rsleevi: If you haven't started reviewing unit-tests yet I can apply some
further cleanups to them and/or merge my other unit-test change into this one.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:115: const RequestParams& request_params() const {
return *request_params_.get(); }
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> No need for .get(), just *request_params_ will do. (operator* overload)

Done.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:119: RequestId AddRequest(FetchCallback callback);
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Pass by const-ref

Done.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:173: std::vector<uint8_t> response_body_;
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Why not use a GrowableIOBuffer? Saves you on line 177 as well.

GrowableIOBuffer is more complicated to use. You need to increase capacity
manually, track its size, and then you end up with a pointer and length to pass
to consumers.

No thanks, I will stick with std::vector<>.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:182: CertNetFetcher* parent_;
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:206: // since that may delete |this|.
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Why is this necessary? |it| is already invalidated by line 211 (as are all
other
> iterators in |requests_|

I have re-worked this so deletion is guaranteed to happen afterwards.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:217: if (requests_.empty() && parent_) {
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> When would |parent_| be NULL? Doesn't that indicate programmer error?

This is a left-over from when I removing jobs from the in-progress set before
invoking callbacks.

That said, I have now restored that behavior so it is relevant again.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:231: base::MessageLoop::current()->PostTask(
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> base::ThreadTaskRunnerHandle::Get()->PostTask

No longer relevant, using a OneShotTimer instead.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:270: DCHECK_EQ(url_request_.get(), request);
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Hopefully we can fix this with https://codereview.chromium.org/837873002/ , in
> which case, the .get() would not be necessary.
> 
> But if wishes were horses...

Acknowledged.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:279: DCHECK(request->url().SchemeIsHTTPOrHTTPS());
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Why this, instead of re-using your helper method?

Removed.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:281: if (http_response_code != 200 &&
request->url().SchemeIsHTTPOrHTTPS()) {
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> BUG? What's up with this &&?
> 
> Does that mean if I somehow slip LDAP in here, and it fails with != 200, then
> we'll call ReadBody?

Removed in favor of unconditionally checking response_code.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:327: request_params_->max_response_size_in_bytes) {
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> something something overflow

Overflow was possible if the consumer of CertNetFetcher were to set the size
limit for the request to a value larger than:

 std::numeric_limits<size_t>::max() - 4096

Which practically speaking is likely to OOM or fail before ever overflowing.

That said I appreciate you pointing out an integer overflow!

In the new patchset I believe overflow is no longer possible as the input is
being given as a signed "int". However I have added some safety checks as well
so there isn't a reliance on relative sizes of size_t <--> int.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:371: //     not iterated through.
On 2015/02/11 20:15:16, eroman wrote:
> On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> > Am I reading this right that if new jobs are attached following a failing
job,
> > they'll be immediately failed?
> > 
> > Does this create an API usability concern where, as a response to a job
> failing,
> > a new request is started, except it's just attached to this existing request
> and
> > failed?
> > 
> > I believe we handle this elsewhere (CertVerifier, HostResolver) by having it
> > indirect first through the parent (to remove the job from the pending list,
so
> > that any new requests create/are joined to a new job) and then end up here
in
> > the Job.
> 
> Yes your reading is correct.
> 
> I had done it this way to easily support the "delete CertNetFetcher withing a
> callback case".
> 
> However I agree with you that on error it would be more useful to re-issue the
> request, and that pattern is more useful for building re-try logic.
> 
> I will change it to match your expectations. Will probably add some concept of
a
> "job currently being completed" to the CertNetFetcher so the deletion case is
> still supported.

This is now addressed in the new patchset. I also simplified Job to not be
refcounted.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:397: for (JobSet::iterator it = jobs_.begin(); it
!= jobs_.end(); ++it) {
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> for (auto* job : jobs) {
>   job->Cancel();
> }

I have deleted this code in favor of calling STLDeleteElements(), as Job is no
longer refcounted.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.cc:448: jobs_.erase(job);
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> performance nit: Even though the .find() call is optimized out in a non-DCHECK
> build, the .erase(job) method will still effectively invoke a find.
> 
> Because of this, you can optimize and avoid a double .find() for DCHECK
builds,
> without trading any of the performance for release builds, by:
> 
> auto it = jobs_.find(job);
> DCHECK(it != jobs_.end());
> jobs_.erase(it);

I changed to using the return value from erase() and testing it it was 1.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetcher.h
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:65: : NON_EXPORTED_BASE(public base::NonThreadSafe)
{
On 2015/02/11 08:41:54, Ryan Sleevi wrote:
> I'm not a fan of inheriting from base::NonThreadSafe, in that it's not a
> pure-virtual base class
> 
> Have you considered using a base::ThreadChecker_ instead?

NonThreadSafe was based on a recommendation by Matt. I can try out ThreadChecker
too, will get back on this.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:67: struct Request;
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Is it truly necessary for this to appear first? Though line 98 exists, I
thought
> that you could use the naming scope rules of classes to allow out-of-order
> declaration (i.e. moving this to line 122 "should" still work)

I get a compile error when moving it down.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:83: struct RequestParams {
On 2015/02/11 20:15:16, eroman wrote:
> On 2015/02/11 08:41:54, Ryan Sleevi wrote:
> > I just dinged another reviewer for using a struct to smuggle params and
> > potentially create overloaded methods and default arguments.
> > 
> > A couple issues with this design approach:
> > 1) By having this struct public, it's part of your API for others to use as
> > well. To me, that does not logically fit with how it is used (as a set of
> > arguments for Fetch)
> > 2) You've now made Fetch() have four possible overloads (at least)
> > - (url, http_method) specified, (max_response, timeout) as default values
> > - (url, http_method, max_response) specified, timeout as default values
> > - (url, http_method, timeout) specified, max_response as default values
> > - (url, http_method, max_response, timeout) specified, no default values
> > 
> > When reading code, I can no longer reason about which of these four methods
is
> > 'effectively' invoked. If refactoring Fetch(), I can no longer reason about
> > which arguments are required and which are optional.
> > 
> > 3) By having operator<() be a member, rather than a comparator helper,
you're
> > encouraging others further to store these values. Indeed, strictly speaking
> from
> > how your API is used, there's no need to expose the operator<() at all
> > 
> > 
> > Suggestion: If you wish to allow response_size and timeout to be optional,
but
> > still have semi-sane defaults, then perhaps declaring public constants for
> these
> > (CertNetFetcher::kDefaultMaxResponseSize) and making Fetch() explicitly
> require
> > all four parameters be present can resolve this ambiguity.
> 
> I will switch this over to using explicit methods, and keep the RequestParams
> internal.
> 
> For the record, some properties of the RequestParams approach that I like:
>   - Adding POST parameters can be expressed as a method on RequestParams
>   - Fewer copying of parameters
>   - Ease of testing by being able to override timeouts and max sizes without
> needing to introduce test-only mechanisms, or require callers to always
specify
> them.

Done. In the latest patchset there are separate methods for fetching, and
RequestParams is an internal detail.

I agree this is a cleaner design.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:113: RequestId Fetch(scoped_ptr<RequestParams>
request_params,
On 2015/02/11 08:41:54, Ryan Sleevi wrote:
> See comments re: implicit overloads being created here.

Acknowledged.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:114: FetchCallback callback);
On 2015/02/11 08:41:53, Ryan Sleevi wrote:
> Pass by const-ref

Done.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:134: Job* FindJob(const RequestParams& params);
On 2015/02/11 08:41:54, Ryan Sleevi wrote:
> DANGER: This API pattern generally results in "doom", in my experience, since
> the result may be invalidated through some nested call.
> 
> Recommendation: return a scoped_refptr<Job>, to ensure that the returned Job
> remains valid for the execution of the FullStatement.

No longer relevant as Job is not refcounted.

[eroman, 2015-02-13 22:11:38]

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
File net/cert/cert_net_fetcher_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/20001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher_unittest.cc:93: base::MessageLoop::current()->Run();
On 2015/02/11 03:51:52, eroman wrote:
> On 2015/02/10 05:22:14, mattm wrote:
> > use base::RunLoop?
> 
> I had copied this code from TestCompletionCallback. Will look into RunLoop()
and
> get back to you on this.

Done.

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetcher.h
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/40001/net/cert/cert_net_fetche...
net/cert/cert_net_fetcher.h:65: : NON_EXPORTED_BASE(public base::NonThreadSafe)
{
On 2015/02/11 23:18:08, eroman wrote:
> On 2015/02/11 08:41:54, Ryan Sleevi wrote:
> > I'm not a fan of inheriting from base::NonThreadSafe, in that it's not a
> > pure-virtual base class
> > 
> > Have you considered using a base::ThreadChecker_ instead?
> 
> NonThreadSafe was based on a recommendation by Matt. I can try out
ThreadChecker
> too, will get back on this.

Done -- switched to ThreadChecker.

[eroman, 2015-02-19 19:45:45]

ping

[Ryan Sleevi, 2015-02-23 20:25:23]

Sincerest apologies for the delay, and for the layering violation that I missed.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:36: const int kNetErrorNot200HttpResponse =
ERR_FAILED;
DESIGN: Given that net::Error is an enum, I would suggest you use the more
explicit type, and then let integer coercion happen at the callsites.

But this is minor quibbling.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:75: result -= kReadBufferSizeInBytes;
This is non-obvious to me how you ensure it's not negative.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:97: DISALLOW_COPY_AND_ASSIGN(Request);
STYLE: Line 96 should specify private: , as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:114: DISALLOW_COPY_AND_ASSIGN(RequestParams);
ditto private

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:151: // Removes |request| from the job and deletes
it.
The "deletes it" is unclear, given that RequestId is presented as an opaque
type.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:285: int error =
CanFetchUrl(redirect_info.new_url);
My remarks on HSTS from the header notwithstanding, this means we'll block AIA
fetches for any HSTS-configured sites.

I think that's fine, but I'm nervous of the implications (e.g. if google.com set
HSTS with includeSubDomains, it'd block pki.google.com)

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:355: response_body_.insert(response_body_.end(),
read_buffer_->data(),
Recommendation: .reserve() first.

[Why? Long story about how .insert() is implemented in different STLs, but a
.reserve() will guarantee the most efficient across implementation strategies]

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:407: currently_completing_job_->Cancel();
I find this a little surprising, or at least, unclear from the header (perhaps I
missed it):

- CertNetFetcher::Job::OnJobCompleted() runs
 - Sets |currently_completing_job_|
 - begins running request callbacks
  - Request callback deletes the CertNetFetcher
   - Calls CertNetFetcher::Job::Cancel()
    - Cancel() delete's all the callbacks then calls Stop()
- OnJobCompleted() now doesn't execute any further request callbacks

I think this is a fine behaviour (and a perfectly reasonable one), but I didn't
see it spelled out.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:476: Job* job = FindJob(*request_params.get());
No need for .get() here

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:14: #include "base/time/time.h"
This appears to be unused now?

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:20: class URLRequestContext;
Hrm. From the perspective of //net/cert, this is a layering violation

That is, //net/cert is used by //net/socket which is used by //net/url_request,
so this is a circular dependency.

For NSS, we have explicitly setup a top level directory in //net, //net/ocsp,
that might otherwise be called //net/cert_fetching, that exists to resolve this
conflict.

It implements an interface specified by NSS (conceptually, a //net/cert
interface) and is injected down.

This suggests to me that we need to pursue an Interface/Impl split here in
//net/cert - //net/cert defines a pure virtual interface, while //net/ocsp
(probably renamed) implements it using //net/url_request

I apologize for not having thought about this sooner.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:28: //   * Restricts URLs to http://
A thought occurred to me - how will we handle HSTS? We can't disable it (and
that's a good thing, generally, from an API perspective), and we'll
automatically inherit any HSTS settings.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:124: typedef std::set<Job*, JobComparator> JobSet;
Per https://chromium-cpp.appspot.com/ , prefer "using ="

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher_unittest.cc:394: // NOTE: The python test server is
single threaded and can service. After a
incomplete sentence?

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher_unittest.cc:447: 
Can you add a test for:

Two requests for same URL
 - thus same job
Wait for callback of first request
delete the fetcher
verify the second request callback is not called

[eroman, 2015-02-23 23:36:58]

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:36: const int kNetErrorNot200HttpResponse =
ERR_FAILED;
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> DESIGN: Given that net::Error is an enum, I would suggest you use the more
> explicit type, and then let integer coercion happen at the callsites.
> 
> But this is minor quibbling.

Done.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:75: result -= kReadBufferSizeInBytes;
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> This is non-obvious to me how you ensure it's not negative.

The first line:
  base::CheckedNumeric<size_t> result(max_response_bytes);
Ensures that max_response_bytes is not negative.

The second line:
  result += kReadBufferSizeInBytes;
Ensures that it does not overflow when adding kReadBufferSizeInBytes

(Well actually it marks the CheckedNumeric as IsInvalid(), but you get the
point).

CheckedNumeric is a bit tricky in that it has implicit constructors for integer
types. However you need to be careful not to do any actual arithmetic in the
constructor or you silently overflow then and there.

Not sure if this helps but I have removed the last -= and changed from CHECK -->
DCHECK.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:97: DISALLOW_COPY_AND_ASSIGN(Request);
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> STYLE: Line 96 should specify private: , as per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:114: DISALLOW_COPY_AND_ASSIGN(RequestParams);
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> ditto private

Done.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:151: // Removes |request| from the job and deletes
it.
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> The "deletes it" is unclear, given that RequestId is presented as an opaque
> type.

My thinking is that the Request object is an internal detail owned by the Job.
The caller in this case just cancels, and the fact that the Request got deleted
(or that the RequestId is that same pointer) is not a detail that needs to be
exposed.

Do you have a suggestion on how to improve the wording here?

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:285: int error =
CanFetchUrl(redirect_info.new_url);
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> My remarks on HSTS from the header notwithstanding, this means we'll block AIA
> fetches for any HSTS-configured sites.
> 
> I think that's fine, but I'm nervous of the implications (e.g. if
http://google.com set
> HSTS with includeSubDomains, it'd block http://pki.google.com)

I would venture to say that this is a bad configuration.
And what we could do instead is surface it as its own error ("failed because of
HSTS redirect to HTTPS") to raise awareness to this problem (a blanket policy on
*.google.com seems like a plausible thing for people to do).

Or alternately we could skip HSTS redirects for AIA, but that strikes me as
problematic in different ways.

What do you think?

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:355: response_body_.insert(response_body_.end(),
read_buffer_->data(),
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> Recommendation: .reserve() first.
> 
> [Why? Long story about how .insert() is implemented in different STLs, but a
> .reserve() will guarantee the most efficient across implementation strategies]

Done.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:407: currently_completing_job_->Cancel();
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> I find this a little surprising, or at least, unclear from the header (perhaps
I
> missed it):
> 
> - CertNetFetcher::Job::OnJobCompleted() runs
>  - Sets |currently_completing_job_|
>  - begins running request callbacks
>   - Request callback deletes the CertNetFetcher
>    - Calls CertNetFetcher::Job::Cancel()
>     - Cancel() delete's all the callbacks then calls Stop()
> - OnJobCompleted() now doesn't execute any further request callbacks
> 
> I think this is a fine behaviour (and a perfectly reasonable one), but I
didn't
> see it spelled out.

Correct. With the additional detail that once OnJobCompleted() returns it will
delete the job (whereas the other jobs get deleted by ~CertNetFetcher()).

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:476: Job* job = FindJob(*request_params.get());
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> No need for .get() here

Done.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:14: #include "base/time/time.h"
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> This appears to be unused now?

Done.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:20: class URLRequestContext;
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> Hrm. From the perspective of //net/cert, this is a layering violation
> 
> That is, //net/cert is used by //net/socket which is used by
//net/url_request,
> so this is a circular dependency.
> 
> For NSS, we have explicitly setup a top level directory in //net, //net/ocsp,
> that might otherwise be called //net/cert_fetching, that exists to resolve
this
> conflict.
> 
> It implements an interface specified by NSS (conceptually, a //net/cert
> interface) and is injected down.
> 
> This suggests to me that we need to pursue an Interface/Impl split here in
> //net/cert - //net/cert defines a pure virtual interface, while //net/ocsp
> (probably renamed) implements it using //net/url_request
> 
> I apologize for not having thought about this sooner.

I can certainly split up as Interface/Impl if needed, however I would like to
better understand what your concern is with using forward declarations.

//net is compiled as a whole and contains plenty of other circular dependencies.
For instance NetworkDelegate. Or in the same vein as the one you mentioned,
URLRequestContext contains a ProxyService which contains a ProxyScriptFetcher
which contains a URLRequest/URLRequestContext.

If we were to compile //net/cert in isolation then we would have an issue. It
doesn't seem that we currently do, but I imagine that is a future goal.

I am fine with making the change, just want to understand exactly which problem
I am fixing.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:28: //   * Restricts URLs to http://
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> A thought occurred to me - how will we handle HSTS? We can't disable it (and
> that's a good thing, generally, from an API perspective), and we'll
> automatically inherit any HSTS settings.

Good point I hadn't considered HSTS. I responded on the other comment, perhaps
we can surface it as its own error rather than try to make it work
automagically.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:124: typedef std::set<Job*, JobComparator> JobSet;
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> Per https://chromium-cpp.appspot.com/ , prefer "using ="

Done. Changed all the typedefs to using=

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher_unittest.cc:394: // NOTE: The python test server is
single threaded and can service. After a
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> incomplete sentence?

Fixed.

https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher_unittest.cc:447: 
On 2015/02/23 20:25:22, Ryan Sleevi wrote:
> Can you add a test for:
> 
> Two requests for same URL
>  - thus same job
> Wait for callback of first request
> delete the fetcher
> verify the second request callback is not called

I had sent these tests as a follow-up change
(https://codereview.chromium.org/906393002)

I have now merged those tests into this changelist so it does not appear
untested.

There are still a few TODOs I have left (with regards to cookies) which I can
address after the first landing.

[eroman, 2015-03-24 21:39:57]

Ping!

Ryan, do you want me to change the reviewer to David or Matt?

[Ryan Sleevi, 2015-03-25 06:16:36]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-03-25 06:16:38]

Yeah, going to punt to davidben@ to do the majority of this.

My biggest concern remains (and it remains because it took me a month to answer
your question, because I'm a bad reviewer; please, ping daily, force my email
queue to overflow) the dependency on URLRequest here.

The push for layering, throughout //chrome, //content, and //components has been
to be very careful about making directories depend on eachother. Part of //net's
complexity comes from it's cyclic nature in some areas.

I think an apt comparison has been how rdsmith@ (now ellyjones@) was/is
implementing SDCH. SDCH is needed to support URL loading, but at the same time,
SDCH needs to perform URL loading (for fetching the dictionaries). Rather than
create a circular dependency between those two components, SDCH defines a base
interface for fetching resources, which then has a separate concrete
implementation elsewhere (tbh, I'm not sure where "elsewhere" is) to break that.

I'm not sure how best to articulate these concerns, other than two modules
depending on eachother is just one of those smells that I can't quite
articulate. Even though they're both in //net, this creates a subtle circle, and
those are bad.

Lakos' "Large Scale C++ Design" touches on this a lot more, as has Will (in his
various rants - one of them being
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Oy3uuiTSwDk/5UDyR...
although that doesn't quite capture this situation, it describes his and my
general unease about weird dep graphs). 

Design wise, I see Chromium using an extreme amount of SOLID design, and the
Dependency Inversion Principle (
http://en.wikipedia.org/wiki/Dependency_inversion_principle ) is about ensuring
the acyclic dependency principle (
http://en.wikipedia.org/wiki/Acyclic_dependencies_principle )

If you get further delayed / design consultation, mmenke is probably good to
chime in if you can get some time slices from him.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:134: return false;
Normally when writing conditional comparators, I like to collapse lines 132-134
to just return 133

If there an intentional reason you avoided that (e.g. to make it easier to add
new fields)?

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:231: parent_ = NULL;
nullptr

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:247: DCHECK(it != requests_.end());
Should this be a CHECK() instead?

Just 248 makes me extremely nervous for subtle and hard to detect memory
corruption; might as well blow things up quick.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:270: this, NULL);
nullptr

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:401: parent_->SetCurrentlyCompletingJob(NULL);
nullptr

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:405: : currently_completing_job_(NULL),
context_(context) {
nullptr

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:505: if (it != jobs_.end() && !(params <
(*it)->request_params()))
Is this second conditional meant to be an equality check, or a sanity check? If
a equality check, you should check both that

!(a < b) && !(b < a)

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:507: return NULL;
nullptr

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:16: #include "url/gurl.h"
Can forward declare

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:73: static const int DEFAULT = -1;
So this syntax is super sketch (there's some old chromium-dev threads that
explain why; TL;DR - static storage needs to be filled in the .cc for MSVC, but
that can make gcc/clang grumpy)

You could use an enum of int type here, since the enum will promote.

[eroman, 2015-03-25 18:30:24]

I will split off a new subdirectory, "cert_fetcher/" and then put the
implementation in there, and an equivalent interface into "cert/".

But TBH I remain unconvinced that this improves the total complexity. We still
have a circular dependency between URLRequest -> CertVerifier -> CertNetFetcher
-> URLRequest when piecing the blocks together, so the root problem remains. The
main difference is if in the future we wanted to have a separable //net/cert
target, then consumers would have the flexibility of choosing cert fetching
without networking capabilities, or additionally add a dependency on
//net/cert_fetcher to pass in a CertNetFetcherImpl dependency to CertVerifier

[eroman, 2015-03-25 18:36:25]

Or a different organization to consider would be to pull //net/cert out of //net
alltogether, and move it into say //crypto/cert

Then we would have //net depending on //crypto/cert, and any networking
capabilities needed by //crypto/cert would be passed via dependency injection
when setting up a CertVerifier.

[Ryan Sleevi, 2015-03-25 18:41:15]

On 2015/03/25 18:30:24, eroman wrote:
> I will split off a new subdirectory, "cert_fetcher/" and then put the
> implementation in there, and an equivalent interface into "cert/".
> 
> But TBH I remain unconvinced that this improves the total complexity. We still
> have a circular dependency between URLRequest -> CertVerifier ->
CertNetFetcher
> -> URLRequest when piecing the blocks together, so the root problem remains.
The
> main difference is if in the future we wanted to have a separable //net/cert
> target, then consumers would have the flexibility of choosing cert fetching
> without networking capabilities, or additionally add a dependency on
> //net/cert_fetcher to pass in a CertNetFetcherImpl dependency to CertVerifier

You shouldn't have a circular dependency if done right. Below is the crappy
ASCII art

CertVerifier --> CertNetFetcher <-- CertNetFetcherImpl --> URLRequest::Delegate
<-- URLRequest

Where -> indicates dependencies
CertVerifier depends solely on the interface CertNetFetcher (not the
implementation).
CertNetFetcher's concrete implementation is CertNetFetcherImpl, which
CertVerifier never knows about. CertNetFetcherImpl depends on
URLRequest::Delegate (and URLRequest, to be fair)
URLRequest never knows about CertNetFetcherImpl - that is, it depends on
URLRequest::Delegate (for getting messages out), and [transitively] CertVerifier
for verifying socket's certs

In order to break the cycle, CertNetFetcherImpl's concrete instantiation gets
uplifted/created elsewhere (e.g. in //content? //chrome? at some layer higher)

If I understood your concern, it's that once you inject CertNetFetcherImpl, you
create a cycle of implicit transitive dependencies? This is true, but that is
done by a higher layer, which is allowed to make cycles; at the lower layer,
there's no code that directly forces that dependency.

You can see this design pattern throughout //content and //components for
breaking things up.

[Ryan Sleevi, 2015-03-25 18:44:18]

On 2015/03/25 18:36:25, eroman wrote:
> Or a different organization to consider would be to pull //net/cert out of
//net
> alltogether, and move it into say //crypto/cert
> 
> Then we would have //net depending on //crypto/cert, and any networking
> capabilities needed by //crypto/cert would be passed via dependency injection
> when setting up a CertVerifier.

Yeah, that's something that honestly remains TBD. It's something Wan-Teh, Adam
and I discussed 3 years ago. At the time, we didn't have compelling reasons.

TBH, I suspect at some point we will do just that, as more implementations
switch to using our certificate verification. e.g., for Chromecast, if they
wanted to do signed message attestations using this, it doesn't make sense that
it's "net" when really it's cryptographic messaging. Or if the extensions system
moved from simply being key based to being certificate based, as has been
explored in the past, that's another consideration.

Which is also part of why I poke at separating the two, so that we could
eventually uplift //net/cert into //crypto/cert, but leave the Impl in //net

[eroman, 2015-03-25 18:45:56]

I am referring to the *runtime* circular dependency when using it.

I agree that the compile time dependency can be uplifted, and I will go ahead
and do that per our earlier discussion.

However I see the larger problem being the total complexity of the circular
(runtime) dependency, which is not improved by shuffling interfaces around :)

[Ryan Sleevi, 2015-03-25 19:11:43]

On 2015/03/25 18:45:56, eroman wrote:
> I am referring to the *runtime* circular dependency when using it.
> 
> I agree that the compile time dependency can be uplifted, and I will go ahead
> and do that per our earlier discussion.
> 
> However I see the larger problem being the total complexity of the circular
> (runtime) dependency, which is not improved by shuffling interfaces around :)

Do you see a solution towards solving the runtime circular dependency? I see
this as fundamental, in as much as it's fundamental to SDCH. I'm certainly
curious, I do see these designs somewhat inextricably circular at runtime, to
which the solution is fundamentally dependency injection.

[eroman, 2015-03-25 19:19:24]

No I don't have a solution to that

[eroman, 2015-03-26 03:50:49]

I split things between an interface (CertNetFetcher) in net/cert, and an
implementation (CertNetFetcherImpl) in net/cert_net.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.cc (right):

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:134: return false;
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> Normally when writing conditional comparators, I like to collapse lines
132-134
> to just return 133
> 
> If there an intentional reason you avoided that (e.g. to make it easier to add
> new fields)?

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:231: parent_ = NULL;
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:247: DCHECK(it != requests_.end());
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> Should this be a CHECK() instead?
> 
> Just 248 makes me extremely nervous for subtle and hard to detect memory
> corruption; might as well blow things up quick.

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:270: this, NULL);
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:401: parent_->SetCurrentlyCompletingJob(NULL);
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:405: : currently_completing_job_(NULL),
context_(context) {
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:505: if (it != jobs_.end() && !(params <
(*it)->request_params()))
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> Is this second conditional meant to be an equality check, or a sanity check?
If
> a equality check, you should check both that
> 
> !(a < b) && !(b < a)

It is essentially an equality check.

std::lower_bound() returns an iterator that is "greater than or equal" to our
search value |params|. Or in other words, it returns the first iterator that is
not less then the search value.

So by definition !(*it < params) and I don't bother testing that in the if.

Re-written another way, we know that if it != jobs_.end():
  params <= *it

So the remaining test is to check if:
  params < *it

If it is NOT, then we have equality (params == *it).


So to address your comment, I can either add a DCHECK(), or a comment explaining
what is going on here since it is admittedly a bit obtuse.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.cc:507: return NULL;
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:16: #include "url/gurl.h"
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> Can forward declare

Done.

https://codereview.chromium.org/908863004/diff/300001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:73: static const int DEFAULT = -1;
On 2015/03/25 06:16:37, Ryan Sleevi wrote:
> So this syntax is super sketch (there's some old chromium-dev threads that
> explain why; TL;DR - static storage needs to be filled in the .cc for MSVC,
but
> that can make gcc/clang grumpy)
> 
> You could use an enum of int type here, since the enum will promote.

Interesting, I have gotten the opposite feedback from other developers, asking
me to turn my enum hack into static const on reviews, claiming that the compiler
issues are no more.

(Will see if I can find that review. I think it was blink side from abarth, but
could be wrong)

OK, I will go ahead and enumify it, its all the same to me.

[eroman, 2015-03-26 03:58:10]

eroman@chromium.org changed reviewers:
- mattm@chromium.org, rsleevi@chromium.org

[eroman, 2015-03-26 03:58:11]

(Promoting David to primary reviewer, and moved Ryan to the cc-list)

[davidben, 2015-03-27 23:34:32]

I'm apparently distractable when doing large reviews and didn't finish going
through all of it. But flushing set of comments so far so they aren't sitting
around as drafts.

https://codereview.chromium.org/908863004/diff/360001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:72: virtual void CancelRequest(RequestId request) =
0;
Rather than CancelRequest, perhaps:

class Request {
  virtual ~Request() = 0;
};

virtual scoped_ptr<Request> FetchCaIssuers(...)

So the caller can just toss it around and stuff. Though it does mean that the
destructor needs to check if the request already completed first. I suspect it'd
also make the ownership around Jobs and Requests less fiddly. Matches how
HttpStreamFactory works too I think.

(I didn't look too carefully at the interaction, under the assumption that this
would change some things. If you prefer the current scheme, I'll review the
current logic more carefully.)

https://codereview.chromium.org/908863004/diff/360001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:89: const FetchCallback& callback) = 0;
Maybe for a followup, but since we want to be able to debug circular
dependencies, it might be prudent to pass in a BoundNetLog bits into these
functions so the CERT_VERIFIER_JOB in net-internals can link to the URL_REQUEST.
(Hrm. There isn't a reverse pointer right now from CERT_VERIFIER_JOB to SOCKET.
That seems worth fixing.)

Though how that interacts with this potential movement from net/cert to
crypto/cert, I have no idea. I suspect the NetLog integration will be valuable.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/README
File net/cert_net/README (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/README#new...
net/cert_net/README:1: cert_net/ contains certificate functionality that depends
on network loading (OCSP, CRL, AIA fetching).
Sort of a strange name, but I don't have better suggestions. :-/

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:36: const Error
kNetErrorNot200HttpResponse = ERR_FAILED;
Just using ERR_TIMED_OUT and the like where you use them seems sufficient.
ERR_TIMED_OUT is already perfectly self-explanatory I think.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:132: return timeout < other.timeout;
Aside: Comparing timeouts here is kinda weird. Though the alternative I can
imagine (each Request has individual timeouts that trigger independently and the
Job stays alive until all Requests have given up) isn't necessarily sound either
and seems kinda complicated. Leaving it the way you have it seems better.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:162: using RequestList =
std::vector<Request*>;
This might change if you make the consumer own the Request, but ScopedVector?

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:175: // Reads as much data as available
from the |request|.
Nit: from the -> from

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:252: delete_this =
parent_->RemoveJob(this);
Why not just "parent_->RemoveJob(this)"? Or moving the variable into the if?

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:290: OnUrlRequestCompleted(request);
This does end up deleting |request| while it's on the stack. Though it seems
URLRequest is generally designed to allow for that.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:393:
parent_->SetCurrentlyCompletingJob(this);
DCHECK somewhere that there isn't another currently completing job?

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.h (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.h:60: struct RequestParams;
Nit: ordering. I think it's classes before structs and then alphabetical?

[davidben, 2015-03-31 20:52:26]

(and the rest of the review)

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:191: // (result_net_error_code_ et al).
Nit: result_net_error_code_ -> result_net_error_

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:304: // measure in case the policy is ever
changed.
I don't understand what this comment is referring to. If the server gave back
302 or even 404, is_success would still be true, right?

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:359:
response_body_.reserve(response_body_.size() + num_bytes);
Does the reserve call do anything? I would have hoped that the vector has enough
information just resize it first. All of libstdc++, stlport, and libc++ seem to
measure std::distance first.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:517: return scoped_ptr<Job>(job);
Optional: make_scoped_ptr might be a little less repetitive. (I recently
discovered it existed.)

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:153: CertNetFetcher* fetcher) {
Nit: Probably makes more sense for the fetcher to be the first argument since
it's the context of sorts?

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:304: GURL
url(test_server_.GetURL("slow/certs.p7c?5.1"));
Why 5.1? The comment says 5.

(I think 5.1 doesn't work right now anyway.
https://codereview.chromium.org/1049343002/ )

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:375: fetcher.CancelRequest(id);
This probably wants a RunUntilIdle and still assert that callback.HasResult is
false, to assert that the callback never runs.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:426: // requests are given opened
sockets in a FIFO order.
Perhaps if we configured the socket pool allow a maximum of one socket per
group. That would force us to serialize on our end and work around the server
bug. Or we could CloseIdleSockets() right after the CancelRequest (though that
depends on CancelRequest synchronously releasing the socket).

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:448: // Wait for the fast request
to complete.
fast -> last?

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:453: fetcher.CancelRequest(id2);
This assumes that callback2 hasn't fired yet. (Realistically this won't happen,
but you have waited on a WaitForResult already.)

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:468: GURL
url(test_server_.GetURL("slow/certs.p7c?20.1"));
Why 20.1 instead of 20?

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:477:
TEST_F(CertNetFetcherImplTest, ParallelFetchDupes) {
Nit: Dupes is a strange word. Duplicates? :-)

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:578: }
Nit: This line has curlies while the rest don't.

(Curly-less for loops actually really really confuse me, but apparently we do
that in //net, so okay.)

[eroman, 2015-04-04 21:57:16]

https://codereview.chromium.org/908863004/diff/360001/net/cert/cert_net_fetch...
File net/cert/cert_net_fetcher.h (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:72: virtual void CancelRequest(RequestId request) =
0;
On 2015/03/27 23:34:31, David Benjamin wrote:
> Rather than CancelRequest, perhaps:
> 
> class Request {
>   virtual ~Request() = 0;
> };
> 
> virtual scoped_ptr<Request> FetchCaIssuers(...)
> 
> So the caller can just toss it around and stuff. Though it does mean that the
> destructor needs to check if the request already completed first. I suspect
it'd
> also make the ownership around Jobs and Requests less fiddly. Matches how
> HttpStreamFactory works too I think.
> 
> (I didn't look too carefully at the interaction, under the assumption that
this
> would change some things. If you prefer the current scheme, I'll review the
> current logic more carefully.)

Done.

I agree this is cleaner, I think we should do use this approach elsewhere in the
network stack. I filed
https://code.google.com/p/chromium/issues/detail?id=473980 to address that.

https://codereview.chromium.org/908863004/diff/360001/net/cert/cert_net_fetch...
net/cert/cert_net_fetcher.h:89: const FetchCallback& callback) = 0;
On 2015/03/27 23:34:31, David Benjamin wrote:
> Maybe for a followup, but since we want to be able to debug circular
> dependencies, it might be prudent to pass in a BoundNetLog bits into these
> functions so the CERT_VERIFIER_JOB in net-internals can link to the
URL_REQUEST.
> (Hrm. There isn't a reverse pointer right now from CERT_VERIFIER_JOB to
SOCKET.
> That seems worth fixing.)
> 
> Though how that interacts with this potential movement from net/cert to
> crypto/cert, I have no idea. I suspect the NetLog integration will be
valuable.

+1 on NetLog integration. I will add that as a followup.
Added a TODO at the top of cert_net_fetcher_impl.cc

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/README
File net/cert_net/README (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/README#new...
net/cert_net/README:1: cert_net/ contains certificate functionality that depends
on network loading (OCSP, CRL, AIA fetching).
On 2015/03/27 23:34:31, David Benjamin wrote:
> Sort of a strange name, but I don't have better suggestions. :-/

It will make more sense when (or if) net/cert moves to crypto/cert.
At that point, net/cert_net can simply be renamed to net/cert.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:36: const Error
kNetErrorNot200HttpResponse = ERR_FAILED;
On 2015/03/27 23:34:32, David Benjamin wrote:
> Just using ERR_TIMED_OUT and the like where you use them seems sufficient.
> ERR_TIMED_OUT is already perfectly self-explanatory I think.

Done.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:132: return timeout < other.timeout;
On 2015/03/27 23:34:32, David Benjamin wrote:
> Aside: Comparing timeouts here is kinda weird. Though the alternative I can
> imagine (each Request has individual timeouts that trigger independently and
the
> Job stays alive until all Requests have given up) isn't necessarily sound
either
> and seems kinda complicated. Leaving it the way you have it seems better.

Yeah agreed. There are some different strategies for how the timeout of a job
corresponds to the timeout of a request. I think this is the simplest one and
should work well enough, so long as incoming requests aren't all using different
timeouts. (I assume they will all be using the same timeout though, so this is
more of a formality).

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:162: using RequestList =
std::vector<Request*>;
On 2015/03/27 23:34:31, David Benjamin wrote:
> This might change if you make the consumer own the Request, but ScopedVector?

No longer applicable, as the request is no longer owned by the Job.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:175: // Reads as much data as available
from the |request|.
On 2015/03/27 23:34:32, David Benjamin wrote:
> Nit: from the -> from

Done.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:191: // (result_net_error_code_ et al).
On 2015/03/31 20:52:23, David Benjamin wrote:
> Nit: result_net_error_code_ -> result_net_error_

Done.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:252: delete_this =
parent_->RemoveJob(this);
On 2015/03/27 23:34:31, David Benjamin wrote:
> Why not just "parent_->RemoveJob(this)"? Or moving the variable into the if?

I wanted to ensure that deletion of |this| was the very last thing that occurs
in CancelRequest. This is accomplished by defining "scoped_ptr<Job> delete_this"
at the top of the function.

What I don't like about deleting inside the "if" is that more code can execute
following the destruction of "this", which may or may not try to access member
variables.

For instance the dtor for |it| will run now after destruction of |this| While in
practice it won't be accessing anything within |this| it still feels wrong since
|it| is an iterator to a vector owned by |this|.

All things being equal I would rather keep the explicit |delete_this| since
deleting |this| makes me very uncomfortable

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:290: OnUrlRequestCompleted(request);
On 2015/03/27 23:34:31, David Benjamin wrote:
> This does end up deleting |request| while it's on the stack. Though it seems
> URLRequest is generally designed to allow for that.

Acknowledged.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:304: // measure in case the policy is ever
changed.
On 2015/03/31 20:52:23, David Benjamin wrote:
> I don't understand what this comment is referring to. If the server gave back
> 302 or even 404, is_success would still be true, right?

Fixed. This comment was referring to an older version of the code, and is no
longer relevant. (I used to have a check that the response was from HTTP before
inspecting the response code, which is being interpreted specific to http).

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:359:
response_body_.reserve(response_body_.size() + num_bytes);
On 2015/03/31 20:52:23, David Benjamin wrote:
> Does the reserve call do anything? I would have hoped that the vector has
enough
> information just resize it first. All of libstdc++, stlport, and libc++ seem
to
> measure std::distance first.

Agreed I don't think it is necessary. However I added it in response to a
recommendation from sleevi:
https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...

I will go ahead and remove it, and welcome my new overlord.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:393:
parent_->SetCurrentlyCompletingJob(this);
On 2015/03/27 23:34:31, David Benjamin wrote:
> DCHECK somewhere that there isn't another currently completing job?

Done. I added a new method ClearCurrentlyCompletingJob() so that
SetCurrentlyCompletingJob() can do the DCHECK.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:517: return scoped_ptr<Job>(job);
On 2015/03/31 20:52:23, David Benjamin wrote:
> Optional: make_scoped_ptr might be a little less repetitive. (I recently
> discovered it existed.)

Done.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:153: CertNetFetcher* fetcher) {
On 2015/03/31 20:52:25, David Benjamin wrote:
> Nit: Probably makes more sense for the fetcher to be the first argument since
> it's the context of sorts?

I left it last because I thought it was more style guide compliant
(out-parameters go at the end; this is kind of an in/out parameter).

I haven't changed this yet; let me know how you want me to proceed.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:304: GURL
url(test_server_.GetURL("slow/certs.p7c?5.1"));
On 2015/03/31 20:52:25, David Benjamin wrote:
> Why 5.1? The comment says 5.
> 
> (I think 5.1 doesn't work right now anyway.
> https://codereview.chromium.org/1049343002/ )

Changed all the floating points to integers.

I had copied the code from elsewhere and wasn't sure if non-floats would work so
just conservatively used floats everywhere.

... And based on your bugfix, turns out even that was wrong ;)

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:375: fetcher.CancelRequest(id);
On 2015/03/31 20:52:24, David Benjamin wrote:
> This probably wants a RunUntilIdle and still assert that callback.HasResult is
> false, to assert that the callback never runs.

Done.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:426: // requests are given opened
sockets in a FIFO order.
On 2015/03/31 20:52:25, David Benjamin wrote:
> Perhaps if we configured the socket pool allow a maximum of one socket per
> group. That would force us to serialize on our end and work around the server
> bug. Or we could CloseIdleSockets() right after the CancelRequest (though that
> depends on CancelRequest synchronously releasing the socket).

I took a quick look on how to configure that and didn't find it. Will leave a
TODO to make this more robust.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:448: // Wait for the fast request
to complete.
On 2015/03/31 20:52:23, David Benjamin wrote:
> fast -> last?

Fast was intentional -- The last request is for an FTP URL, so it will fail
immediately. Whereas the other requests will try to talk to the test server. So
it is expected that the last request will be the first to complete.

Reworded the comment.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:453: fetcher.CancelRequest(id2);
On 2015/03/31 20:52:24, David Benjamin wrote:
> This assumes that callback2 hasn't fired yet. (Realistically this won't
happen,
> but you have waited on a WaitForResult already.)

With the new scoped_ptr<Request> approach this is no longer a problem (since the
Request is owned by caller).

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:468: GURL
url(test_server_.GetURL("slow/certs.p7c?20.1"));
On 2015/03/31 20:52:26, David Benjamin wrote:
> Why 20.1 instead of 20?

Done.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:477:
TEST_F(CertNetFetcherImplTest, ParallelFetchDupes) {
On 2015/03/31 20:52:25, David Benjamin wrote:
> Nit: Dupes is a strange word. Duplicates? :-)

Done.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:578: }
On 2015/03/31 20:52:25, David Benjamin wrote:
> Nit: This line has curlies while the rest don't.
> 
> (Curly-less for loops actually really really confuse me, but apparently we do
> that in //net, so okay.)

Done.

[davidben, 2015-04-07 19:22:25]

Just some minor comments (note that some are attached to the previous patch
set). The thing with the currently-completing job is a little confusing, but I
don't see a better way to do it and still keep all the properties we have now
(like destroying the fetcher in a callback stops iteration).

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:359:
response_body_.reserve(response_body_.size() + num_bytes);
On 2015/04/04 21:57:15, eroman wrote:
> On 2015/03/31 20:52:23, David Benjamin wrote:
> > Does the reserve call do anything? I would have hoped that the vector has
> enough
> > information just resize it first. All of libstdc++, stlport, and libc++ seem
> to
> > measure std::distance first.
> 
> Agreed I don't think it is necessary. However I added it in response to a
> recommendation from sleevi:
>
https://codereview.chromium.org/908863004/diff/140001/net/cert/cert_net_fetch...
> 
> I will go ahead and remove it, and welcome my new overlord.

Oh, hrmf. Sorry, I should have checked one more STL first:

MSVC's std::vector::insert just calls push_back in a loop and *doesn't* measure
std::distance. So it is probably marginally better to do the reserve. :-/ Mehhh.

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:153: CertNetFetcher* fetcher) {
On 2015/04/04 21:57:15, eroman wrote:
> On 2015/03/31 20:52:25, David Benjamin wrote:
> > Nit: Probably makes more sense for the fetcher to be the first argument
since
> > it's the context of sorts?
> 
> I left it last because I thought it was more style guide compliant
> (out-parameters go at the end; this is kind of an in/out parameter).
> 
> I haven't changed this yet; let me know how you want me to proceed.

Hrm. I see it as more a context than an in/out parameter (you aren't really
writing to fetcher, you're doing something with it). I guess the style guide
doesn't actually say anything about "context parameters" or that they're a
thing. Anyway, I don't feel terribly strongly either way.

(I'm mostly getting that wording from BoringSSL, which does context/out/in. And
since I wrote that style guide, I may as well have made it up. But I think I
borrowed that word from agl, so...)

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:276: if (requests_.empty() && parent_)
Is it possible for parent_ to be null here? Right after we null parent_, we
clear through all the requests, which means DetachRequest can't have been
called.

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:420: requests_.erase(requests_.begin());
Probably doesn't matter, but note this loop is O(N^2). (If we want it linear,
this could be a std::list and each Request could hold onto an iterator into
requests_ so that removal of a single request is also O(1). Another option is a
std::set, though then you don't get FIFO ordering.)

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:433: STLDeleteElements(&jobs_);
Maybe add a comment here like:

  // The CertNetFetcher was destroyed in a FetchCallback. Detach all remaining
requests from the job so no further callbacks are called.

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:508: return request.Pass();
Perhaps this trio could be something like

  return job->CreateRequest(callback);

Since a RequestImpl always gets OnAttachedToJob called immediately after
creation anyway. It may as well be a parameter in the constructor.

[eroman, 2015-04-07 19:54:55]

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:420: requests_.erase(requests_.begin());
On 2015/04/07 19:22:25, David Benjamin wrote:
> Probably doesn't matter, but note this loop is O(N^2). (If we want it linear,
> this could be a std::list and each Request could hold onto an iterator into
> requests_ so that removal of a single request is also O(1). Another option is
a
> std::set, though then you don't get FIFO ordering.)

The simplest fix would be to use a dequeue instead of a vector.

Alternately I could use an std::vector and pop elements off the back (perhaps
having reversed it first).

I am not sure how important FIFO completion is. Likely doesn't matter but kind
of makes more sense for things to complete in order.

[eroman, 2015-04-07 20:22:40]

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/908863004/diff/360001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl_unittest.cc:153: CertNetFetcher* fetcher) {
On 2015/04/07 19:22:25, David Benjamin wrote:
> On 2015/04/04 21:57:15, eroman wrote:
> > On 2015/03/31 20:52:25, David Benjamin wrote:
> > > Nit: Probably makes more sense for the fetcher to be the first argument
> since
> > > it's the context of sorts?
> > 
> > I left it last because I thought it was more style guide compliant
> > (out-parameters go at the end; this is kind of an in/out parameter).
> > 
> > I haven't changed this yet; let me know how you want me to proceed.
> 
> Hrm. I see it as more a context than an in/out parameter (you aren't really
> writing to fetcher, you're doing something with it). I guess the style guide
> doesn't actually say anything about "context parameters" or that they're a
> thing. Anyway, I don't feel terribly strongly either way.
> 
> (I'm mostly getting that wording from BoringSSL, which does context/out/in.
And
> since I wrote that style guide, I may as well have made it up. But I think I
> borrowed that word from agl, so...)

Done.

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:276: if (requests_.empty() && parent_)
On 2015/04/07 19:22:25, David Benjamin wrote:
> Is it possible for parent_ to be null here? Right after we null parent_, we
> clear through all the requests, which means DetachRequest can't have been
> called.

Done.

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:420: requests_.erase(requests_.begin());
On 2015/04/07 19:54:54, eroman (sick slow) wrote:
> On 2015/04/07 19:22:25, David Benjamin wrote:
> > Probably doesn't matter, but note this loop is O(N^2). (If we want it
linear,
> > this could be a std::list and each Request could hold onto an iterator into
> > requests_ so that removal of a single request is also O(1). Another option
is
> a
> > std::set, though then you don't get FIFO ordering.)
> 
> The simplest fix would be to use a dequeue instead of a vector.
> 
> Alternately I could use an std::vector and pop elements off the back (perhaps
> having reversed it first).
> 
> I am not sure how important FIFO completion is. Likely doesn't matter but kind
> of makes more sense for things to complete in order.

Done (switched to deque).

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:433: STLDeleteElements(&jobs_);
On 2015/04/07 19:22:25, David Benjamin wrote:
> Maybe add a comment here like:
> 
>   // The CertNetFetcher was destroyed in a FetchCallback. Detach all remaining
> requests from the job so no further callbacks are called.

Done.

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:508: return request.Pass();
On 2015/04/07 19:22:25, David Benjamin wrote:
> Perhaps this trio could be something like
> 
>   return job->CreateRequest(callback);
> 
> Since a RequestImpl always gets OnAttachedToJob called immediately after
> creation anyway. It may as well be a parameter in the constructor.

Done.

[davidben, 2015-04-07 21:22:39]

lgtm

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:420: requests_.erase(requests_.begin());
On 2015/04/07 20:22:40, eroman (sick slow) wrote:
> On 2015/04/07 19:54:54, eroman (sick slow) wrote:
> > On 2015/04/07 19:22:25, David Benjamin wrote:
> > > Probably doesn't matter, but note this loop is O(N^2). (If we want it
> linear,
> > > this could be a std::list and each Request could hold onto an iterator
into
> > > requests_ so that removal of a single request is also O(1). Another option
> is
> > a
> > > std::set, though then you don't get FIFO ordering.)
> > 
> > The simplest fix would be to use a dequeue instead of a vector.
> > 
> > Alternately I could use an std::vector and pop elements off the back
(perhaps
> > having reversed it first).
> > 
> > I am not sure how important FIFO completion is. Likely doesn't matter but
kind
> > of makes more sense for things to complete in order.
> 
> Done (switched to deque).

The reason I suggested a list was that a deque still makes canceling N requests
attached to a job take O(N^2). I doubt this really matters though, so *shrug*

https://codereview.chromium.org/908863004/diff/500001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/500001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:263: request->OnAttachedToJob(this);
I was actually thinking you could then just do

  scoped_ptr<RI> request(new RI(this, callback));
  requests_.push_back(request.get());
  return request.Pass();

and drop the OnAttachedToJob as well. *shrug*

[eroman, 2015-04-08 16:23:39]

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/480001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:420: requests_.erase(requests_.begin());
On 2015/04/07 21:22:39, David Benjamin wrote:
> On 2015/04/07 20:22:40, eroman (sick slow) wrote:
> > On 2015/04/07 19:54:54, eroman (sick slow) wrote:
> > > On 2015/04/07 19:22:25, David Benjamin wrote:
> > > > Probably doesn't matter, but note this loop is O(N^2). (If we want it
> > linear,
> > > > this could be a std::list and each Request could hold onto an iterator
> into
> > > > requests_ so that removal of a single request is also O(1). Another
option
> > is
> > > a
> > > > std::set, though then you don't get FIFO ordering.)
> > > 
> > > The simplest fix would be to use a dequeue instead of a vector.
> > > 
> > > Alternately I could use an std::vector and pop elements off the back
> (perhaps
> > > having reversed it first).
> > > 
> > > I am not sure how important FIFO completion is. Likely doesn't matter but
> kind
> > > of makes more sense for things to complete in order.
> > 
> > Done (switched to deque).
> 
> The reason I suggested a list was that a deque still makes canceling N
requests
> attached to a job take O(N^2). I doubt this really matters though, so *shrug*

Will leave it as is for now along with a comment.
I don't think this will matter in practice, but can do a follow-up after some
testing.

https://codereview.chromium.org/908863004/diff/500001/net/cert_net/cert_net_f...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/908863004/diff/500001/net/cert_net/cert_net_f...
net/cert_net/cert_net_fetcher_impl.cc:263: request->OnAttachedToJob(this);
On 2015/04/07 21:22:39, David Benjamin wrote:
> I was actually thinking you could then just do
> 
>   scoped_ptr<RI> request(new RI(this, callback));
>   requests_.push_back(request.get());
>   return request.Pass();
> 
> and drop the OnAttachedToJob as well. *shrug*

Done.

[eroman, 2015-04-08 16:24:26]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-04-08 16:24:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/908863004/#ps520001
(title: "address more comments")

[commit-bot: I haz the power, 2015-04-08 16:26:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908863004/520001

[commit-bot: I haz the power, 2015-04-08 16:36:07]

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

[commit-bot: I haz the power, 2015-04-08 16:36:08]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[eroman, 2015-04-08 16:44:36]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-04-08 16:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/908863004/#ps540001
(title: "disable on ios")

[commit-bot: I haz the power, 2015-04-08 16:45:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908863004/540001

[commit-bot: I haz the power, 2015-04-08 16:56:33]

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

[commit-bot: I haz the power, 2015-04-08 16:56:34]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[eroman, 2015-04-08 17:08:51]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-04-08 17:08:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/908863004/#ps560001
(title: "add missing comma")

[commit-bot: I haz the power, 2015-04-08 17:09:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908863004/560001

[eroman, 2015-04-08 18:02:44]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2015-04-08 18:02:53]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-04-08 18:04:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908863004/560001

[commit-bot: I haz the power, 2015-04-08 18:54:19]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2015-04-08 18:55:20]

Patchset 29 (id:??) landed as
https://crrev.com/0fad62b76d552f09f8a1c90edce26fe64df9a7f9
Cr-Commit-Position: refs/heads/master@{#324261}