Do we need this functionality for download resumption?

Yes.  If the server isn't advertising intermediary certs, then we can't download from SSL (And it's not safe to just bypass cert validation in any case, including download resumption).  Losing DNS over HTTPS is also a privacy loss.  Cert revocation checking we presumably also want (Note that download resumption does not verify that the same cert is now being used as when we started the download)

I think we'd probably be fine for HTTP downloads, but I don't think we want to make HTTP downloads "better" than HTTPS ones in any cases, since we want to encourage use of HTTPS.

There are a bunch of things we need from policy_service and pref_service, but both are depending on the g_browser_process, I am wondering whether we can save these settings to a file and keep it updated on android. And when ServiceManager starts, we can read that file so we don't need to create g_browser_process

Matt: thanks for the explanation.

Min: can you enumerate everything needed? 

Not sure if I am missing anything, but digging through SystemNetworkContextManager, here is a list of prefs/policies we added to the NetworkContextParams and NetworkSerice

From prefservice, we need the following
General: 
  kQuickCheckEnabled, kPacHttpsUrlStrippingEnabled, kEnableReferrers;

DNS over http related: 
  kDnsOverHttpsServers, kDnsOverHttpsServerMethods, kBuiltInDnsClientEnabled;

SSLConfigServiceManagerPref related: 
  kCertRevocationCheckingEnabled, kCertRevocationCheckingRequiredLocalAnchors, kCertEnableSha1LocalAnchors, kCertEnableSymantecLegacyInfrastructure, kSSLVersionMin,
kSSLVersionMax, kTLS13Variant, kCipherSuiteBlacklist

ProxyConfigService related:
proxy_config::prefs::kProxy

From PolicyService, we need: kHttp09OnNonDefaultPortsEnabled, kQuicAllowed

In Ran's CL (https://chromium-review.googlesource.com/c/chromium/src/+/1081759), we introduce a simple pref service. It basically reads the same file which is used to create the local_state, and the simple perf store will be passed to create the local_state when full browser starts to avoid reading the file twice. I think we can expose this perf store to handle the perfservice needed in Min's comment#8.

kBuiltInDnsClientEnabled isn't for DNS over HTTP, actually, just DNS in general.

There are also a bunch of auth-related prefs.

I'm unaware of anything else we need just for the SystemURLRequestContext.

For the profile's request itself, we presumably need the data reduction proxy code, cookie blacklist/whitelists, and I'm not really sure what else.

Thanks Matt. For Auth, here are the pereferences:
kAuthSchemes
kAuthServerWhitelist, kAuthNegotiateDelegateWhitelist,kDisableAuthNegotiateCnameLookup, 
kEnableAuthNegotiatePort,
kNtlmV2Enabled,
kAuthAndroidNegotiateAccountType.

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

commit b4bba9de032e036a87ad1e163734c002ff8a7ccf
Author: Min Qin <qinmin@chromium.org>
Date: Tue Aug 21 18:26:54 2018

More CreateDefaultNetworkContextParams() to SystemNetworkContextManager

As we are planning to allow NetworkService creation before g_browser_process
creation, CreateDefaultNetworkContextParams() shouldn't be using
g_browser_process in it.
Since SystemNetworkContextManager will ultimately become a singleton and
can be created without g_browser_process, this CL moves
CreateDefaultNetworkContextParams() into SystemNetworkContextManager.
Classes can use g_browser_process->system_network_context_manager() to
access SystemNetworkContextManager for now.
Later they could use SystemNetworkContextManager::GetInstance() once
https://chromium-review.googlesource.com/c/chromium/src/+/1171798 lands.

BUG= 866028 

Change-Id: I7047682e349a47e9249fece11fc977978ed001b1
Reviewed-on: https://chromium-review.googlesource.com/1180172
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Xiangjun Zhang <xjz@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584847}
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/BUILD.gn
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/media/cast_mirroring_service_host.cc
[delete] https://crrev.com/9572c88286704503f87e49b7a0036bab234ccc79/chrome/browser/net/default_network_context_params.cc
[delete] https://crrev.com/9572c88286704503f87e49b7a0036bab234ccc79/chrome/browser/net/default_network_context_params.h
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/net/network_context_configuration_browsertest.cc
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/net/network_quality_tracker_browsertest.cc
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/net/profile_network_context_service.cc
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/net/system_network_context_manager.cc
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/net/system_network_context_manager.h
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/safe_browsing/safe_browsing_service.cc
[modify] https://crrev.com/b4bba9de032e036a87ad1e163734c002ff8a7ccf/chrome/browser/ssl/ssl_browsertest.cc

Xi's CL has landed: https://chromium-review.googlesource.com/c/chromium/src/+/1148959, this allows us to read from the user_pref_store before launching the full browser.

However, there is still a remaining issue:
g_browser_process->local_state() uses a PrefValueStore, which includes managed_prefs, extension_prefs, command_line_prefs, superuser_prefs, and recommendation_prefs,in addition to user_pref_store, which means prefs can come from different sources.

One solution would be introducing a cached_pref_store, so that we can store all the pref_values for network service from those different sources, excluding the command_line_prefs. However, one issue with that is whether the prefs can change when chrome is offline. If that is possible, then we need the policy service to be ready before we can create network service.
Android has device policy, not sure whether some of the prefs above are coming from that. Maybe the worst case scenario is download fails to resume if device policy changes while chrome is offline?

 

mmenke@, do you know if there are any prefs needed by SystemNetworkContextManager are coming from the stores other than command_line_store and user_pref_store? 

If not, we can simply use user_pref_store and command_line_store to get all the pref values.

I don't really understand the other pref store - If managed_prefs is whatever prefs an enterprise wants to put there, then I don't see how we can set things up without it, unless we support changing the pref store at runtime (And that means any downloads without Chrome would be using a different set of network prefs, which seems not great)

Chatted with the enterprise team, on Android the managed_prefs are not loaded when local_state is contructed, which means we only need user_pref_store and command_line_store to construct the default network context.

So to move forward, i think i can add an initialize() function into SystemNetworkContextManager to initialize all the pref values mentioned above, and to construct the default network context. Later when local_state is constructed, SystemNetworkContext<anager can then use the local_state to listen to pref changes.

mmenke@, does this sounds good to you?

That just sounds too regression prone to me.  If we're going to allow swapping in a new PrefService, I think we're going to want to update all settings when we do.

ok, I can add a check function to monitor the pref value changes when swapping PrefService. If something changes, i can update the network service accordingly.
In idea real world situation, swapping shouldn't cause any settings change so we don't need perform an update. Hopefully the check method will guard against future regressions.

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

commit caf7581385e44908ea4c517e3b3aff3bba858cc1
Author: Min Qin <qinmin@chromium.org>
Date: Wed Aug 29 22:08:51 2018

Remove 2 policy related prefs on android when creating default network context

QuicAllowed and Http09OnNonDefaultPortsEnabled are not part of
android policies. See https://www.chromium.org/administrators/policy-list-3.
On desktop, they are retrieved from the policy service.
On Android, we are planning to run ServiceManager only without the browser
process. As a result, calling policy_service() doesn't work when that is
enabled.
This CL simply disables the code on Android, so we won't need
to worry about them when creating default network context without
browser process.
If later on these policies are enabled to Android, we can add them to a
persistent pref store, which can be loaded without browser process.

BUG= 866028 

Change-Id: Ie992d072ff26e085f236817e7a7f08c5bb8b3cba
Reviewed-on: https://chromium-review.googlesource.com/1195794
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587308}
[modify] https://crrev.com/caf7581385e44908ea4c517e3b3aff3bba858cc1/chrome/browser/net/system_network_context_manager.cc

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

commit caa1e2eade8ea211a2d5d2512af85529c149d925
Author: Min Qin <qinmin@chromium.org>
Date: Fri Sep 14 20:56:48 2018

Allowing PrefService to add new PrefStores

When only running the service manager, a simple PrefService containing user
PrefStore and commandline PrefStore can be created before BrowserProcessImpl.
And it will be passed to the BrowserProcessImpl to create the local_state.
The implementation has already landed here:
https://chromium-review.googlesource.com/c/chromium/src/+/1148959
This CL makes it possible to migrate the simple PrefService to local state
by adding new pref stores, rather than creating the latter from scratch.
The benefit of doing this is that existing listeners to the simple PrefService
won't need to handle the case of swapping PrefServices,
re-registering PrefChangeRegistrar and default values, and verifying no
pref value changes during the swap.


Bug:  866028 
Change-Id: I35b643d27ffabd9621f21f68a3261aa3482ca173
Reviewed-on: https://chromium-review.googlesource.com/1199944
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591469}
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_notifier_impl.h
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_service.cc
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_service.h
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_service_factory.cc
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_service_factory.h
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_service_unittest.cc
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_value_store.cc
[modify] https://crrev.com/caa1e2eade8ea211a2d5d2512af85529c149d925/components/prefs/pref_value_store.h

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

commit 9a8017e15165328f36b81658ded8309f684bfe36
Author: Min Qin <qinmin@chromium.org>
Date: Tue Sep 18 20:37:06 2018

Passing ChromeFeatureListCreator to BrowserProcessImpl

This CL is based on https://chromium-review.googlesource.com/c/chromium/src/+/1199944
When ServiceManager launches, ChromeFeatureListCreator will be created to
load the user pref store and a simple local state.
Later on the user pref store will be passed to BrowserProcessImpl to
create a new local state.
This CL simplifies the process by passing ChromeFeatureListCreator
to BrowserProcessImpl, so that the simple local state can be transformed
into g_browser_process->local_state.

BUG= 866028 

Change-Id: Ia2e6f67bc6dc54f31b6efc2cdabe6c7d4d6158ae
Reviewed-on: https://chromium-review.googlesource.com/1204926
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592179}
[modify] https://crrev.com/9a8017e15165328f36b81658ded8309f684bfe36/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/9a8017e15165328f36b81658ded8309f684bfe36/chrome/browser/browser_process_impl.h
[modify] https://crrev.com/9a8017e15165328f36b81658ded8309f684bfe36/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/9a8017e15165328f36b81658ded8309f684bfe36/chrome/browser/chrome_feature_list_creator.cc
[modify] https://crrev.com/9a8017e15165328f36b81658ded8309f684bfe36/chrome/browser/chrome_feature_list_creator.h
[modify] https://crrev.com/9a8017e15165328f36b81658ded8309f684bfe36/chrome/browser/prefs/chrome_pref_service_factory.cc
[modify] https://crrev.com/9a8017e15165328f36b81658ded8309f684bfe36/chrome/browser/prefs/chrome_pref_service_factory.h

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

commit 1a8b65d821e252841dad5c0cace12876202e325a
Author: Min Qin <qinmin@chromium.org>
Date: Fri Sep 21 23:22:34 2018

Change SystemNetworkContextManager ctor to take a local state

with https://chromium-review.googlesource.com/c/chromium/src/+/1204926,
the global local state can now be constructed without BrowserProcessImpl.
And it will be passed to BrowserProcessImpl when the latter is created.
This CL changes the SystemNetworkContextManager to take a local state
param in its ctor, so that it can be created without g_browser_process.

BUG= 866028 

Change-Id: I3cc5260f45fffdf515f8fa3da6424bd6b39dacb8
Reviewed-on: https://chromium-review.googlesource.com/1235290
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593371}
[modify] https://crrev.com/1a8b65d821e252841dad5c0cace12876202e325a/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/1a8b65d821e252841dad5c0cace12876202e325a/chrome/browser/net/system_network_context_manager.cc
[modify] https://crrev.com/1a8b65d821e252841dad5c0cace12876202e325a/chrome/browser/net/system_network_context_manager.h

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

commit f9242676b241da69075950318efbb7b03e58f2a9
Author: Min Qin <qinmin@chromium.org>
Date: Mon Oct 01 22:27:51 2018

Passing local_state to ProxyConfigMonitor

SystemNetworkContextManager already takes a PrefService in its ctor.
As a memeber variable of SystemNetworkContextManager, ProxyConfigMonitor
should do the same.
This CL also fixes an issue that ProxyServiceFactory is using
BrowserThread::UI to create the system proxy service. It is possible
that BrowserThread is not created if ServiceManager is started alone.
Switching to use base::ThreadTaskRunnerHandle::Get() instead.

BUG= 866028 

Change-Id: I6a33310e94a6050ca0bd22bdd716660aea790b58
Reviewed-on: https://chromium-review.googlesource.com/1255144
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595594}
[modify] https://crrev.com/f9242676b241da69075950318efbb7b03e58f2a9/chrome/browser/net/proxy_config_monitor.cc
[modify] https://crrev.com/f9242676b241da69075950318efbb7b03e58f2a9/chrome/browser/net/proxy_config_monitor.h
[modify] https://crrev.com/f9242676b241da69075950318efbb7b03e58f2a9/chrome/browser/net/proxy_service_factory.cc
[modify] https://crrev.com/f9242676b241da69075950318efbb7b03e58f2a9/chrome/browser/net/system_network_context_manager.cc
[modify] https://crrev.com/f9242676b241da69075950318efbb7b03e58f2a9/chrome/browser/safe_browsing/safe_browsing_service.cc

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

commit 8caab1dc729f94fe62d1ba3b5bb9ffecf03f57be
Author: Min Qin <qinmin@chromium.org>
Date: Wed Oct 03 17:28:13 2018

Add static method to allow creating global SystemNetworkContextManager on demand

When running network service without launching full browser process,
ChromeContentBrowserClient::OnNetworkServiceCreated() will be called
without g_browser_process. This CL allows us to create
SystemNetworkContextManager on demand and services depending on the
network service can call SystemNetworkContextManager::GetInstance()
to access it.

BUG= 866028 

Change-Id: I9a557921e1bb164ecd62c766dd698137498c0a47
Reviewed-on: https://chromium-review.googlesource.com/1256207
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596269}
[modify] https://crrev.com/8caab1dc729f94fe62d1ba3b5bb9ffecf03f57be/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/8caab1dc729f94fe62d1ba3b5bb9ffecf03f57be/chrome/browser/browser_process_impl.h
[modify] https://crrev.com/8caab1dc729f94fe62d1ba3b5bb9ffecf03f57be/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/8caab1dc729f94fe62d1ba3b5bb9ffecf03f57be/chrome/browser/chrome_feature_list_creator.h
[modify] https://crrev.com/8caab1dc729f94fe62d1ba3b5bb9ffecf03f57be/chrome/browser/net/system_network_context_manager.cc
[modify] https://crrev.com/8caab1dc729f94fe62d1ba3b5bb9ffecf03f57be/chrome/browser/net/system_network_context_manager.h

This is mostly fixed for now