Remove dependence on a message loop for net::PathBuilder.

Remove dependence on a message loop for net::PathBuilder.

 * Path building now can only complete synchronously
 * Access to trust store is done synchronously
 * The network dependencies (AIA fetching) will block the thread

BUG=649017
Committed: https://crrev.com/994db999829331ea3b982d9c9ec469fde22de21d
Cr-Commit-Position: refs/heads/master@{#434769}

[eroman, 2016-10-27 01:23:57]

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

[commit-bot: I haz the power, 2016-10-27 01:24:24]

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

[commit-bot: I haz the power, 2016-10-27 02:07:55]

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

[commit-bot: I haz the power, 2016-10-27 02:07:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eroman, 2016-10-27 02:32:44]

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

[mattm, 2016-10-28 02:11:56]

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/cert_issu...
File net/cert/internal/cert_issuer_source_aia.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/cert_issu...
net/cert/internal/cert_issuer_source_aia.cc:65: bool
AiaRequest::OnFetchCompleted(Error error,
Wondering if this should be renamed, since it makes it sound like this is still
a callback..

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
net/cert/internal/path_builder.cc:114: // If asynchronous requests were made,
which ones are still outstanding?
comment is a bit confusing / indirect. maybe mention it is an index into
pending_async_requests_, and that it is next outstanding one to process, or
something?

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
File net/cert/internal/path_builder_unittest.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
net/cert/internal/path_builder_unittest.cc:601: // path built with newroot
fails.
comment needs updating

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_nss_unittest.cc (left):

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_nss_unittest.cc:118: SCOPED_TRACE(loc.ToString());
this was so you could tell which ExpectTrustStoreContains call failed.. but
since it now only has the EXPECT_EQ at the end, you could just return a bool and
have EXPECT_TRUE in the caller

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:61: class NET_EXPORT
AsyncCertNetFetcherImpl {
NET_EXPORT of anonymous class?

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:93: // |currently_completing_job_|.
currently_completing_job_ doesn't exist anymore

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:96: // Not owned. AsyncCertNetFetcherImpl
must outlive the URLRequestContext.
backwards?

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:306: void RequestCore::Cancel() {
Should be up with RequestCore definition? does it need to be after the Job
definition?

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:543: class CertNetFetcherRequestImpl :
public CertNetFetcher::Request {
class comments for all the classes. How about a file-level comment giving an
overview how everything fits together too?

[eroman, 2016-11-21 18:48:52]

Thanks for the comments!

Sorry for not responding sooner -- I was out past few weeks (between offset,
then vacation, then catchup-week/bugtriage).

Getting back to this now :)

[eroman, 2016-11-23 22:06:34]

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

[commit-bot: I haz the power, 2016-11-23 22:07:21]

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

[eroman, 2016-11-23 22:10:21]

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/cert_issu...
File net/cert/internal/cert_issuer_source_aia.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/cert_issu...
net/cert/internal/cert_issuer_source_aia.cc:65: bool
AiaRequest::OnFetchCompleted(Error error,
On 2016/10/28 02:11:55, mattm wrote:
> Wondering if this should be renamed, since it makes it sound like this is
still
> a callback..

Done -- renamed to AddCompletedFetchToResults()

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
net/cert/internal/path_builder.cc:114: // If asynchronous requests were made,
which ones are still outstanding?
On 2016/10/28 02:11:55, mattm wrote:
> comment is a bit confusing / indirect. maybe mention it is an index into
> pending_async_requests_, and that it is next outstanding one to process, or
> something?

Done.

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
File net/cert/internal/path_builder_unittest.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert/internal/path_buil...
net/cert/internal/path_builder_unittest.cc:601: // path built with newroot
fails.
On 2016/10/28 02:11:55, mattm wrote:
> comment needs updating

Done.

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
File net/cert_net/cert_net_fetcher_impl.cc (right):

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:61: class NET_EXPORT
AsyncCertNetFetcherImpl {
On 2016/10/28 02:11:56, mattm wrote:
> NET_EXPORT of anonymous class?

Done.

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:96: // Not owned. AsyncCertNetFetcherImpl
must outlive the URLRequestContext.
On 2016/10/28 02:11:55, mattm wrote:
> backwards?

Done.

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:306: void RequestCore::Cancel() {
On 2016/10/28 02:11:56, mattm wrote:
> Should be up with RequestCore definition? does it need to be after the Job
> definition?

There is a circular dependency between Job and RequestCore.

 * Job can call methods on RequestCore, hence those definitions must be after
RequestCore's declaration

 * RequestCore::Cancel() can call a method on Job and hence its definition must
be after RequestCore's declaration.


So I just moved RequestCore::Cancel() to appear lower in the file.

https://codereview.chromium.org/2453093004/diff/1/net/cert_net/cert_net_fetch...
net/cert_net/cert_net_fetcher_impl.cc:543: class CertNetFetcherRequestImpl :
public CertNetFetcher::Request {
On 2016/10/28 02:11:56, mattm wrote:
> class comments for all the classes. How about a file-level comment giving an
> overview how everything fits together too?

Thanks, done!

[mattm, 2016-11-23 23:49:19]

lgtm

[eroman, 2016-11-23 23:50:42]

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

[eroman, 2016-11-23 23:50:42]

+sheretov for cast changes

[sheretov, 2016-11-23 23:55:12]

lgtm

[commit-bot: I haz the power, 2016-11-24 00:10:06]

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

[commit-bot: I haz the power, 2016-11-24 00:10:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[eroman, 2016-11-24 02:17:00]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-11-24 02:17:48]

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

[commit-bot: I haz the power, 2016-11-24 04:19:13]

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

[commit-bot: I haz the power, 2016-11-24 04:19:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[eroman, 2016-11-28 17:43:46]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-11-28 17:44:31]

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

[commit-bot: I haz the power, 2016-11-28 17:51:48]

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

[commit-bot: I haz the power, 2016-11-28 17:51:49]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[eroman, 2016-11-28 18:04:07]

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

[commit-bot: I haz the power, 2016-11-28 18:05:01]

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

[eroman, 2016-11-28 19:26:38]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2016-11-28 19:26:41]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-11-28 19:26:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
sheretov@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2453093004/#ps40001
(title: "remove unnecessary forward decl")

[commit-bot: I haz the power, 2016-11-28 19:27:51]

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

[commit-bot: I haz the power, 2016-11-28 23:23:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480361201604900,
"parent_rev": "eb8953250ee8d2f8403e17b71a8a694e9d5c27f3", "commit_rev":
"d16ab5ce8d00b5c258595566c1d381650c194172"}

[commit-bot: I haz the power, 2016-11-28 23:24:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-28 23:27:05]

Description was changed from

==========
Remove dependence on a message loop for net::PathBuilder.

 * Path building now can only complete synchronously
 * Access to trust store is done synchronously
 * The network dependencies (AIA fetching) will block the thread

BUG=649017
==========

to

==========
Remove dependence on a message loop for net::PathBuilder.

 * Path building now can only complete synchronously
 * Access to trust store is done synchronously
 * The network dependencies (AIA fetching) will block the thread

BUG=649017

Committed: https://crrev.com/994db999829331ea3b982d9c9ec469fde22de21d
Cr-Commit-Position: refs/heads/master@{#434769}
==========

[commit-bot: I haz the power, 2016-11-28 23:27:06]

Patchset 3 (id:??) landed as
https://crrev.com/994db999829331ea3b982d9c9ec469fde22de21d
Cr-Commit-Position: refs/heads/master@{#434769}