To followup in the bug per Ryan's request.

I'm suggesting that we make the logic in CertVerifierBrowserTest usable in content/ by refactoring that class. If it was a member that a test can use in its SetUp methods, instead of having to inherit, that would be possible. This would also be more in line with the style guide that discourages implementation inheritance.

This gets content to parity with chrome for mocking certs in a way that's compatible with the network service.

As for not using net::MockCertVerifier at all, since it uses a global: I realize globals suck but the alternative to fixing this now is a non trivial amount of work. Is there any pressing need to changing that?

Bringing the //chrome MockCertVerifier (with mojo-awareness) down to //content SGTM - definitely was thinking that was a necessary step.

I don't think this is a blocker for the Web Packaging to fix, since it looks like it's not as low hanging w/r/t ContentShell as it had appeared (compared to the LayoutTests approach to subclassing, which seems necessary for LayoutTests and seemed like it provides a good model at least for content_browsertests). Nor do I think it's a blocker for S13N work, in case that may have come across.

Also - it wasn't that MockCertVerifier uses a global, but rather, that we should try not to introduce new global methods that expose/configure MockCertVerifiers, and instead prefer to have defined interfaces for passing it in || configuring it.

For //contents case, I spent several hours looking at how various tests were doing things - whether SetBrowserClientForTesting() to supply fakes, or looking at how LayoutTests does a lot of implementation sub-classing (which seemed to be part of the design of the ShellURLRequestContextGetter-and-upwards, given the explicit virtual methods for subclasses to override). Specifically for the //content case, I was hoping find a way for those content_browsertests to have their URLRequestContextGetters for the content_shell configured in such a way as to allow testing (and configuring the network service), and that would work without requiring full S13N (so that it'd work in either mode). That's where the subclassing suggestion came from.

For more general //content-derived testing (e.g. //chrome), which isn't using //content's specific harness for content_browsertests, we don't have a good way to inject or provide the dependencies to the URLRequestContextGetters (and the builders they use) for our custom objects, thus the nasty global. My understanding is that the answer for those cases, longer term, is going to be that everything above //content will be interfacing with the network service, and will just grab the URLRequestContext's associated handle and call the test methods on it to configure it, which will naturally let us kill the global.

I don't think we want test-only methods as part of the NetworkContext interface, and I don't think we want to be modifying already created URLRequestContexts, for that matter (Having a URLRequestContext change when there's a live request scares me).

My own preference would be a single global callback that we pass NetworkContextParams on creation, and maybe pass it the name of the NetworkContext being created.  We may be able to inject it somewhere on MainBrowserParts startup, but it may be simplest just to add a global to content/browser/network_service_instance.cc, and make sure creation params go through it before being passed to the network service.

> I don't think we want test-only methods as part of the NetworkContext interface,

Isn't this effectively what we have today, with https://cs.chromium.org/chromium/src/services/network/network_context.h?rcl=5fdfbe58167e032f8da507505c78b1794c5725d5&l=76

> and I don't think we want to be modifying already created URLRequestContexts

That's what we're already doing -
 https://cs.chromium.org/chromium/src/chrome/browser/ssl/cert_verifier_browser_test.cc?rcl=5fdfbe58167e032f8da507505c78b1794c5725d5&l=58

I don't think it's unreasonable to mutate a CertVerifier after creation, which is what we're doing with https://cs.chromium.org/chromium/src/services/network/public/mojom/network_service_test.mojom?rcl=5fdfbe58167e032f8da507505c78b1794c5725d5&l=20

Unless you meant changing the associated CertVerifier, in which case, I'm 100% in agreement with you, and why I pushed back on the initial CL that did that. The only safe way to control the CertVerifier construction is at the construction of the URLRequestContext's construction - but I think mutating the CertVerifier state can be done safely (both memory and thread)

While I'm probably using the wrong terms, the goal would be that if we're running in a 'test' context, the NetworkService can initialize with a MockCertVerifier (or moral equivalent), either wrapping the real OS verifier or, more preferably, providing a fully simulated interface (e.g. no OS interaction). If we're running in real, it follows the normal Network Service initialization path.

We want that to be per-profile (or at the minimum, we should ensure our solution doesn't preclude being per-profile/do a bunch of work we'd have to scrap).

The only alternative I can think of is that we revisit which side the verification lives in, have the network service aggressively cache internally before IPC'ing back out, and have a flag for the network service to flush that cache (for the external layer to use as appropriate). But I'm totally open to other alternatives.

> Isn't this effectively what we have today, with https://cs.chromium.org/chromium/src/services/network/network_context.h?rcl=5fdfbe58167e032f8da507505c78b1794c5725d5&l=76

Looks like it, though I'm not sure that just because a pattern was added in a CL landed in the last couple months should be used as a reason to continue using that pattern.

> That's what we're already doing -
 https://cs.chromium.org/chromium/src/chrome/browser/ssl/cert_verifier_browser_test.cc?rcl=5fdfbe58167e032f8da507505c78b1794c5725d5&l=58

I thought you were suggesting injecting a CertVerifier at an arbitrary point in time.  That code is mutating a CertVerifier, not the URLRequestContext itself.  I'm fine with mutating objects that are part of the URLRequestContext (Heck, just issuing a network request does that), I'm not fine with replacing them after the URLRequestContext may have been used.

Correction, that test-only method is not part of the NetworkContext interface...It's part of network::NetworkContext, which is (basically) a private network service interface.  network::mojom::NetworkContext is the actual Mojo interface.

And digging more, it looks like the cert verifier is set *before* creating a NetworkContext, and then grabbed by the NetworkContext on construction.  This seems to be completely analogous to my suggestion, with my suggestion being more general, since it allows mocking out any component passed to the NetworkContext through the Mojo interface.

> I thought you were suggesting injecting a CertVerifier at an arbitrary point in time.

Oh! No, definitely not!

My remark on the CL was that that there's no way to do that in content_browsertests, short of doing what LayoutTests is doing (where LayoutTestsFoo is-a ShellFoo via implementation inheritance - like LayoutTestBrowserClient is-a ShellBrowserClient and so forth), which ultimately allows LayoutTests to change how the URLRequestContext is built (providing a wrapped CertVerifier that can ignore certificate errors).

Then, once we've put our own CertVerifier in place, we need a way to get it back somehow in the test so that we can mutate the state post-creation - that is, knowing that the URLRequestContext->cert_verifier() is-a MockCertVerifier*, and that we can call AddResultForCert on it.

I'm 100% on-board with not changing the object itself, and that was what I raised on the CL. Put differently:
1) We need to control how the CertVerifier is created (do we use a real or a fake)
2) We need to be able to mutate its state after creation (e.g. after creation, but before the test itself has run)

The CertVerifierBrowserTest accomplishes the former by having the network::NetworkContext ensure that the objects are created with MockCertVerifiers (rather than the real cert verifier), and accomplishes the latter by exposing IPC.

That's the model I was trying to encourage and recommend.

We can solve the (specific) content_browsertests case through the method I described, but that doesn't work for general content embedders - because they're responsible for setting up the browser context, so we can't just subclass ShellBrowser* for them.

That was my remark in Comment #3:
> Bringing the //chrome MockCertVerifier (with mojo-awareness) down to //content SGTM - definitely was thinking that was a necessary step.

For solving it for other content embedders who write browser tests, my musing in
> For more general //content-derived testing

was trying to figure out whether we can punt that until after Network S13N, in which //content-embedders (and not just content_browsertests) can use methods on the network::NetworkContext - the private network service interface - for testing, the same way //chrome is today.

And in summary, turns out rsleevi and I are on pretty much the exact same page here.  I just misunderstood the final sentence of comment #3.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5

commit 4f6427c2e56cc2625a1bf90139a78fc320a9f5a5
Author: John Abd-El-Malek <jam@chromium.org>
Date: Thu Aug 23 17:06:08 2018

Refactor CertVerifierBrowserTest so that the internal implementation moves to content/public/test.

This way it can be used by tests inside content.

Bug:  817187 
Change-Id: If1a7c1470b738296f8d121a796f5b5b2492497d6
Reviewed-on: https://chromium-review.googlesource.com/1186028
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585526}
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/autofill/autofill_interactive_uitest.cc
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/extensions/bookmark_app_navigation_browsertest.cc
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/extensions/bookmark_app_navigation_browsertest.h
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/ssl/cert_verifier_browser_test.cc
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/ssl/cert_verifier_browser_test.h
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
[add] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/content/public/test/content_cert_verifier_browser_test.cc
[add] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/content/public/test/content_cert_verifier_browser_test.h
[add] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/content/public/test/content_mock_cert_verifier.cc
[add] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/content/public/test/content_mock_cert_verifier.h
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/content/shell/browser/shell_url_request_context_getter.cc
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/content/shell/browser/shell_url_request_context_getter.h
[modify] https://crrev.com/4f6427c2e56cc2625a1bf90139a78fc320a9f5a5/content/test/BUILD.gn