Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2546533003:
Respect QuicAllowed policy for new streams (Closed)
|
Created:
4 years ago by pmarko Modified:
3 years, 11 months ago Reviewers:
Bence, sclittle, gab, Dmitry Titov, Ryan Hamilton, Mr4D (OOO till 08-26), Nathan Parker, pastarmovj, mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, arv+watch_chromium.org, asanka Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRespect QuicAllowed policy for new streams
Introduce a mechanism for an incoming QuicAllowed cloud policy
to control QUIC protocol usage for new streams even after
IOThread initialization.
NetPrefObserver has been re-introduced to observe the change
(after it was removed in crrev.com/2140733002 and
crrev.com/2172543003).
BUG=658454
TEST=browser_tests --gtest_filter=*QuicAllowed*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2546533003
Cr-Commit-Position: refs/heads/master@{#442866}
Committed: https://chromium.googlesource.com/chromium/src/+/6ab8be2474cd5017e22a4027019546a145474207
Patch Set 1 #Patch Set 2 : Fixed accesses to iothread globals from UI thread #
Total comments: 38
Patch Set 3 : Reviewers' feedback, formatting #Patch Set 4 : Improve enable_quic_for_new_streams comment #
Total comments: 9
Patch Set 5 : Naming, formatting #
Total comments: 13
Patch Set 6 : Single variable for enable_quic #Patch Set 7 : Rebase #Patch Set 8 : Fix prerequisites for tests, format #Patch Set 9 : IOS io_thread also initializes DynamicSharedParams #
Total comments: 38
Patch Set 10 : IsQuicEnabled in HttpNetworkSession, base::MakeUnique #Patch Set 11 : HttpNetworkSessions notified of param changes #Patch Set 12 : Introduced IOThread::UpdateNetworkSessionParams #
Total comments: 10
Patch Set 13 : Reviewers' feedback #
Total comments: 12
Patch Set 14 : No details about net params in SafeBrowsingService, other simplifications #Patch Set 15 : Rebase #
Total comments: 12
Patch Set 16 : Rename observer class, move instance to ProfileIOData #Patch Set 17 : Rebase #Patch Set 18 : Reset NetHttpSessionParamsObserver on UI thread #Patch Set 19 : Move ApplySettingsOnIOThread out of NetHttpSessionParamsObserver #
Total comments: 22
Patch Set 20 : Make explicit that we are only disabling QUIC dynamically #
Total comments: 2
Patch Set 21 : Remove accidentally merged in comment #Patch Set 22 : Add more browser tests, remove unnecessary include #Patch Set 23 : Formatting in browsertest #
Total comments: 11
Patch Set 24 : Cleanup #
Total comments: 4
Patch Set 25 : Rebase to master #Patch Set 26 : Further cleanup #Patch Set 27 : Revert accidental format #Messages
Total messages: 140 (57 generated)
Description was changed from ========== Respect QuicAllowed policy for new streams Introduce a mechanism for an incoming QuicAllowed cloud policy to control QUIC protocol usage for new streams even after IOThread initialization. NetPrefObserver has been re-introduced to observe the change (after it was removed in crrev.com/2140733002 and crrev.com/2172543003). BUG=658454 TEST=browser_tests --gtest_filter=*QuicAllowed* ========== to ========== Respect QuicAllowed policy for new streams Introduce a mechanism for an incoming QuicAllowed cloud policy to control QUIC protocol usage for new streams even after IOThread initialization. NetPrefObserver has been re-introduced to observe the change (after it was removed in crrev.com/2140733002 and crrev.com/2172543003). BUG=658454 TEST=browser_tests --gtest_filter=*QuicAllowed* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pmarko@chromium.org changed reviewers: + bnc@chromium.org
As discussed in bug 658454, I've tried to resurrect NetPrefObserver to allow for QuicAllowed=false to take effect when pushed as cloud policy even after IOThread initialization. I've tried to avoid the static variables (as it used to exist for controlling SPDY) and associated problems such as having to reset the static variable before every test. Please provide me feedback if you think the "SharedParams" approach is feasible or if you think that it is too over-engineered.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/01 18:11:15, pmarko wrote: > As discussed in bug 658454, I've tried to resurrect NetPrefObserver to allow for > QuicAllowed=false to take effect when pushed as cloud policy even after IOThread > initialization. > > I've tried to avoid the static variables (as it used to exist for controlling > SPDY) and associated problems such as having to reset the static variable before > every test. > > Please provide me feedback if you think the "SharedParams" approach is feasible > or if you think that it is too over-engineered. Why Patch Set 2: There was an access to io_thread->globals() from the UI thread which broke almost all tests with DCHECK_CURRENTLY_ON active. Should have run them as Debug locally or read the doc on IOThread::globals().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for doing this. I do not think that SharedParams is an overkill: the way you implement it makes ownership very clear, I think this is the right way to do it. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:15: Please remove this empty line. (Did you remember to run git cl format?) https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:25: net::HttpNetworkSession::SharedParams* shared_params = Move this line two spaces to the left please. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:28: shared_params->enable_quic_for_new_streams = quic_allowed; Move this line two spaces to the left please. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:37: base::Closure prefs_callback = base::Bind(&NetPrefObserver::ApplySettings, Please run git cl format to reformat this line. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:52: if (io_thread) { Maybe do early return instead? if (!io_thread) { return; } bool quic_allowed = true; ... https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:60: FROM_HERE, Merge with previous line. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This is a new file. Please remove "(c)" and change the year to 2016. (This is what src/tools/boilerplate.py would generate.) https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:31: bool quic_enabled_for_new_streams = Please run git cl format to reformat this multi-line statement. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:138: class QuicAllowedPolicyIsSetToFalseAfterInit: public QuicAllowedPolicyTestBase { Space required before ":". With that, this line is too long. Use git cl format to break it up in conformance with the style guide. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:140: QuicAllowedPolicyIsSetToFalseAfterInit() : QuicAllowedPolicyTestBase() {} I believe that it is unnecessary to define this constructor; please remove it. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:144: } Move this closing brace to the previous line. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:149: POLICY_SOURCE_CLOUD, Move these two lines one space to the left please. https://codereview.chromium.org/2546533003/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:442: "quic.allowed"; Merge with previous line. https://codereview.chromium.org/2546533003/diff/20001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_network_s... net/http/http_network_session.h:221: bool is_enable_quic_for_new_streams() const; Rename to |enable_quic_for_new_streams| in accordance with other members. https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:739: if (session_->params().is_enable_quic_for_new_streams()) I believe this should be gated on both: if (session_->params().enable_quic && session_->params().enable_quic_for_new_streams()) https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:940: if (!session_->params().is_enable_quic_for_new_streams()) I believe this should be gated on both: if (!session_->params().enable_quic || !session_->params().enable_quic_for_new_streams()) https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:1040: if (!session_->params().is_enable_quic_for_new_streams() || I believe this should be gated on all three: if (!session_->params().enable_quic || !session_->params().enable_quic_for_new_streams() || session_->quic_stream_factor()->IsQuicDisabled()) {
pmarko@chromium.org changed reviewers: + pastarmovj@chromium.org
Bence: Thanks for reviewing, PTAL! You're right, I actually forgot to run git cl format, it's one of my first chromium CLs. Sorry about that. pastarmovj@chromium.org: Please review changes in policy parts. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:15: On 2016/12/02 18:53:25, Bence wrote: > Please remove this empty line. (Did you remember to run git cl format?) Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:25: net::HttpNetworkSession::SharedParams* shared_params = On 2016/12/02 18:53:25, Bence wrote: > Move this line two spaces to the left please. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:28: shared_params->enable_quic_for_new_streams = quic_allowed; On 2016/12/02 18:53:25, Bence wrote: > Move this line two spaces to the left please. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:37: base::Closure prefs_callback = base::Bind(&NetPrefObserver::ApplySettings, On 2016/12/02 18:53:25, Bence wrote: > Please run git cl format to reformat this line. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:52: if (io_thread) { On 2016/12/02 18:53:25, Bence wrote: > Maybe do early return instead? > > if (!io_thread) { > return; > } > > bool quic_allowed = true; > ... Good idea, thanks! Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.cc:60: FROM_HERE, On 2016/12/02 18:53:25, Bence wrote: > Merge with previous line. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_pref_observer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/12/02 18:53:25, Bence wrote: > This is a new file. Please remove "(c)" and change the year to 2016. (This is > what src/tools/boilerplate.py would generate.) Done. Thanks for mentioning boilerplate.py, didn't know about that yet! https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:31: bool quic_enabled_for_new_streams = On 2016/12/02 18:53:26, Bence wrote: > Please run git cl format to reformat this multi-line statement. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:138: class QuicAllowedPolicyIsSetToFalseAfterInit: public QuicAllowedPolicyTestBase { On 2016/12/02 18:53:26, Bence wrote: > Space required before ":". With that, this line is too long. Use git cl format > to break it up in conformance with the style guide. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:140: QuicAllowedPolicyIsSetToFalseAfterInit() : QuicAllowedPolicyTestBase() {} On 2016/12/02 18:53:25, Bence wrote: > I believe that it is unnecessary to define this constructor; please remove it. The problem is that all classes in this file have DISALLOW_COPY_AND_ASSIGN which as a side effect causes no default constructor to be implicitly created by the compiler... Alternatives: - remove DISALLOW_COPY_AND_ASSIGN and explicit default constructors on the classes in this file - leave it as it is What do you think? I think that DISALLOW_COPY_AND_ASSIGN is not really necessary in test fixtures (what against are we protecting against?) but I'm new so I might be wrong. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:144: } On 2016/12/02 18:53:25, Bence wrote: > Move this closing brace to the previous line. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:149: POLICY_SOURCE_CLOUD, On 2016/12/02 18:53:25, Bence wrote: > Move these two lines one space to the left please. Done. https://codereview.chromium.org/2546533003/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:442: "quic.allowed"; On 2016/12/02 18:53:26, Bence wrote: > Merge with previous line. Done. https://codereview.chromium.org/2546533003/diff/20001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_network_s... net/http/http_network_session.h:221: bool is_enable_quic_for_new_streams() const; On 2016/12/02 18:53:26, Bence wrote: > Rename to |enable_quic_for_new_streams| in accordance with other members. Done. https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:739: if (session_->params().is_enable_quic_for_new_streams()) On 2016/12/02 18:53:26, Bence wrote: > I believe this should be gated on both: > if (session_->params().enable_quic && > session_->params().enable_quic_for_new_streams()) The way I've done it in my first attempt is that enable_quic_for_new_streams() checks for that internally. So basically, enable_quic_for_new_streams() == (enable_quic && (shared_params == null || shared_params->enable_quic_for_new_streams)) Would you prefer to decouple those and have the callers explicitly do params->enable_quic && params->enable_quic_for_new_streams() ? Or do you think enable_quic_for_new_streams() should check for the "general enable_quic" implicitly as it is now? https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:940: if (!session_->params().is_enable_quic_for_new_streams()) On 2016/12/02 18:53:26, Bence wrote: > I believe this should be gated on both: > if (!session_->params().enable_quic || > !session_->params().enable_quic_for_new_streams()) See comment in http_stream_factory_impl_job.cc - I'll change all three references if you prefer to decouple enable_quic_for_new_streams() from the general enable_quic flag. https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:1040: if (!session_->params().is_enable_quic_for_new_streams() || On 2016/12/02 18:53:26, Bence wrote: > I believe this should be gated on all three: > if (!session_->params().enable_quic || > !session_->params().enable_quic_for_new_streams() || > session_->quic_stream_factor()->IsQuicDisabled()) { See comment in http_stream_factory_impl_job.cc - I'll change all three references if you prefer to decouple enable_quic_for_new_streams() from the general enable_quic flag.
net/ LGTM. Thank you. https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:140: QuicAllowedPolicyIsSetToFalseAfterInit() : QuicAllowedPolicyTestBase() {} On 2016/12/08 17:01:59, pmarko wrote: > On 2016/12/02 18:53:25, Bence wrote: > > I believe that it is unnecessary to define this constructor; please remove it. > The problem is that all classes in this file have DISALLOW_COPY_AND_ASSIGN which > as a side effect causes no default constructor to be implicitly created by the > compiler... Alternatives: > - remove DISALLOW_COPY_AND_ASSIGN and explicit default constructors on the > classes in this file > - leave it as it is > > What do you think? I think that DISALLOW_COPY_AND_ASSIGN is not really necessary > in test fixtures (what against are we protecting against?) but I'm new so I > might be wrong. Sorry, I didn't realize that. Then I think it's better just to leave it as it is. Thanks. https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:739: if (session_->params().is_enable_quic_for_new_streams()) On 2016/12/08 17:02:00, pmarko wrote: > On 2016/12/02 18:53:26, Bence wrote: > > I believe this should be gated on both: > > if (session_->params().enable_quic && > > session_->params().enable_quic_for_new_streams()) > > The way I've done it in my first attempt is that enable_quic_for_new_streams() > checks for that internally. > > So basically, enable_quic_for_new_streams() == (enable_quic && (shared_params == > null || shared_params->enable_quic_for_new_streams)) > > Would you prefer to decouple those and have the callers explicitly do > params->enable_quic && params->enable_quic_for_new_streams() ? > Or do you think enable_quic_for_new_streams() should check for the "general > enable_quic" implicitly as it is now? Okay, I see. I think what you are doing is fine.
pmarko@chromium.org changed reviewers: + gab@chromium.org, mmenke@chromium.org
gab@chromium.org: Please review changes in chrome/browser/prefs. mmenke@chromium.org: Please review changes in chrome/browser/io_thread*, chrome/browser/net, chrome/browser/profiles, chrome/browser/resources/net_internals. Thank you! https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_network_browsertest.cc:140: QuicAllowedPolicyIsSetToFalseAfterInit() : QuicAllowedPolicyTestBase() {} On 2016/12/08 17:41:59, Bence wrote: > On 2016/12/08 17:01:59, pmarko wrote: > > On 2016/12/02 18:53:25, Bence wrote: > > > I believe that it is unnecessary to define this constructor; please remove > it. > > The problem is that all classes in this file have DISALLOW_COPY_AND_ASSIGN > which > > as a side effect causes no default constructor to be implicitly created by the > > compiler... Alternatives: > > - remove DISALLOW_COPY_AND_ASSIGN and explicit default constructors on the > > classes in this file > > - leave it as it is > > > > What do you think? I think that DISALLOW_COPY_AND_ASSIGN is not really > necessary > > in test fixtures (what against are we protecting against?) but I'm new so I > > might be wrong. > > Sorry, I didn't realize that. Then I think it's better just to leave it as it > is. Thanks. Acknowledged. https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2546533003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:739: if (session_->params().is_enable_quic_for_new_streams()) On 2016/12/08 17:41:59, Bence wrote: > On 2016/12/08 17:02:00, pmarko wrote: > > On 2016/12/02 18:53:26, Bence wrote: > > > I believe this should be gated on both: > > > if (session_->params().enable_quic && > > > session_->params().enable_quic_for_new_streams()) > > > > The way I've done it in my first attempt is that enable_quic_for_new_streams() > > checks for that internally. > > > > So basically, enable_quic_for_new_streams() == (enable_quic && (shared_params > == > > null || shared_params->enable_quic_for_new_streams)) > > > > Would you prefer to decouple those and have the callers explicitly do > > params->enable_quic && params->enable_quic_for_new_streams() ? > > Or do you think enable_quic_for_new_streams() should check for the "general > > enable_quic" implicitly as it is now? > > Okay, I see. I think what you are doing is fine. OK thanks! I've added that information to the method comment in http_network_session.h
https://codereview.chromium.org/2546533003/diff/60001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/60001/net/http/http_network_s... net/http/http_network_session.h:110: // Holds parameters which can change after initialization This aspect seems much more important than the ownership model to me. RunTimeModifiableParams? DynamicSharedParams? VariableParams?
https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "quic.allowed"; I don't see any other "quic." prefs, should this just go in net.? e.g. "net.quic_allowed" ?
https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "quic.allowed"; On 2016/12/08 20:23:27, gab wrote: > I don't see any other "quic." prefs, should this just go in net.? > > e.g. "net.quic_allowed" ? Net doesn't do any command line parsing, to let it be configured on a per-URLRequestContext basis, and net can't depend on prefs. There are plenty of quic constants in hrome_switches.h.
https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "quic.allowed"; On 2016/12/08 20:34:49, mmenke wrote: > On 2016/12/08 20:23:27, gab wrote: > > I don't see any other "quic." prefs, should this just go in net.? > > > > e.g. "net.quic_allowed" ? > > Net doesn't do any command line parsing, to let it be configured on a > per-URLRequestContext basis, and net can't depend on prefs. > > There are plenty of quic constants in hrome_switches.h. I meant, not in net/, but just have the pref be prefixed by "net." -- just like the surrounding prefs here.
https://codereview.chromium.org/2546533003/diff/60001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2546533003/diff/60001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:100: {key::kHomepageLocation, prefs::kHomePage, base::Value::TYPE_STRING}, Please undo the auto-formatting here. Bonus point if you figure out some directive (magic comment) to make git cl format skip this file. https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "quic.allowed"; On 2016/12/08 21:49:45, gab wrote: > On 2016/12/08 20:34:49, mmenke wrote: > > On 2016/12/08 20:23:27, gab wrote: > > > I don't see any other "quic." prefs, should this just go in net.? > > > > > > e.g. "net.quic_allowed" ? > > > > Net doesn't do any command line parsing, to let it be configured on a > > per-URLRequestContext basis, and net can't depend on prefs. > > > > There are plenty of quic constants in hrome_switches.h. > > I meant, not in net/, but just have the pref be prefixed by "net." -- just like > the surrounding prefs here. +1 to a common prefix.
mmenke: PTAL, I've renamed "SharedParams" to "DynamicSharedParams". Do you think the name is OK? gab: PTAL, I've added net. as a prefix to the pref name but not the constant name (seems to be standard around there) pastarmovj: PTAL, auto-formatting reverted and added "magic directive". https://codereview.chromium.org/2546533003/diff/60001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2546533003/diff/60001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:100: {key::kHomepageLocation, prefs::kHomePage, base::Value::TYPE_STRING}, On 2016/12/09 11:12:54, pastarmovj wrote: > Please undo the auto-formatting here. > > Bonus point if you figure out some directive (magic comment) to make git cl > format skip this file. Done. As we use clang-format the comment clang-format off should do the trick. Not 100% sure if it always works with the diff-only formatting but seemed to work fine for this patch. https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "quic.allowed"; On 2016/12/09 11:12:54, pastarmovj wrote: > On 2016/12/08 21:49:45, gab wrote: > > On 2016/12/08 20:34:49, mmenke wrote: > > > On 2016/12/08 20:23:27, gab wrote: > > > > I don't see any other "quic." prefs, should this just go in net.? > > > > > > > > e.g. "net.quic_allowed" ? > > > > > > Net doesn't do any command line parsing, to let it be configured on a > > > per-URLRequestContext basis, and net can't depend on prefs. > > > > > > There are plenty of quic constants in hrome_switches.h. > > > > I meant, not in net/, but just have the pref be prefixed by "net." -- just > like > > the surrounding prefs here. > > +1 to a common prefix. Done. https://codereview.chromium.org/2546533003/diff/60001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/60001/net/http/http_network_s... net/http/http_network_session.h:110: // Holds parameters which can change after initialization On 2016/12/08 20:06:44, mmenke wrote: > This aspect seems much more important than the ownership model to me. > RunTimeModifiableParams? DynamicSharedParams? VariableParams? Good point - I think DynamicSharedParams is a good choice because it transfers both important aspects. Renamed.
https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... File net/http/http_network_session.cc (right): https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.cc:90: dynamic_shared_params(NULL), nullptr is now preferred (Feel free to change the others in this list, if you want, but not needed) https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.cc:142: dynamic_shared_params->enable_quic_for_new_streams); I assume we do need both this and enable_quic, and can't just merge them. https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.cc:377: return params_.enable_quic_for_new_streams(); HttpNetworkSession::IsProtocolEnabled() -> enable_quic && is_enabled_for_new_streams seems very unexpected to me. Sorry I didn't notice this before. Can we either name functions/values in some internally consistent manner, or merge the quic values? https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.h:72: // One e instance of this struct is intended to be shared accross multiple "One e" -> "One" https://codereview.chromium.org/2546533003/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2546533003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:774: destination_)) && So this overrides the new_streams setting, but not the quic enabled settings? I thin kwe need a consistent story about what the new value means. net/http/http_stream_factory_impl_job_controller.cc also uses .enable_quic in a couple places, I assume you've checked those.
https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... File net/http/http_network_session.cc (right): https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.cc:142: dynamic_shared_params->enable_quic_for_new_streams); On 2016/12/13 18:48:54, mmenke wrote: > I assume we do need both this and enable_quic, and can't just merge them. Correct. For example, |enable_quic| can be cleared by a command line flag or a field trial. In that case, QUIC should not be enabled even if the user policy changes after HttpNetworkSession initialization.
On 2016/12/13 19:24:45, Bence wrote: > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > File net/http/http_network_session.cc (right): > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > net/http/http_network_session.cc:142: > dynamic_shared_params->enable_quic_for_new_streams); > On 2016/12/13 18:48:54, mmenke wrote: > > I assume we do need both this and enable_quic, and can't just merge them. > > Correct. For example, |enable_quic| can be cleared by a command line flag or a > field trial. In that case, QUIC should not be enabled even if the user policy > changes after HttpNetworkSession initialization. I don't see why that combination has to be implemented at the net/ layer, though. Those all seem like chrome layer details, which could be merged into one bool. With two sources of truth, things get confusing, and bugs can arise.
On 2016/12/13 19:33:22, mmenke wrote: > On 2016/12/13 19:24:45, Bence wrote: > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > File net/http/http_network_session.cc (right): > > > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > net/http/http_network_session.cc:142: > > dynamic_shared_params->enable_quic_for_new_streams); > > On 2016/12/13 18:48:54, mmenke wrote: > > > I assume we do need both this and enable_quic, and can't just merge them. > > > > Correct. For example, |enable_quic| can be cleared by a command line flag or > a > > field trial. In that case, QUIC should not be enabled even if the user policy > > changes after HttpNetworkSession initialization. > > I don't see why that combination has to be implemented at the net/ layer, > though. Those all seem like chrome layer details, which could be merged into > one bool. With two sources of truth, things get confusing, and bugs can arise. Good point. I'll see if I can come up with a better way to do it. Until now it was that QUIC was either initialized at startup (then it was enabled for all times) or not (then it was disabled for all times). The "changing it dynamically" requirement makes this more challenging (because it must affect more than one already-created HttpNetworkSession), and it used to be implemented using a static class member variable for disabling SPDY by policy. I didn't like the static variable approach, so this is what I came up with. I wanted delete the original Params::enable_quic variable and rely only on the DynamicSharedParams:: variable, but then there are parts of code which create a new HttpNetworkSession, copying the Params from the old one and explicitly setting Params::enable_quic=false . Only having the DynamicSharedParams:: variable would lead to this new HttpNetworkSession needing to have its own instance of it, which I thought might get overly complex, so I went with the "Params" has one and "DynamicSharedParams" has one too. But I agree that it's not really a good thing. I'll try to implement HttpNetworkSession::Params being able to actually own its DynamicsharedParams instance so the "copy without inheriting quic_enabled" usecase would be supported with only one "is quic enabled" variable.
lgtm, I'll let you decide what to do about comment below. https://codereview.chromium.org/2546533003/diff/80001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "net.quic.allowed"; The second . will create a dictionary for this standalone pref. If we expect more net.quic.* prefs that's perhaps okay, otherwise net.quic_allowed might be less overkill.
On 2016/12/13 21:36:16, gab wrote: > lgtm, I'll let you decide what to do about comment below. prefs/ lgtm that is > > https://codereview.chromium.org/2546533003/diff/80001/chrome/common/pref_name... > File chrome/common/pref_names.cc (right): > > https://codereview.chromium.org/2546533003/diff/80001/chrome/common/pref_name... > chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "net.quic.allowed"; > The second . will create a dictionary for this standalone pref. If we expect > more net.quic.* prefs that's perhaps okay, otherwise net.quic_allowed might be > less overkill.
policy lgtm
On 2016/12/13 19:33:22, mmenke wrote: > On 2016/12/13 19:24:45, Bence wrote: > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > File net/http/http_network_session.cc (right): > > > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > net/http/http_network_session.cc:142: > > dynamic_shared_params->enable_quic_for_new_streams); > > On 2016/12/13 18:48:54, mmenke wrote: > > > I assume we do need both this and enable_quic, and can't just merge them. > > > > Correct. For example, |enable_quic| can be cleared by a command line flag or > a > > field trial. In that case, QUIC should not be enabled even if the user policy > > changes after HttpNetworkSession initialization. > > I don't see why that combination has to be implemented at the net/ layer, > though. Those all seem like chrome layer details, which could be merged into > one bool. With two sources of truth, things get confusing, and bugs can arise. mmenke: Do you think it is acceptable if it was implemented such as: - The shared instance is called "DynamicGlobalParams" and it has a member DynamicGlobalParams::enable_quic_global - The Params:: has a member enable_quic_session_local which would be an enum with the constants USE_GLOBAL, FORCE_FALSE, FORCE_TRUE - All accesses go through Params::enable_quic() which does the combination (if enable_quic_session_local==USE_GLOBAL -- use the thing from global -- otherwise, use whatever enable_quic_session_local says). Edge case: USE_GLOBAL && dynamic_global_params==null results in enable_quic() returning false. Pros: + Still relatively easy&effective to use from chrome code (no need to keep track of all created HttpNetworkSession instances, no need to re-create HttpNetworkSession instances, while still alowing single HttpNetworkSession instances to e.g. force QUIC disabled) + Naming makes it clear what those mean (the global state vs. the "overriding" local thing) Cons: - Combination still done in the net layer (but at least it should be much easier to understand) - if someone uses the net layer outside of chrome and they set enable_quic they'd need to update their code I'd think that would be an acceptable trade-off, but I'm new and not a net layer expert.
On 2016/12/14 19:52:49, pmarko wrote: > On 2016/12/13 19:33:22, mmenke wrote: > > On 2016/12/13 19:24:45, Bence wrote: > > > > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > > File net/http/http_network_session.cc (right): > > > > > > > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > > net/http/http_network_session.cc:142: > > > dynamic_shared_params->enable_quic_for_new_streams); > > > On 2016/12/13 18:48:54, mmenke wrote: > > > > I assume we do need both this and enable_quic, and can't just merge them. > > > > > > Correct. For example, |enable_quic| can be cleared by a command line flag > or > > a > > > field trial. In that case, QUIC should not be enabled even if the user > policy > > > changes after HttpNetworkSession initialization. > > > > I don't see why that combination has to be implemented at the net/ layer, > > though. Those all seem like chrome layer details, which could be merged into > > one bool. With two sources of truth, things get confusing, and bugs can > arise. > > mmenke: Do you think it is acceptable if it was implemented such as: > - The shared instance is called "DynamicGlobalParams" and it has a member > DynamicGlobalParams::enable_quic_global > - The Params:: has a member enable_quic_session_local which would be an enum > with the constants USE_GLOBAL, FORCE_FALSE, FORCE_TRUE > - All accesses go through Params::enable_quic() which does the combination (if > enable_quic_session_local==USE_GLOBAL -- use the thing from global -- otherwise, > use whatever enable_quic_session_local says). Edge case: USE_GLOBAL && > dynamic_global_params==null results in enable_quic() returning false. > > Pros: > + Still relatively easy&effective to use from chrome code (no need to keep track > of all created HttpNetworkSession instances, no need to re-create > HttpNetworkSession instances, while still alowing single HttpNetworkSession > instances to e.g. force QUIC disabled) > + Naming makes it clear what those mean (the global state vs. the "overriding" > local thing) > > Cons: > - Combination still done in the net layer (but at least it should be much easier > to understand) > - if someone uses the net layer outside of chrome and they set enable_quic > they'd need to update their code > > I'd think that would be an acceptable trade-off, but I'm new and not a net layer > expert. So where's the code that copies the session params but turns QUIC off? I'm not seeing it. Using an enum class to indicate whether to use the dynamic global param or not is an improvement, but want to make sure we really need it.
On 2016/12/14 22:26:31, mmenke (Out Dec 17 to Jan 2) wrote: > On 2016/12/14 19:52:49, pmarko wrote: > > On 2016/12/13 19:33:22, mmenke wrote: > > > On 2016/12/13 19:24:45, Bence wrote: > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > > > File net/http/http_network_session.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... > > > > net/http/http_network_session.cc:142: > > > > dynamic_shared_params->enable_quic_for_new_streams); > > > > On 2016/12/13 18:48:54, mmenke wrote: > > > > > I assume we do need both this and enable_quic, and can't just merge > them. > > > > > > > > Correct. For example, |enable_quic| can be cleared by a command line flag > > or > > > a > > > > field trial. In that case, QUIC should not be enabled even if the user > > policy > > > > changes after HttpNetworkSession initialization. > > > > > > I don't see why that combination has to be implemented at the net/ layer, > > > though. Those all seem like chrome layer details, which could be merged > into > > > one bool. With two sources of truth, things get confusing, and bugs can > > arise. > > > > mmenke: Do you think it is acceptable if it was implemented such as: > > - The shared instance is called "DynamicGlobalParams" and it has a member > > DynamicGlobalParams::enable_quic_global > > - The Params:: has a member enable_quic_session_local which would be an enum > > with the constants USE_GLOBAL, FORCE_FALSE, FORCE_TRUE > > - All accesses go through Params::enable_quic() which does the combination (if > > enable_quic_session_local==USE_GLOBAL -- use the thing from global -- > otherwise, > > use whatever enable_quic_session_local says). Edge case: USE_GLOBAL && > > dynamic_global_params==null results in enable_quic() returning false. > > > > Pros: > > + Still relatively easy&effective to use from chrome code (no need to keep > track > > of all created HttpNetworkSession instances, no need to re-create > > HttpNetworkSession instances, while still alowing single HttpNetworkSession > > instances to e.g. force QUIC disabled) > > + Naming makes it clear what those mean (the global state vs. the "overriding" > > local thing) > > > > Cons: > > - Combination still done in the net layer (but at least it should be much > easier > > to understand) > > - if someone uses the net layer outside of chrome and they set enable_quic > > they'd need to update their code > > > > I'd think that would be an acceptable trade-off, but I'm new and not a net > layer > > expert. > > So where's the code that copies the session params but turns QUIC off? I'm not > seeing it. > > Using an enum class to indicate whether to use the dynamic global param or not > is an improvement, but want to make sure we really need it. Glad that you asked - while writing up how the mechanism seems to work I found out that I was wrong about it. The case I had in mind is the URLRequestContextBuilder which gets a HttpNetworkSession::Params in URLRequestContextBuilder::SetHttpNetworkSessionComponents and takes parts of it. Then has the method "SetSpdyAndQuicEnabled" to explicitly set enable_quic, which has two callers setting it to false. BUT it turns out that if SetSpdyAndQuicEnabled is not called, enable_quic is not inherited from the passed HttpNetworkSession::Params, instead it is just false by default - so the usecase I was talking about was just living in my head. We should be good to go with just one variable and combine stuff like command line flags and policy in chrome, as you've said. I'll come back with code.
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you all again for your feedback. PTAL, I've now made sure there's only one variable which controls QUIC enablement (quic_enable). As this should be controlled by the "dynamic shared" state, it now only lives in DynamicSharedParams. A few consequences of this were: (1) I've updated tests which used to set enable_quic to first initialize DynamicSharedParams and then set DynamicSharedParams::enable_quic. (2) IOS io_thread now also initializes DynamicSharedParams (3) URLRequestContextStorage can now hold a DynamicSharedParams whose lifetime it manages, because URLRequestContextBuilder gets a "QUIC enabled" setting which is independent of the shared state in the io_thread's HttpNetworkSession. Also, with more testing, one flaw in my original NetPrefObserver implementation was revealed: In the old solution, if two users logged in in two profiles, the enable_quic of the user won whose policy was refreshed last. This was bad, and as the usecase of the customer who wants this is disabling QUIC for all time for the whole browser as soon as the "master" user's policy takes effect, I've made it that NetPrefObserver can now only set enable_quic=false. Resetting it to true requires a restart. I've commented this fact in NetPrefObserver. https://codereview.chromium.org/2546533003/diff/80001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2546533003/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:441: const char kQuicAllowed[] = "net.quic.allowed"; On 2016/12/13 21:36:16, gab wrote: > The second . will create a dictionary for this standalone pref. If we expect > more net.quic.* prefs that's perhaps okay, otherwise net.quic_allowed might be > less overkill. Good point! Changed to net.quic_allowed - Done. https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... File net/http/http_network_session.cc (right): https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.cc:90: dynamic_shared_params(NULL), On 2016/12/13 18:48:54, mmenke wrote: > nullptr is now preferred (Feel free to change the others in this list, if you > want, but not needed) Done. https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.cc:142: dynamic_shared_params->enable_quic_for_new_streams); On 2016/12/13 19:24:45, Bence wrote: > On 2016/12/13 18:48:54, mmenke wrote: > > I assume we do need both this and enable_quic, and can't just merge them. > > Correct. For example, |enable_quic| can be cleared by a command line flag or a > field trial. In that case, QUIC should not be enabled even if the user policy > changes after HttpNetworkSession initialization. Reduced to only enable_quic, the logic is now outside the net layer. https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.cc:377: return params_.enable_quic_for_new_streams(); On 2016/12/13 18:48:54, mmenke (Out Dec 17 to Jan 2) wrote: > HttpNetworkSession::IsProtocolEnabled() -> enable_quic && > is_enabled_for_new_streams seems very unexpected to me. Sorry I didn't notice > this before. Can we either name functions/values in some internally consistent > manner, or merge the quic values? Reduced to only enable_quic, the logic is now outside the net layer. https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/80001/net/http/http_network_s... net/http/http_network_session.h:72: // One e instance of this struct is intended to be shared accross multiple On 2016/12/13 18:48:54, mmenke wrote: > "One e" -> "One" Done. https://codereview.chromium.org/2546533003/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2546533003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:774: destination_)) && On 2016/12/13 18:48:54, mmenke (Out Dec 17 to Jan 2) wrote: > So this overrides the new_streams setting, but not the quic enabled settings? I > thin kwe need a consistent story about what the new value means. > > net/http/http_stream_factory_impl_job_controller.cc also uses .enable_quic in a > couple places, I assume you've checked those. Reduced to only enable_quic, the logic is now outside the net layer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pmarko@chromium.org changed reviewers: + asanka@chromium.org, gcasto@chromium.org, sclittle@chromium.org
@asanka: mmenke was in the process of reviewing the net/ parts of the issue. As he went OOO now and the issue is getting more important for a customer, would you mind reviewing the parts? @sclittle: Please review componnets/data_reduction_proxy @gcasto: Please review components/grpc_support To give you a heads-up: There is a QuicAllowed policy which can be used to disable QUIC. However it is currently only effective if QuicAllowed=false is set before IOThread construction. The purpose of this patch is to allow the QuicAllowed=false policy to turn off QUIC for new streams even after IOThread construction. The approach is to move enable_quic into a DynamicSharedParams struct, of which one instance is shared accross HttpNetworkSessions. Thank you!
Description was changed from ========== Respect QuicAllowed policy for new streams Introduce a mechanism for an incoming QuicAllowed cloud policy to control QUIC protocol usage for new streams even after IOThread initialization. NetPrefObserver has been re-introduced to observe the change (after it was removed in crrev.com/2140733002 and crrev.com/2172543003). BUG=658454 TEST=browser_tests --gtest_filter=*QuicAllowed* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Respect QuicAllowed policy for new streams Introduce a mechanism for an incoming QuicAllowed cloud policy to control QUIC protocol usage for new streams even after IOThread initialization. NetPrefObserver has been re-introduced to observe the change (after it was removed in crrev.com/2140733002 and crrev.com/2172543003). BUG=658454 TEST=browser_tests --gtest_filter=*QuicAllowed* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
pmarko@chromium.org changed reviewers: + rch@chromium.org, skuhne@chromium.org - asanka@chromium.org
@rch: mmenke was in the process of reviewing the net/ and chrome/browser/io_thread* parts of the issue. As he went OOO now, would you mind reviewing the parts? @asanka: Sorry for bothering you before, I've just noticed that you are not an owner of chrome/browser/io_thread* - my bad! @Mr4D: Please review chrome/browser/profiles*
net/ and components/network_session_configurator/ LGTM with nits. Thank you. https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:643: globals_->http_network_session_dynamic_shared_params.reset( Please consider assigning to base::MakeUnique<>() here in accordance with https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?l=44. https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.cc:36: // We'd also need to evaluated command-line switches::kDisableQuic here. s/evaluated/evaluate/ https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_impl.cc:619: net_pref_observer_.reset(new NetPrefObserver(prefs_.get())); Assign to base::MakeUnique<NetPrefObserver>(prefs_.get()). https://codereview.chromium.org/2546533003/diff/160001/ios/chrome/browser/ios... File ios/chrome/browser/ios_chrome_io_thread.mm (right): https://codereview.chromium.org/2546533003/diff/160001/ios/chrome/browser/ios... ios/chrome/browser/ios_chrome_io_thread.mm:413: globals_->http_network_session_dynamic_shared_params.reset( = base::MakeUnique https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:114: // May be nullptr Please append period to last sentence. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:216: // Evaluates if QUIC is enabled for new streams Please append period to this sentence. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:217: bool enable_quic() const; Please move this up to right after the destructor: methods (even accessors) usually come before members, see https://google.github.io/styleguide/cppguide.html#Declaration_Order. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1140: HttpNetworkSession::DynamicSharedParams dynamic_shared_params; There should be no need to create an instance here: |session_deps| already owns one and sets up |params| to point to it. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1196: HttpNetworkSession::DynamicSharedParams dynamic_shared_params; Same here. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1256: HttpNetworkSession::DynamicSharedParams dynamic_shared_params; Same here. https://codereview.chromium.org/2546533003/diff/160001/net/url_request/url_re... File net/url_request/url_request_quic_unittest.cc (right): https://codereview.chromium.org/2546533003/diff/160001/net/url_request/url_re... net/url_request/url_request_quic_unittest.cc:64: dynamic_shared_params_.reset(new HttpNetworkSession::DynamicSharedParams); = MakeUnique
https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:24: virtual ~NetPrefObserver(); Does this need to be virtual? It doesn't have a base class and I don't seen any derived classes. https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:26: static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); Can you add a comment about this method? I'm not super familiar with this code, but from the name, I'm not sure what it does. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:84: bool enable_quic; nit: quic_enabled might be a better choice since it sounds more like a state instead of like an action. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:114: // May be nullptr I'm ramping up on this CL, so sorry for the basic question. Under what circumstances should this be null? https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:115: DynamicSharedParams* dynamic_shared_params; I'm surprised not to see any cronet code which sets this. Can you confirm that cronet's QUIC-enabling code still works after this refactor? https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:217: bool enable_quic() const; The style guide prohibits methods on structs. Perhaps you could add an IsQuicEnabled() method to the session itself?
Thanks for your input! PTAL. https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:643: globals_->http_network_session_dynamic_shared_params.reset( On 2016/12/20 14:58:07, Bence wrote: > Please consider assigning to base::MakeUnique<>() here in accordance with > https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?l=44. Done. https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.cc:36: // We'd also need to evaluated command-line switches::kDisableQuic here. On 2016/12/20 14:58:07, Bence wrote: > s/evaluated/evaluate/ Done. https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:24: virtual ~NetPrefObserver(); On 2016/12/20 15:38:12, Ryan Hamilton wrote: > Does this need to be virtual? It doesn't have a base class and I don't seen any > derived classes. Right - I resurrected this from an older CL where it was virtual and didn't notice. No need for this to be virtual. Done. https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:26: static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); On 2016/12/20 15:38:12, Ryan Hamilton wrote: > Can you add a comment about this method? I'm not super familiar with this code, > but from the name, I'm not sure what it does. Good point, I've added a comment. The name seems to be a common name across the codebase with components registering "their own" prefs (see https://cs.chromium.org/search/?q=RegisterProfilePrefs&p=2&sq=package:chromiu... ). https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2546533003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_impl.cc:619: net_pref_observer_.reset(new NetPrefObserver(prefs_.get())); On 2016/12/20 14:58:07, Bence wrote: > Assign to base::MakeUnique<NetPrefObserver>(prefs_.get()). Done. https://codereview.chromium.org/2546533003/diff/160001/ios/chrome/browser/ios... File ios/chrome/browser/ios_chrome_io_thread.mm (right): https://codereview.chromium.org/2546533003/diff/160001/ios/chrome/browser/ios... ios/chrome/browser/ios_chrome_io_thread.mm:413: globals_->http_network_session_dynamic_shared_params.reset( On 2016/12/20 14:58:07, Bence wrote: > = base::MakeUnique Done. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:84: bool enable_quic; On 2016/12/20 15:38:12, Ryan Hamilton wrote: > nit: quic_enabled might be a better choice since it sounds more like a state > instead of like an action. I also thought so at first, but then I wanted to make this consistent with Params::enable_* - seeing as this used to be Params::enable_quic. What would you prefer -- consistency with Params:: members or the higher clarity of action/state semantics? https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:114: // May be nullptr On 2016/12/20 14:58:07, Bence wrote: > Please append period to last sentence. Bence: Done. Ryan: Good point! Basically, by introducing this, I've added a burden of creating and assigning the dynamic_shared_params when using HttpNetworkSession with QUIC. As I didn't want to touch all existing unit tests which create a HttpNetworkSession, I've thought of this: - When dynamic_shared_params==nullptr, use what used to be default if the user code didn't change enable_quic. - This means that existing unit tests which don't use QUIC need no changes (this remains nullptr, and the resulting behavior is enable_quic=false, which is the same as they had until now). I've added a comment clarifying this and referencing to comments on DynamicSharedParams:: members, where the defaults are commented. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:115: DynamicSharedParams* dynamic_shared_params; On 2016/12/20 15:38:12, Ryan Hamilton wrote: > I'm surprised not to see any cronet code which sets this. Can you confirm that > cronet's QUIC-enabling code still works after this refactor? Good point! Didn't think of cronet, but it should be fine - I've just checked that cronet_environment.mm uses net::URLRequestContextBuilder which sets this (see cronet_environment.mm:295 , url_request_context_config.cc:373, and url_request_context_builder.cc in this patch). https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:216: // Evaluates if QUIC is enabled for new streams On 2016/12/20 14:58:07, Bence wrote: > Please append period to this sentence. Done. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:217: bool enable_quic() const; On 2016/12/20 15:38:12, Ryan Hamilton wrote: > The style guide prohibits methods on structs. Perhaps you could add an > IsQuicEnabled() method to the session itself? Bence: Probably obselete now :) Ryan: Ah, I didn't know that, but it makes sense. Will do. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1140: HttpNetworkSession::DynamicSharedParams dynamic_shared_params; On 2016/12/20 14:58:07, Bence wrote: > There should be no need to create an instance here: |session_deps| already owns > one and sets up |params| to point to it. Good catch!! Done. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1196: HttpNetworkSession::DynamicSharedParams dynamic_shared_params; On 2016/12/20 14:58:07, Bence wrote: > Same here. Done. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1256: HttpNetworkSession::DynamicSharedParams dynamic_shared_params; On 2016/12/20 14:58:07, Bence wrote: > Same here. Done. https://codereview.chromium.org/2546533003/diff/160001/net/url_request/url_re... File net/url_request/url_request_quic_unittest.cc (right): https://codereview.chromium.org/2546533003/diff/160001/net/url_request/url_re... net/url_request/url_request_quic_unittest.cc:64: dynamic_shared_params_.reset(new HttpNetworkSession::DynamicSharedParams); On 2016/12/20 14:58:07, Bence wrote: > = MakeUnique Done.
components/data_reduction_proxy LGTM
https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:84: bool enable_quic; On 2016/12/20 18:00:37, pmarko wrote: > On 2016/12/20 15:38:12, Ryan Hamilton wrote: > > nit: quic_enabled might be a better choice since it sounds more like a state > > instead of like an action. > > I also thought so at first, but then I wanted to make this consistent with > Params::enable_* - seeing as this used to be Params::enable_quic. > > What would you prefer -- consistency with Params:: members or the higher clarity > of action/state semantics? Ah, fair enough. Let's remain consistent. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:114: // May be nullptr On 2016/12/20 18:00:37, pmarko wrote: > On 2016/12/20 14:58:07, Bence wrote: > > Please append period to last sentence. > > Bence: Done. > > Ryan: Good point! Basically, by introducing this, I've added a burden of > creating and assigning the dynamic_shared_params when using HttpNetworkSession > with QUIC. As I didn't want to touch all existing unit tests which create a > HttpNetworkSession, I've thought of this: > - When dynamic_shared_params==nullptr, use what used to be default if the user > code didn't change enable_quic. > - This means that existing unit tests which don't use QUIC need no changes (this > remains nullptr, and the resulting behavior is enable_quic=false, which is the > same as they had until now). Hm. I think you're saying that the dynamic params should never be null in production. If so, I'd strongly prefer a DCHECK(params_.dynamic_shared_params != null); in HttpNetworkSession to make sure callers do the right thing. Alas, as you point out, this means that a bunch of tests will need to change. That's annoying, but less annoying than accidentally not enabling QUIC. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:115: DynamicSharedParams* dynamic_shared_params; On 2016/12/20 18:00:37, pmarko wrote: > On 2016/12/20 15:38:12, Ryan Hamilton wrote: > > I'm surprised not to see any cronet code which sets this. Can you confirm that > > cronet's QUIC-enabling code still works after this refactor? > > Good point! Didn't think of cronet, but it should be fine - I've just checked > that cronet_environment.mm uses net::URLRequestContextBuilder which sets this > (see cronet_environment.mm:295 , url_request_context_config.cc:373, and > url_request_context_builder.cc in this patch). Ah, good point.
On 2016/12/21 21:23:58, Ryan Hamilton wrote: > Hm. I think you're saying that the dynamic params should never be null in > production. If so, I'd strongly prefer a DCHECK(params_.dynamic_shared_params != > null); in HttpNetworkSession to make sure callers do the right thing. Alas, as > you point out, this means that a bunch of tests will need to change. That's > annoying, but less annoying than accidentally not enabling QUIC. > > https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... > net/http/http_network_session.h:115: DynamicSharedParams* dynamic_shared_params; > On 2016/12/20 18:00:37, pmarko wrote: > > On 2016/12/20 15:38:12, Ryan Hamilton wrote: > > > I'm surprised not to see any cronet code which sets this. Can you confirm > that > > > cronet's QUIC-enabling code still works after this refactor? > > > > Good point! Didn't think of cronet, but it should be fine - I've just checked > > that cronet_environment.mm uses net::URLRequestContextBuilder which sets this > > (see cronet_environment.mm:295 , url_request_context_config.cc:373, and > > url_request_context_builder.cc in this patch). > > Ah, good point. You're right, but the more I think about it, I come to the impression that I'm on a wrong path with this. In the beginning the idea was to "do something similar that used to be done with SPDY", but without the static variable involved there, so this led me to the creation DynamicSharedParams as a "nicer alternative". As long as only one instance of DynamicSharedParam was supposed to exist, it was small and understandable. On the other hand it was agreeably inconsistent and weird to maintain because of the two variables involved. To be honest, the current CL makes it difficult for me to reason about how many DynamicSharedParams instances exist, when they go out of scope, who might then still refer to them, etc. and I don't like that. I'd suggest that I turn this around: - I'll revert Params. There will be no more DynamicSharedParams. Plain old boolean again. - HttpNetworkSession::IsQuicEnabled will stay, and I'll introduce HttpNetworkSession::SetQuicEnabled - Instead of changing one struct instance which is supposed to be shared by relevant HttpNetworkSessions, I'll make sure to call all relevant HttpNetworkSessions' SetQuicEnabled instead.
Argh, I quoted the wrong part of your reply. Sorry about that. I wanted to quote: On 2016/12/21 21:23:58, Ryan Hamilton wrote: > Hm. I think you're saying that the dynamic params should never be null in > production. If so, I'd strongly prefer a DCHECK(params_.dynamic_shared_params != > null); in HttpNetworkSession to make sure callers do the right thing. Alas, as > you point out, this means that a bunch of tests will need to change. That's > annoying, but less annoying than accidentally not enabling QUIC. *imagine text from my last reply here*
On 2016/12/22 20:09:38, pmarko wrote: > On 2016/12/21 21:23:58, Ryan Hamilton wrote: > > Hm. I think you're saying that the dynamic params should never be null in > > production. If so, I'd strongly prefer a DCHECK(params_.dynamic_shared_params > != > > null); in HttpNetworkSession to make sure callers do the right thing. Alas, as > > you point out, this means that a bunch of tests will need to change. That's > > annoying, but less annoying than accidentally not enabling QUIC. > > > > > https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... > > net/http/http_network_session.h:115: DynamicSharedParams* > dynamic_shared_params; > > On 2016/12/20 18:00:37, pmarko wrote: > > > On 2016/12/20 15:38:12, Ryan Hamilton wrote: > > > > I'm surprised not to see any cronet code which sets this. Can you confirm > > that > > > > cronet's QUIC-enabling code still works after this refactor? > > > > > > Good point! Didn't think of cronet, but it should be fine - I've just > checked > > > that cronet_environment.mm uses net::URLRequestContextBuilder which sets > this > > > (see cronet_environment.mm:295 , url_request_context_config.cc:373, and > > > url_request_context_builder.cc in this patch). > > > > Ah, good point. > > You're right, but the more I think about it, I come to the impression that I'm > on a wrong path with this. > > In the beginning the idea was to "do something similar that used to be done with > SPDY", but without the static variable involved there, so this led me to the > creation DynamicSharedParams as a "nicer alternative". > As long as only one instance of DynamicSharedParam was supposed to exist, it was > small and understandable. On the other hand it was agreeably inconsistent and > weird to maintain because of the two variables involved. To be honest, the > current CL makes it difficult for me to reason about how many > DynamicSharedParams instances exist, when they go out of scope, who might then > still refer to them, etc. and I don't like that. > > I'd suggest that I turn this around: > - I'll revert Params. There will be no more DynamicSharedParams. Plain old > boolean again. > - HttpNetworkSession::IsQuicEnabled will stay, and I'll introduce > HttpNetworkSession::SetQuicEnabled > - Instead of changing one struct instance which is supposed to be shared by > relevant HttpNetworkSessions, I'll make sure to call all relevant > HttpNetworkSessions' SetQuicEnabled instead. Cool!
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pmarko@chromium.org changed reviewers: + dimich@chromium.org, nparker@chromium.org
pmarko@chromium.org changed reviewers: - gcasto@chromium.org, sclittle@chromium.org
PTAL, I've inversed the logic to notify HttpNetworkSessions of parameter changes. @Dimitry: Please review google_apis/gcm/engine (The idea here is that since the QUIC enablement is dynamic, gcmclientimpl now updates its own HttpNetsorkSession with the IsQuicEnabled from the outer session) @Nathan: Please review chrome/browser/safe_browsing https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... File net/http/http_network_session.h (right): https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:84: bool enable_quic; On 2016/12/21 21:23:58, Ryan Hamilton wrote: > On 2016/12/20 18:00:37, pmarko wrote: > > On 2016/12/20 15:38:12, Ryan Hamilton wrote: > > > nit: quic_enabled might be a better choice since it sounds more like a state > > > instead of like an action. > > > > I also thought so at first, but then I wanted to make this consistent with > > Params::enable_* - seeing as this used to be Params::enable_quic. > > > > What would you prefer -- consistency with Params:: members or the higher > clarity > > of action/state semantics? > > Ah, fair enough. Let's remain consistent. (obsolete - Params has been reverted) https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:114: // May be nullptr On 2016/12/21 21:23:58, Ryan Hamilton wrote: > On 2016/12/20 18:00:37, pmarko wrote: > > On 2016/12/20 14:58:07, Bence wrote: > > > Please append period to last sentence. > > > > Bence: Done. > > > > Ryan: Good point! Basically, by introducing this, I've added a burden of > > creating and assigning the dynamic_shared_params when using HttpNetworkSession > > with QUIC. As I didn't want to touch all existing unit tests which create a > > HttpNetworkSession, I've thought of this: > > - When dynamic_shared_params==nullptr, use what used to be default if the user > > code didn't change enable_quic. > > - This means that existing unit tests which don't use QUIC need no changes > (this > > remains nullptr, and the resulting behavior is enable_quic=false, which is the > > same as they had until now). > > Hm. I think you're saying that the dynamic params should never be null in > production. If so, I'd strongly prefer a DCHECK(params_.dynamic_shared_params != > null); in HttpNetworkSession to make sure callers do the right thing. Alas, as > you point out, this means that a bunch of tests will need to change. That's > annoying, but less annoying than accidentally not enabling QUIC. See my reply in comments thread - you're right that this is not really consitent, and making it consistent and thus requiring everyone to set this is too big a change for what I'm trying to achieve in my opinion. Thus: Changed the logic: No more DynamicSharedParams, instead HttpNetworkSession now has SetQuicEnabled which is called from outside. https://codereview.chromium.org/2546533003/diff/160001/net/http/http_network_... net/http/http_network_session.h:115: DynamicSharedParams* dynamic_shared_params; On 2016/12/21 21:23:58, Ryan Hamilton wrote: > On 2016/12/20 18:00:37, pmarko wrote: > > On 2016/12/20 15:38:12, Ryan Hamilton wrote: > > > I'm surprised not to see any cronet code which sets this. Can you confirm > that > > > cronet's QUIC-enabling code still works after this refactor? > > > > Good point! Didn't think of cronet, but it should be fine - I've just checked > > that cronet_environment.mm uses net::URLRequestContextBuilder which sets this > > (see cronet_environment.mm:295 , url_request_context_config.cc:373, and > > url_request_context_builder.cc in this patch). > > Ah, good point. (obsolete - Params has been reverted)
net/ LGTM. I'm not thrilled about NetPrefObserver keeping a WeakPtr of ProfileImpl (in the callback), but this seems to be a small price to pay for overall simplicity. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.cc:65: // Apply settings for globally owned HttpNetworkSessions Please end this sentence with a period. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:40: explicit NetPrefObserver(PrefService* prefs, Do not use |explicit|, because constructor takes more than one argument. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_impl.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_impl.h:135: // Observers prefs_ for net parameter changes s/Observers/Observes/ Append period at end of sentence. s/prefs_/|prefs_|/ (Variable names are conventionally enclosed in "|" in comments.) https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_io_data.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_io_data.h:72: // Called on UI thread thread when net parameters change Append period to comment. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.h:105: // Called on UI thread thread when net parameters change Period.
net/ LGTM. I'm not thrilled about NetPrefObserver keeping a WeakPtr of ProfileImpl (in the callback), but this seems to be a small price to pay for overall simplicity. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.cc:65: // Apply settings for globally owned HttpNetworkSessions Please end this sentence with a period. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:40: explicit NetPrefObserver(PrefService* prefs, Do not use |explicit|, because constructor takes more than one argument. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_impl.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_impl.h:135: // Observers prefs_ for net parameter changes s/Observers/Observes/ Append period at end of sentence. s/prefs_/|prefs_|/ (Variable names are conventionally enclosed in "|" in comments.) https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_io_data.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_io_data.h:72: // Called on UI thread thread when net parameters change Append period to comment. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.h:105: // Called on UI thread thread when net parameters change Period.
@Bence: Thank you for the comments! (1) Actually, in profile_impl.cc:628, I've made the callback with a base::Unretained(this) so there should be no actual WeakPtr involved. The idea was that since ProfileImpl owns NetPrefObserver, ProfileImpl always lives longer so there's no need to use a WeakPtr. (2) I'll have to write a script checking for periods on comments I've touched :) https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.cc:65: // Apply settings for globally owned HttpNetworkSessions On 2016/12/26 18:47:56, Bence wrote: > Please end this sentence with a period. Done. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:40: explicit NetPrefObserver(PrefService* prefs, On 2016/12/26 18:47:56, Bence wrote: > Do not use |explicit|, because constructor takes more than one argument. Done. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_impl.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_impl.h:135: // Observers prefs_ for net parameter changes On 2016/12/26 18:47:56, Bence wrote: > s/Observers/Observes/ > Append period at end of sentence. > s/prefs_/|prefs_|/ (Variable names are conventionally enclosed in "|" in > comments.) Done. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_io_data.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_io_data.h:72: // Called on UI thread thread when net parameters change On 2016/12/26 18:47:56, Bence wrote: > Append period to comment. Done. https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/2546533003/diff/220001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.h:105: // Called on UI thread thread when net parameters change On 2016/12/26 18:47:56, Bence wrote: > Period. Done.
Happy new year to everyone! @those who might be back from vacation: I'd appreciate it if you could take a look at this issue again. Thanks!
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.cc:24: NetPrefObserver::NetPrefObserver( Do we really need a class for this? Profile[Impl]IOData already has a bunch of code to use individual prefs on the IO thread. Admittedly, most of them are polled rather than pushed, but BooleanPrefMember does support that, even if ProfileIOData is not currently using that capability.
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_impl.cc:1296: io_data_.UpdateNetParams(net_params_change); BUG: There are potentially multiple profiles. IOData shouldn't just follow the parameters of the one which had its QUIC status most recently changed. Not sure how group policy is normally applied to global objects.
@mmenke: Thank you for your comments, please see my replies below! https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.cc:24: NetPrefObserver::NetPrefObserver( On 2017/01/03 18:47:26, mmenke wrote: > Do we really need a class for this? Profile[Impl]IOData already has a bunch of > code to use individual prefs on the IO thread. Admittedly, most of them are > polled rather than pushed, but BooleanPrefMember does support that, even if > ProfileIOData is not currently using that capability. Hm, I don't really have a strong opinion on this. Pro for separate class: The logic in ApplySettings got a bit more involved. Pro for integration into Profile: we could probably avoid the callback altogether. https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_impl.cc:1296: io_data_.UpdateNetParams(net_params_change); On 2017/01/03 18:49:40, mmenke wrote: > BUG: There are potentially multiple profiles. IOData shouldn't just follow the > parameters of the one which had its QUIC status most recently changed. Not sure > how group policy is normally applied to global objects. As far as I can see, this is not actually global - every Profile(Impl) has its own io_data_ - it's an instance of a ProfileIOData subclass (here, ProfileImplIOData) owned by the profile. Or am I missing something here? But yes, there are global HttpNetworkSessions owned by io_thread / g_browser_process->safe_browsing_service() , these are updated directly from NetPrefObserver, so your concern does indeed apply to these. That's why NetPrefObserver only propagates changes true->false, see net_pref_observer.cc:58. This way, changes false->true require a restart, but that's OK with the customer needing this so it might be a good compromise. If we didn't do that, we'd have to keep a look at all profiles, && their quicallowed all together and pass that to the globally-owned HttpNetworkSessions, or something like that.
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_impl.cc:1296: io_data_.UpdateNetParams(net_params_change); On 2017/01/03 19:30:49, pmarko wrote: > On 2017/01/03 18:49:40, mmenke wrote: > > BUG: There are potentially multiple profiles. IOData shouldn't just follow > the > > parameters of the one which had its QUIC status most recently changed. Not > sure > > how group policy is normally applied to global objects. > > As far as I can see, this is not actually global - every Profile(Impl) has its > own io_data_ - it's an instance of a ProfileIOData subclass (here, > ProfileImplIOData) owned by the profile. Or am I missing something here? > > But yes, there are global HttpNetworkSessions owned by io_thread / > g_browser_process->safe_browsing_service() , these are updated directly from > NetPrefObserver, so your concern does indeed apply to these. > > That's why NetPrefObserver only propagates changes true->false, see > net_pref_observer.cc:58. This way, changes false->true require a restart, but > that's OK with the customer needing this so it might be a good compromise. If we > didn't do that, we'd have to keep a look at all profiles, && their quicallowed > all together and pass that to the globally-owned HttpNetworkSessions, or > something like that. Oops, sorry, was thinking this was IOData...Where is IOData's corresponding method called?
On 2017/01/03 19:35:42, mmenke wrote: > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > File chrome/browser/profiles/profile_impl.cc (right): > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > chrome/browser/profiles/profile_impl.cc:1296: > io_data_.UpdateNetParams(net_params_change); > On 2017/01/03 19:30:49, pmarko wrote: > > On 2017/01/03 18:49:40, mmenke wrote: > > > BUG: There are potentially multiple profiles. IOData shouldn't just follow > > the > > > parameters of the one which had its QUIC status most recently changed. Not > > sure > > > how group policy is normally applied to global objects. > > > > As far as I can see, this is not actually global - every Profile(Impl) has its > > own io_data_ - it's an instance of a ProfileIOData subclass (here, > > ProfileImplIOData) owned by the profile. Or am I missing something here? > > > > But yes, there are global HttpNetworkSessions owned by io_thread / > > g_browser_process->safe_browsing_service() , these are updated directly from > > NetPrefObserver, so your concern does indeed apply to these. > > > > That's why NetPrefObserver only propagates changes true->false, see > > net_pref_observer.cc:58. This way, changes false->true require a restart, but > > that's OK with the customer needing this so it might be a good compromise. If > we > > didn't do that, we'd have to keep a look at all profiles, && their quicallowed > > all together and pass that to the globally-owned HttpNetworkSessions, or > > something like that. > > Oops, sorry, was thinking this was IOData...Where is IOData's corresponding > method called? Hm, I haven't come across an IOData yet in my (still short) time with chromium, and codesearch isn't particularly helpful there either. Do you mean IOThread's globals? If yes, this is done through: NetPrefObserver::ApplySettings() - this does the true->false guard I mentioned -> NetPrefObserver::ApplySettingsOnIOThread() -> IOThread::UpdateNetworkSessionParam
On 2017/01/03 20:05:57, pmarko wrote: > On 2017/01/03 19:35:42, mmenke wrote: > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > File chrome/browser/profiles/profile_impl.cc (right): > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > chrome/browser/profiles/profile_impl.cc:1296: > > io_data_.UpdateNetParams(net_params_change); > > On 2017/01/03 19:30:49, pmarko wrote: > > > On 2017/01/03 18:49:40, mmenke wrote: > > > > BUG: There are potentially multiple profiles. IOData shouldn't just > follow > > > the > > > > parameters of the one which had its QUIC status most recently changed. > Not > > > sure > > > > how group policy is normally applied to global objects. > > > > > > As far as I can see, this is not actually global - every Profile(Impl) has > its > > > own io_data_ - it's an instance of a ProfileIOData subclass (here, > > > ProfileImplIOData) owned by the profile. Or am I missing something here? > > > > > > But yes, there are global HttpNetworkSessions owned by io_thread / > > > g_browser_process->safe_browsing_service() , these are updated directly from > > > NetPrefObserver, so your concern does indeed apply to these. > > > > > > That's why NetPrefObserver only propagates changes true->false, see > > > net_pref_observer.cc:58. This way, changes false->true require a restart, > but > > > that's OK with the customer needing this so it might be a good compromise. > If > > we > > > didn't do that, we'd have to keep a look at all profiles, && their > quicallowed > > > all together and pass that to the globally-owned HttpNetworkSessions, or > > > something like that. > > > > Oops, sorry, was thinking this was IOData...Where is IOData's corresponding > > method called? > > Hm, I haven't come across an IOData yet in my (still short) time with chromium, > and codesearch isn't particularly helpful there either. Do you mean IOThread's > globals? If yes, this is done through: > NetPrefObserver::ApplySettings() - this does the true->false guard I mentioned > -> NetPrefObserver::ApplySettingsOnIOThread() > -> IOThread::UpdateNetworkSessionParam Sorry, I meant IOThread's globals, right. Thanks!
On 2017/01/03 20:09:58, mmenke wrote: > On 2017/01/03 20:05:57, pmarko wrote: > > On 2017/01/03 19:35:42, mmenke wrote: > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > File chrome/browser/profiles/profile_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > chrome/browser/profiles/profile_impl.cc:1296: > > > io_data_.UpdateNetParams(net_params_change); > > > On 2017/01/03 19:30:49, pmarko wrote: > > > > On 2017/01/03 18:49:40, mmenke wrote: > > > > > BUG: There are potentially multiple profiles. IOData shouldn't just > > follow > > > > the > > > > > parameters of the one which had its QUIC status most recently changed. > > Not > > > > sure > > > > > how group policy is normally applied to global objects. > > > > > > > > As far as I can see, this is not actually global - every Profile(Impl) has > > its > > > > own io_data_ - it's an instance of a ProfileIOData subclass (here, > > > > ProfileImplIOData) owned by the profile. Or am I missing something here? > > > > > > > > But yes, there are global HttpNetworkSessions owned by io_thread / > > > > g_browser_process->safe_browsing_service() , these are updated directly > from > > > > NetPrefObserver, so your concern does indeed apply to these. > > > > > > > > That's why NetPrefObserver only propagates changes true->false, see > > > > net_pref_observer.cc:58. This way, changes false->true require a restart, > > but > > > > that's OK with the customer needing this so it might be a good compromise. > > If > > > we > > > > didn't do that, we'd have to keep a look at all profiles, && their > > quicallowed > > > > all together and pass that to the globally-owned HttpNetworkSessions, or > > > > something like that. > > > > > > Oops, sorry, was thinking this was IOData...Where is IOData's corresponding > > > method called? > > > > Hm, I haven't come across an IOData yet in my (still short) time with > chromium, > > and codesearch isn't particularly helpful there either. Do you mean IOThread's > > globals? If yes, this is done through: > > NetPrefObserver::ApplySettings() - this does the true->false guard I mentioned > > -> NetPrefObserver::ApplySettingsOnIOThread() > > -> IOThread::UpdateNetworkSessionParam > > Sorry, I meant IOThread's globals, right. Thanks! So yea...I was wrong about the path, but we are having individual profiles tell the IOThread to use their configuration, possibly fighting with each other.
On 2017/01/03 20:11:34, mmenke wrote: > On 2017/01/03 20:09:58, mmenke wrote: > > On 2017/01/03 20:05:57, pmarko wrote: > > > On 2017/01/03 19:35:42, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > File chrome/browser/profiles/profile_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > chrome/browser/profiles/profile_impl.cc:1296: > > > > io_data_.UpdateNetParams(net_params_change); > > > > On 2017/01/03 19:30:49, pmarko wrote: > > > > > On 2017/01/03 18:49:40, mmenke wrote: > > > > > > BUG: There are potentially multiple profiles. IOData shouldn't just > > > follow > > > > > the > > > > > > parameters of the one which had its QUIC status most recently changed. > > > > Not > > > > > sure > > > > > > how group policy is normally applied to global objects. > > > > > > > > > > As far as I can see, this is not actually global - every Profile(Impl) > has > > > its > > > > > own io_data_ - it's an instance of a ProfileIOData subclass (here, > > > > > ProfileImplIOData) owned by the profile. Or am I missing something here? > > > > > > > > > > But yes, there are global HttpNetworkSessions owned by io_thread / > > > > > g_browser_process->safe_browsing_service() , these are updated directly > > from > > > > > NetPrefObserver, so your concern does indeed apply to these. > > > > > > > > > > That's why NetPrefObserver only propagates changes true->false, see > > > > > net_pref_observer.cc:58. This way, changes false->true require a > restart, > > > but > > > > > that's OK with the customer needing this so it might be a good > compromise. > > > If > > > > we > > > > > didn't do that, we'd have to keep a look at all profiles, && their > > > quicallowed > > > > > all together and pass that to the globally-owned HttpNetworkSessions, or > > > > > something like that. > > > > > > > > Oops, sorry, was thinking this was IOData...Where is IOData's > corresponding > > > > method called? > > > > > > Hm, I haven't come across an IOData yet in my (still short) time with > > chromium, > > > and codesearch isn't particularly helpful there either. Do you mean > IOThread's > > > globals? If yes, this is done through: > > > NetPrefObserver::ApplySettings() - this does the true->false guard I > mentioned > > > -> NetPrefObserver::ApplySettingsOnIOThread() > > > -> IOThread::UpdateNetworkSessionParam > > > > Sorry, I meant IOThread's globals, right. Thanks! > > So yea...I was wrong about the path, but we are having individual profiles tell > the IOThread to use their configuration, possibly fighting with each other. I had the same concern, that's why NetPrefObserver only forwards changes where quic_allowed_new=false (this is what I referred to as "true->false guard"). This has the effect, that: - if no profile has QuicAllowed=false policy, the globals will have quic_allowed=true (default) - if any single profile has QuicAllowed=false policy, the globals will have quic_allowed=false (this works for dynamic changes too) - if dynamically QuicAllowed=false policy is removed from all profiles which had it, globals will stay at quic_allowed=false and only a browser restart will let it return to true. This seemed like an acceptable compromise for avoiding a "fight over globals" while providing a solution for the customer's problem and avoiding complexity to re-evalute all profiles at any change.
On 2017/01/03 20:20:46, pmarko wrote: > On 2017/01/03 20:11:34, mmenke wrote: > > On 2017/01/03 20:09:58, mmenke wrote: > > > On 2017/01/03 20:05:57, pmarko wrote: > > > > On 2017/01/03 19:35:42, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > File chrome/browser/profiles/profile_impl.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > chrome/browser/profiles/profile_impl.cc:1296: > > > > > io_data_.UpdateNetParams(net_params_change); > > > > > On 2017/01/03 19:30:49, pmarko wrote: > > > > > > On 2017/01/03 18:49:40, mmenke wrote: > > > > > > > BUG: There are potentially multiple profiles. IOData shouldn't > just > > > > follow > > > > > > the > > > > > > > parameters of the one which had its QUIC status most recently > changed. > > > > > > Not > > > > > > sure > > > > > > > how group policy is normally applied to global objects. > > > > > > > > > > > > As far as I can see, this is not actually global - every Profile(Impl) > > has > > > > its > > > > > > own io_data_ - it's an instance of a ProfileIOData subclass (here, > > > > > > ProfileImplIOData) owned by the profile. Or am I missing something > here? > > > > > > > > > > > > But yes, there are global HttpNetworkSessions owned by io_thread / > > > > > > g_browser_process->safe_browsing_service() , these are updated > directly > > > from > > > > > > NetPrefObserver, so your concern does indeed apply to these. > > > > > > > > > > > > That's why NetPrefObserver only propagates changes true->false, see > > > > > > net_pref_observer.cc:58. This way, changes false->true require a > > restart, > > > > but > > > > > > that's OK with the customer needing this so it might be a good > > compromise. > > > > If > > > > > we > > > > > > didn't do that, we'd have to keep a look at all profiles, && their > > > > quicallowed > > > > > > all together and pass that to the globally-owned HttpNetworkSessions, > or > > > > > > something like that. > > > > > > > > > > Oops, sorry, was thinking this was IOData...Where is IOData's > > corresponding > > > > > method called? > > > > > > > > Hm, I haven't come across an IOData yet in my (still short) time with > > > chromium, > > > > and codesearch isn't particularly helpful there either. Do you mean > > IOThread's > > > > globals? If yes, this is done through: > > > > NetPrefObserver::ApplySettings() - this does the true->false guard I > > mentioned > > > > -> NetPrefObserver::ApplySettingsOnIOThread() > > > > -> IOThread::UpdateNetworkSessionParam > > > > > > Sorry, I meant IOThread's globals, right. Thanks! > > > > So yea...I was wrong about the path, but we are having individual profiles > tell > > the IOThread to use their configuration, possibly fighting with each other. > > I had the same concern, that's why NetPrefObserver only forwards changes where > quic_allowed_new=false (this is what I referred to as "true->false guard"). > This has the effect, that: > - if no profile has QuicAllowed=false policy, the globals will have > quic_allowed=true (default) > - if any single profile has QuicAllowed=false policy, the globals will have > quic_allowed=false > (this works for dynamic changes too) > - if dynamically QuicAllowed=false policy is removed from all profiles which had > it, globals will stay at quic_allowed=false and only a browser restart will > let it return to true. > > This seemed like an acceptable compromise for avoiding a "fight over globals" > while providing a solution for the customer's problem and avoiding complexity to > re-evalute all profiles at any change. Not going to get back to this today, but I'll get back to it tomorrow. Don't want to delay weeks over this, but a day or two more to figure things out seems not unreasonable. Want to better understand startup behavior (Can we make QUIC requests though the IOThread's context before profiles are created? What about ChromeOS guest mode on corp devices? etc), and figure out if there are pre-existing patterns for this sort of case.
On 2017/01/03 21:11:02, mmenke wrote: > On 2017/01/03 20:20:46, pmarko wrote: > > On 2017/01/03 20:11:34, mmenke wrote: > > > On 2017/01/03 20:09:58, mmenke wrote: > > > > On 2017/01/03 20:05:57, pmarko wrote: > > > > > On 2017/01/03 19:35:42, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > > File chrome/browser/profiles/profile_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > > chrome/browser/profiles/profile_impl.cc:1296: > > > > > > io_data_.UpdateNetParams(net_params_change); > > > > > > On 2017/01/03 19:30:49, pmarko wrote: > > > > > > > On 2017/01/03 18:49:40, mmenke wrote: > > > > > > > > BUG: There are potentially multiple profiles. IOData shouldn't > > just > > > > > follow > > > > > > > the > > > > > > > > parameters of the one which had its QUIC status most recently > > changed. > > > > > > > > Not > > > > > > > sure > > > > > > > > how group policy is normally applied to global objects. > > > > > > > > > > > > > > As far as I can see, this is not actually global - every > Profile(Impl) > > > has > > > > > its > > > > > > > own io_data_ - it's an instance of a ProfileIOData subclass (here, > > > > > > > ProfileImplIOData) owned by the profile. Or am I missing something > > here? > > > > > > > > > > > > > > But yes, there are global HttpNetworkSessions owned by io_thread / > > > > > > > g_browser_process->safe_browsing_service() , these are updated > > directly > > > > from > > > > > > > NetPrefObserver, so your concern does indeed apply to these. > > > > > > > > > > > > > > That's why NetPrefObserver only propagates changes true->false, see > > > > > > > net_pref_observer.cc:58. This way, changes false->true require a > > > restart, > > > > > but > > > > > > > that's OK with the customer needing this so it might be a good > > > compromise. > > > > > If > > > > > > we > > > > > > > didn't do that, we'd have to keep a look at all profiles, && their > > > > > quicallowed > > > > > > > all together and pass that to the globally-owned > HttpNetworkSessions, > > or > > > > > > > something like that. > > > > > > > > > > > > Oops, sorry, was thinking this was IOData...Where is IOData's > > > corresponding > > > > > > method called? > > > > > > > > > > Hm, I haven't come across an IOData yet in my (still short) time with > > > > chromium, > > > > > and codesearch isn't particularly helpful there either. Do you mean > > > IOThread's > > > > > globals? If yes, this is done through: > > > > > NetPrefObserver::ApplySettings() - this does the true->false guard I > > > mentioned > > > > > -> NetPrefObserver::ApplySettingsOnIOThread() > > > > > -> IOThread::UpdateNetworkSessionParam > > > > > > > > Sorry, I meant IOThread's globals, right. Thanks! > > > > > > So yea...I was wrong about the path, but we are having individual profiles > > tell > > > the IOThread to use their configuration, possibly fighting with each other. > > > > I had the same concern, that's why NetPrefObserver only forwards changes where > > quic_allowed_new=false (this is what I referred to as "true->false guard"). > > This has the effect, that: > > - if no profile has QuicAllowed=false policy, the globals will have > > quic_allowed=true (default) > > - if any single profile has QuicAllowed=false policy, the globals will have > > quic_allowed=false > > (this works for dynamic changes too) > > - if dynamically QuicAllowed=false policy is removed from all profiles which > had > > it, globals will stay at quic_allowed=false and only a browser restart will > > let it return to true. > > > > This seemed like an acceptable compromise for avoiding a "fight over globals" > > while providing a solution for the customer's problem and avoiding complexity > to > > re-evalute all profiles at any change. > > Not going to get back to this today, but I'll get back to it tomorrow. Don't > want to delay weeks over this, but a day or two more to figure things out seems > not unreasonable. > > Want to better understand startup behavior (Can we make QUIC requests though the > IOThread's context before profiles are created? What about ChromeOS guest mode > on corp devices? etc), and figure out if there are pre-existing patterns for > this sort of case. Thanks for the heads-up and for your time! Regarding the concrete questions, here's my understanding: - Can we make QUIC requests though the IOThread's context before profiles are created? -> As far as I see, in theory yes. quic_allowed is true by default, so e.g. any communication on the sign-in screen on a corp device could make QUIC requests. - What about ChromeOS guest mode on corp devices? -> Case 1: Guest user logs in, then user with QuicAllowed=false logs in Guest user will be able to make QUIC requests. -> case 2: User with QuicAllowed=false logs in, then Guest user logs in Guest user will not be able to make QUIC requests. The reason for this behavior is that guest user's parameters are copied from globals on Profile creation. Basically, a user logging in with QuicAllowed=false also disables QUIC for subsequently created profiles. That behavior is fine for the usecases people want to use this policy for. - if there are pre-existing patterns for this sort of case -> the only similar thing I've seen is what Bence pointed me to -- see removed code in https://codereview.chromium.org/2140733002 (this is where NetPrefObserver came from :-) ).
On 2017/01/03 21:35:26, pmarko wrote: > On 2017/01/03 21:11:02, mmenke wrote: > > On 2017/01/03 20:20:46, pmarko wrote: > > > On 2017/01/03 20:11:34, mmenke wrote: > > > > On 2017/01/03 20:09:58, mmenke wrote: > > > > > On 2017/01/03 20:05:57, pmarko wrote: > > > > > > On 2017/01/03 19:35:42, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > > > File chrome/browser/profiles/profile_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > > > chrome/browser/profiles/profile_impl.cc:1296: > > > > > > > io_data_.UpdateNetParams(net_params_change); > > > > > > > On 2017/01/03 19:30:49, pmarko wrote: > > > > > > > > On 2017/01/03 18:49:40, mmenke wrote: > > > > > > > > > BUG: There are potentially multiple profiles. IOData shouldn't > > > just > > > > > > follow > > > > > > > > the > > > > > > > > > parameters of the one which had its QUIC status most recently > > > changed. > > > > > > > > > > Not > > > > > > > > sure > > > > > > > > > how group policy is normally applied to global objects. > > > > > > > > > > > > > > > > As far as I can see, this is not actually global - every > > Profile(Impl) > > > > has > > > > > > its > > > > > > > > own io_data_ - it's an instance of a ProfileIOData subclass (here, > > > > > > > > ProfileImplIOData) owned by the profile. Or am I missing something > > > here? > > > > > > > > > > > > > > > > But yes, there are global HttpNetworkSessions owned by io_thread / > > > > > > > > g_browser_process->safe_browsing_service() , these are updated > > > directly > > > > > from > > > > > > > > NetPrefObserver, so your concern does indeed apply to these. > > > > > > > > > > > > > > > > That's why NetPrefObserver only propagates changes true->false, > see > > > > > > > > net_pref_observer.cc:58. This way, changes false->true require a > > > > restart, > > > > > > but > > > > > > > > that's OK with the customer needing this so it might be a good > > > > compromise. > > > > > > If > > > > > > > we > > > > > > > > didn't do that, we'd have to keep a look at all profiles, && their > > > > > > quicallowed > > > > > > > > all together and pass that to the globally-owned > > HttpNetworkSessions, > > > or > > > > > > > > something like that. > > > > > > > > > > > > > > Oops, sorry, was thinking this was IOData...Where is IOData's > > > > corresponding > > > > > > > method called? > > > > > > > > > > > > Hm, I haven't come across an IOData yet in my (still short) time with > > > > > chromium, > > > > > > and codesearch isn't particularly helpful there either. Do you mean > > > > IOThread's > > > > > > globals? If yes, this is done through: > > > > > > NetPrefObserver::ApplySettings() - this does the true->false guard I > > > > mentioned > > > > > > -> NetPrefObserver::ApplySettingsOnIOThread() > > > > > > -> IOThread::UpdateNetworkSessionParam > > > > > > > > > > Sorry, I meant IOThread's globals, right. Thanks! > > > > > > > > So yea...I was wrong about the path, but we are having individual profiles > > > tell > > > > the IOThread to use their configuration, possibly fighting with each > other. > > > > > > I had the same concern, that's why NetPrefObserver only forwards changes > where > > > quic_allowed_new=false (this is what I referred to as "true->false guard"). > > > This has the effect, that: > > > - if no profile has QuicAllowed=false policy, the globals will have > > > quic_allowed=true (default) > > > - if any single profile has QuicAllowed=false policy, the globals will have > > > quic_allowed=false > > > (this works for dynamic changes too) > > > - if dynamically QuicAllowed=false policy is removed from all profiles which > > had > > > it, globals will stay at quic_allowed=false and only a browser restart > will > > > let it return to true. > > > > > > This seemed like an acceptable compromise for avoiding a "fight over > globals" > > > while providing a solution for the customer's problem and avoiding > complexity > > to > > > re-evalute all profiles at any change. > > > > Not going to get back to this today, but I'll get back to it tomorrow. Don't > > want to delay weeks over this, but a day or two more to figure things out > seems > > not unreasonable. > > > > Want to better understand startup behavior (Can we make QUIC requests though > the > > IOThread's context before profiles are created? What about ChromeOS guest > mode > > on corp devices? etc), and figure out if there are pre-existing patterns for > > this sort of case. > > Thanks for the heads-up and for your time! > > Regarding the concrete questions, here's my understanding: > - Can we make QUIC requests though the IOThread's context before profiles are > created? > -> As far as I see, in theory yes. quic_allowed is true by default, so e.g. any > communication on the sign-in screen on a corp device could make QUIC requests. > > - What about ChromeOS guest mode on corp devices? > -> Case 1: Guest user logs in, then user with QuicAllowed=false logs in > Guest user will be able to make QUIC requests. > -> case 2: User with QuicAllowed=false logs in, then Guest user logs in > Guest user will not be able to make QUIC requests. > The reason for this behavior is that guest user's parameters are copied > from globals on Profile creation. Basically, a user logging in with > QuicAllowed=false also disables QUIC for subsequently created profiles. > That behavior is fine for the usecases people want to use this policy for. > > - if there are pre-existing patterns for this sort of case > -> the only similar thing I've seen is what Bence pointed me to -- see removed > code in https://codereview.chromium.org/2140733002 (this is where > NetPrefObserver came from :-) ). IOThread's constructor takes in a policy service that is apparently an aggregate of all available policies. Can it just use that, instead of using the pref (Which I think comes from the policy service)?
On 2017/01/04 18:54:38, mmenke wrote: > On 2017/01/03 21:35:26, pmarko wrote: > > On 2017/01/03 21:11:02, mmenke wrote: > > > On 2017/01/03 20:20:46, pmarko wrote: > > > > On 2017/01/03 20:11:34, mmenke wrote: > > > > > On 2017/01/03 20:09:58, mmenke wrote: > > > > > > On 2017/01/03 20:05:57, pmarko wrote: > > > > > > > On 2017/01/03 19:35:42, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > > > > File chrome/browser/profiles/profile_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/profile... > > > > > > > > chrome/browser/profiles/profile_impl.cc:1296: > > > > > > > > io_data_.UpdateNetParams(net_params_change); > > > > > > > > On 2017/01/03 19:30:49, pmarko wrote: > > > > > > > > > On 2017/01/03 18:49:40, mmenke wrote: > > > > > > > > > > BUG: There are potentially multiple profiles. IOData > shouldn't > > > > just > > > > > > > follow > > > > > > > > > the > > > > > > > > > > parameters of the one which had its QUIC status most recently > > > > changed. > > > > > > > > > > > > Not > > > > > > > > > sure > > > > > > > > > > how group policy is normally applied to global objects. > > > > > > > > > > > > > > > > > > As far as I can see, this is not actually global - every > > > Profile(Impl) > > > > > has > > > > > > > its > > > > > > > > > own io_data_ - it's an instance of a ProfileIOData subclass > (here, > > > > > > > > > ProfileImplIOData) owned by the profile. Or am I missing > something > > > > here? > > > > > > > > > > > > > > > > > > But yes, there are global HttpNetworkSessions owned by io_thread > / > > > > > > > > > g_browser_process->safe_browsing_service() , these are updated > > > > directly > > > > > > from > > > > > > > > > NetPrefObserver, so your concern does indeed apply to these. > > > > > > > > > > > > > > > > > > That's why NetPrefObserver only propagates changes true->false, > > see > > > > > > > > > net_pref_observer.cc:58. This way, changes false->true require a > > > > > restart, > > > > > > > but > > > > > > > > > that's OK with the customer needing this so it might be a good > > > > > compromise. > > > > > > > If > > > > > > > > we > > > > > > > > > didn't do that, we'd have to keep a look at all profiles, && > their > > > > > > > quicallowed > > > > > > > > > all together and pass that to the globally-owned > > > HttpNetworkSessions, > > > > or > > > > > > > > > something like that. > > > > > > > > > > > > > > > > Oops, sorry, was thinking this was IOData...Where is IOData's > > > > > corresponding > > > > > > > > method called? > > > > > > > > > > > > > > Hm, I haven't come across an IOData yet in my (still short) time > with > > > > > > chromium, > > > > > > > and codesearch isn't particularly helpful there either. Do you mean > > > > > IOThread's > > > > > > > globals? If yes, this is done through: > > > > > > > NetPrefObserver::ApplySettings() - this does the true->false guard I > > > > > mentioned > > > > > > > -> NetPrefObserver::ApplySettingsOnIOThread() > > > > > > > -> IOThread::UpdateNetworkSessionParam > > > > > > > > > > > > Sorry, I meant IOThread's globals, right. Thanks! > > > > > > > > > > So yea...I was wrong about the path, but we are having individual > profiles > > > > tell > > > > > the IOThread to use their configuration, possibly fighting with each > > other. > > > > > > > > I had the same concern, that's why NetPrefObserver only forwards changes > > where > > > > quic_allowed_new=false (this is what I referred to as "true->false > guard"). > > > > This has the effect, that: > > > > - if no profile has QuicAllowed=false policy, the globals will have > > > > quic_allowed=true (default) > > > > - if any single profile has QuicAllowed=false policy, the globals will > have > > > > quic_allowed=false > > > > (this works for dynamic changes too) > > > > - if dynamically QuicAllowed=false policy is removed from all profiles > which > > > had > > > > it, globals will stay at quic_allowed=false and only a browser restart > > will > > > > let it return to true. > > > > > > > > This seemed like an acceptable compromise for avoiding a "fight over > > globals" > > > > while providing a solution for the customer's problem and avoiding > > complexity > > > to > > > > re-evalute all profiles at any change. > > > > > > Not going to get back to this today, but I'll get back to it tomorrow. > Don't > > > want to delay weeks over this, but a day or two more to figure things out > > seems > > > not unreasonable. > > > > > > Want to better understand startup behavior (Can we make QUIC requests though > > the > > > IOThread's context before profiles are created? What about ChromeOS guest > > mode > > > on corp devices? etc), and figure out if there are pre-existing patterns > for > > > this sort of case. > > > > Thanks for the heads-up and for your time! > > > > Regarding the concrete questions, here's my understanding: > > - Can we make QUIC requests though the IOThread's context before profiles are > > created? > > -> As far as I see, in theory yes. quic_allowed is true by default, so e.g. > any > > communication on the sign-in screen on a corp device could make QUIC > requests. > > > > - What about ChromeOS guest mode on corp devices? > > -> Case 1: Guest user logs in, then user with QuicAllowed=false logs in > > Guest user will be able to make QUIC requests. > > -> case 2: User with QuicAllowed=false logs in, then Guest user logs in > > Guest user will not be able to make QUIC requests. > > The reason for this behavior is that guest user's parameters are copied > > from globals on Profile creation. Basically, a user logging in with > > QuicAllowed=false also disables QUIC for subsequently created profiles. > > That behavior is fine for the usecases people want to use this policy for. > > > > - if there are pre-existing patterns for this sort of case > > -> the only similar thing I've seen is what Bence pointed me to -- see > removed > > code in https://codereview.chromium.org/2140733002 (this is where > > NetPrefObserver came from :-) ). > > IOThread's constructor takes in a policy service that is apparently an aggregate > of all available policies. Can it just use that, instead of using the pref > (Which I think comes from the policy service)? Good idea - the PolicyService coming into IOThread consists of the browser-wide policies, which also seem to include the "primary user's" policies on chromeos, which would be sufficient for this - as this is officially "per profile = false" policy, it is actually expected for the primary user's value to dictate quic_enabled for all profiles. I'll see if it works.
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:251: http_network_session_->SetQuicEnabled(quic_enabled); It seems like over-coupling to have this class need to know about QUIC. How about something like http_network_session_->ApplyParamsChange(net_params_change); That way any other policy changes don't need to be exposed to this file. Even better would be to have a method on http_network_session_ that would register get params updates. That way we wouldn't need to plumb it through SafeBrowsingService. And then future param changes would be a lot simpler.
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:251: http_network_session_->SetQuicEnabled(quic_enabled); On 2017/01/04 20:06:04, Nathan Parker wrote: > It seems like over-coupling to have this class need to know about QUIC. How > about something like > > http_network_session_->ApplyParamsChange(net_params_change); > > That way any other policy changes don't need to be exposed to this file. > > Even better would be to have a method on http_network_session_ that would > register get params updates. That way we wouldn't need to plumb it through > SafeBrowsingService. And then future param changes would be a lot simpler. Good point. I didn't want everyone need to #include something new, but I guess we could add a NetParamsChange struct in http_network_session.h so classes like this would not need to know about internals. On the other hand, I'm not sure I completely understand the implications of your second proposal: SafeBrowsingService creates its own URLRequestContextGetter which creates its own HttpNetworkSession. So the registration would still need to happen in this class. We would probably get rid of the UpdateNetParams method, but on the other hand we would need to pass something in for the newly-created HttpNetworkSession to register into, or am I missing something?
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:251: http_network_session_->SetQuicEnabled(quic_enabled); On 2017/01/04 20:06:04, Nathan Parker wrote: > It seems like over-coupling to have this class need to know about QUIC. How > about something like > > http_network_session_->ApplyParamsChange(net_params_change); > > That way any other policy changes don't need to be exposed to this file. > > Even better would be to have a method on http_network_session_ that would > register get params updates. That way we wouldn't need to plumb it through > SafeBrowsingService. And then future param changes would be a lot simpler. Classes, in general, shouldn't be creating their own HttpNetworkSessions. I think implementing a way to get params updates is much worse than adding code for the few consumers that do this. Don't suppose there's any chance of just getting this code to use either the main URLRequestContext, or the system one, instead, which would make all this code much cleaner.
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:251: http_network_session_->SetQuicEnabled(quic_enabled); On 2017/01/04 21:36:02, mmenke wrote: > On 2017/01/04 20:06:04, Nathan Parker wrote: > > It seems like over-coupling to have this class need to know about QUIC. How > > about something like > > > > http_network_session_->ApplyParamsChange(net_params_change); > > > > That way any other policy changes don't need to be exposed to this file. > > > > Even better would be to have a method on http_network_session_ that would > > register get params updates. That way we wouldn't need to plumb it through > > SafeBrowsingService. And then future param changes would be a lot simpler. > > Classes, in general, shouldn't be creating their own HttpNetworkSessions. I > think implementing a way to get params updates is much worse than adding code > for the few consumers that do this. > > Don't suppose there's any chance of just getting this code to use either the > main URLRequestContext, or the system one, instead, which would make all this > code much cleaner. It may be... We do need a separate cookie jar for SB requests -- would that still be possible? I think that's the only requirement. I'm not familiar with the ChannelId machinations.
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:251: http_network_session_->SetQuicEnabled(quic_enabled); On 2017/01/04 21:54:10, Nathan Parker wrote: > On 2017/01/04 21:36:02, mmenke wrote: > > On 2017/01/04 20:06:04, Nathan Parker wrote: > > > It seems like over-coupling to have this class need to know about QUIC. How > > > about something like > > > > > > http_network_session_->ApplyParamsChange(net_params_change); > > > > > > That way any other policy changes don't need to be exposed to this file. > > > > > > Even better would be to have a method on http_network_session_ that would > > > register get params updates. That way we wouldn't need to plumb it through > > > SafeBrowsingService. And then future param changes would be a lot simpler. > > > > Classes, in general, shouldn't be creating their own HttpNetworkSessions. I > > think implementing a way to get params updates is much worse than adding code > > for the few consumers that do this. > > > > Don't suppose there's any chance of just getting this code to use either the > > main URLRequestContext, or the system one, instead, which would make all this > > code much cleaner. > > It may be... We do need a separate cookie jar for SB requests -- would that > still be possible? I think that's the only requirement. I'm not familiar with > the ChannelId machinations. Why do we need a separate cookie jar for safebrowsing? No other module else has that requirement. We can't share connections with different cookie jars (Which is what the HttpNetworkSession stores, among many other things)
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:251: http_network_session_->SetQuicEnabled(quic_enabled); On 2017/01/04 21:56:56, mmenke wrote: > On 2017/01/04 21:54:10, Nathan Parker wrote: > > On 2017/01/04 21:36:02, mmenke wrote: > > > On 2017/01/04 20:06:04, Nathan Parker wrote: > > > > It seems like over-coupling to have this class need to know about QUIC. > How > > > > about something like > > > > > > > > http_network_session_->ApplyParamsChange(net_params_change); > > > > > > > > That way any other policy changes don't need to be exposed to this file. > > > > > > > > Even better would be to have a method on http_network_session_ that would > > > > register get params updates. That way we wouldn't need to plumb it > through > > > > SafeBrowsingService. And then future param changes would be a lot simpler. > > > > > > Classes, in general, shouldn't be creating their own HttpNetworkSessions. I > > > think implementing a way to get params updates is much worse than adding > code > > > for the few consumers that do this. > > > > > > Don't suppose there's any chance of just getting this code to use either the > > > main URLRequestContext, or the system one, instead, which would make all > this > > > code much cleaner. > > > > It may be... We do need a separate cookie jar for SB requests -- would that > > still be possible? I think that's the only requirement. I'm not familiar with > > the ChannelId machinations. > > Why do we need a separate cookie jar for safebrowsing? No other module else has > that requirement. We can't share connections with different cookie jars (Which > is what the HttpNetworkSession stores, among many other things) It stores the connections, that is, not the cookie jars.
https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:251: http_network_session_->SetQuicEnabled(quic_enabled); On 2017/01/04 22:02:26, mmenke wrote: > On 2017/01/04 21:56:56, mmenke wrote: > > On 2017/01/04 21:54:10, Nathan Parker wrote: > > > On 2017/01/04 21:36:02, mmenke wrote: > > > > On 2017/01/04 20:06:04, Nathan Parker wrote: > > > > > It seems like over-coupling to have this class need to know about QUIC. > > How > > > > > about something like > > > > > > > > > > http_network_session_->ApplyParamsChange(net_params_change); > > > > > > > > > > That way any other policy changes don't need to be exposed to this file. > > > > > > > > > > Even better would be to have a method on http_network_session_ that > would > > > > > register get params updates. That way we wouldn't need to plumb it > > through > > > > > SafeBrowsingService. And then future param changes would be a lot > simpler. > > > > > > > > Classes, in general, shouldn't be creating their own HttpNetworkSessions. > I > > > > think implementing a way to get params updates is much worse than adding > > code > > > > for the few consumers that do this. > > > > > > > > Don't suppose there's any chance of just getting this code to use either > the > > > > main URLRequestContext, or the system one, instead, which would make all > > this > > > > code much cleaner. > > > > > > It may be... We do need a separate cookie jar for SB requests -- would that > > > still be possible? I think that's the only requirement. I'm not familiar > with > > > the ChannelId machinations. > > > > Why do we need a separate cookie jar for safebrowsing? No other module else > has > > that requirement. We can't share connections with different cookie jars > (Which > > is what the HttpNetworkSession stores, among many other things) > > It stores the connections, that is, not the cookie jars. I wasn't around when this code was written, but from what I understand, the privacy requirements are that we never be able to correlate the cookie set by Safe Browsing connections with other Google cookies (GAIA, etc). Safe Browsing service is enabled by default and makes strong guarantees about not tracking users.
On 2017/01/04 22:57:39, Nathan Parker wrote: > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_service.cc (right): > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_service.cc:251: > http_network_session_->SetQuicEnabled(quic_enabled); > On 2017/01/04 22:02:26, mmenke wrote: > > On 2017/01/04 21:56:56, mmenke wrote: > > > On 2017/01/04 21:54:10, Nathan Parker wrote: > > > > On 2017/01/04 21:36:02, mmenke wrote: > > > > > On 2017/01/04 20:06:04, Nathan Parker wrote: > > > > > > It seems like over-coupling to have this class need to know about > QUIC. > > > How > > > > > > about something like > > > > > > > > > > > > http_network_session_->ApplyParamsChange(net_params_change); > > > > > > > > > > > > That way any other policy changes don't need to be exposed to this > file. > > > > > > > > > > > > Even better would be to have a method on http_network_session_ that > > would > > > > > > register get params updates. That way we wouldn't need to plumb it > > > through > > > > > > SafeBrowsingService. And then future param changes would be a lot > > simpler. > > > > > > > > > > Classes, in general, shouldn't be creating their own > HttpNetworkSessions. > > I > > > > > think implementing a way to get params updates is much worse than adding > > > code > > > > > for the few consumers that do this. > > > > > > > > > > Don't suppose there's any chance of just getting this code to use either > > the > > > > > main URLRequestContext, or the system one, instead, which would make all > > > this > > > > > code much cleaner. > > > > > > > > It may be... We do need a separate cookie jar for SB requests -- would > that > > > > still be possible? I think that's the only requirement. I'm not familiar > > with > > > > the ChannelId machinations. > > > > > > Why do we need a separate cookie jar for safebrowsing? No other module else > > has > > > that requirement. We can't share connections with different cookie jars > > (Which > > > is what the HttpNetworkSession stores, among many other things) > > > > It stores the connections, that is, not the cookie jars. > > I wasn't around when this code was written, but from what I understand, the > privacy requirements are that we never be able to correlate the cookie set by > Safe Browsing connections with other Google cookies (GAIA, etc). Safe Browsing > service is enabled by default and makes strong guarantees about not tracking > users. I assume it needs cookies (If not, easy to send cookie-less requests), and can't use its own domain. The system cookie store shouldn't be used by per-profile requests, since it's shared between users, but no idea how strict consumers are about that.
On 2017/01/04 23:01:48, mmenke wrote: > On 2017/01/04 22:57:39, Nathan Parker wrote: > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... > > File chrome/browser/safe_browsing/safe_browsing_service.cc (right): > > > > > https://codereview.chromium.org/2546533003/diff/240001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_service.cc:251: > > http_network_session_->SetQuicEnabled(quic_enabled); > > On 2017/01/04 22:02:26, mmenke wrote: > > > On 2017/01/04 21:56:56, mmenke wrote: > > > > On 2017/01/04 21:54:10, Nathan Parker wrote: > > > > > On 2017/01/04 21:36:02, mmenke wrote: > > > > > > On 2017/01/04 20:06:04, Nathan Parker wrote: > > > > > > > It seems like over-coupling to have this class need to know about > > QUIC. > > > > How > > > > > > > about something like > > > > > > > > > > > > > > http_network_session_->ApplyParamsChange(net_params_change); > > > > > > > > > > > > > > That way any other policy changes don't need to be exposed to this > > file. > > > > > > > > > > > > > > Even better would be to have a method on http_network_session_ that > > > would > > > > > > > register get params updates. That way we wouldn't need to plumb it > > > > through > > > > > > > SafeBrowsingService. And then future param changes would be a lot > > > simpler. > > > > > > > > > > > > Classes, in general, shouldn't be creating their own > > HttpNetworkSessions. > > > I > > > > > > think implementing a way to get params updates is much worse than > adding > > > > code > > > > > > for the few consumers that do this. > > > > > > > > > > > > Don't suppose there's any chance of just getting this code to use > either > > > the > > > > > > main URLRequestContext, or the system one, instead, which would make > all > > > > this > > > > > > code much cleaner. > > > > > > > > > > It may be... We do need a separate cookie jar for SB requests -- would > > that > > > > > still be possible? I think that's the only requirement. I'm not > familiar > > > with > > > > > the ChannelId machinations. > > > > > > > > Why do we need a separate cookie jar for safebrowsing? No other module > else > > > has > > > > that requirement. We can't share connections with different cookie jars > > > (Which > > > > is what the HttpNetworkSession stores, among many other things) > > > > > > It stores the connections, that is, not the cookie jars. > > > > I wasn't around when this code was written, but from what I understand, the > > privacy requirements are that we never be able to correlate the cookie set by > > Safe Browsing connections with other Google cookies (GAIA, etc). Safe > Browsing > > service is enabled by default and makes strong guarantees about not tracking > > users. > > I assume it needs cookies (If not, easy to send cookie-less requests), and can't > use its own domain. The system cookie store shouldn't be used by per-profile > requests, since it's shared between users, but no idea how strict consumers are > about that. In other words, I'd think the system cookie store doesn't have GAIA cookies.
Please see changeset 15. @Nathan, Matt: - Instead of NetPrefObserver::NetParamChange, there is now HttpNetworkSession::ParamsUpdate, which can be wired through SafeBrowsingService without it needing to know about QUIC specifically. - I've got rid of a few PostTasks(BrowserThread::IO, ..). @Matt: I've tried to get rid of NetPrefObserver and do it with a BooleanPrefMember in ProfileIOData as you suggested, but this got pretty ugly. The reason is updating the globally-owned HttpNetworkSessions, for which I'd need access to IOThread and SafeBrowsingService, so I'd need to switch to the UI thread again (to use g_browser_process->), or store the pointers in ProfileIOData, both of which seem kind of unpleasant. As for IOThread simply using the passed PolicyService, this PolicyService is basically for Device Policy only (on chromeos, it also has the policies of the primary user), so this would not achieve the desired behavior that if at least one Profile has QuicAllowed=false, QUIC should be disabled for global HttpNetworkSessions. I do think that the NetPrefObserver is good for making what happens explicit and doing the Profile update AND global sessions update in a sensible way.
lgtm LGTM for safe_browsing/ Matt -- Let's move the cookie-jar discussion to http://crbug.com/678653 https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:249: if (!http_network_session_) nit: could be if (http_network_session_) http_network_session_->...
lgtm for gcm.
Still not a fan of this approach, but not going to invest more time on alternatives. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:30: class NetPrefObserver { NetHttpSessionParamsObserver? There are a number of network prefs, all of the others are watched directly by ProfileIOData, rather than by this. This class also seems to belong in chrome/browser/profiles, since that's where it's used, particularly since it partially lives on the UI thread. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_impl.h (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_impl.h:128: net::HttpNetworkSession::ParamsUpdate params_update); include http_network_session.h https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_impl.h:136: std::unique_ptr<NetPrefObserver> net_pref_observer_; include <memory> https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... chrome/browser/profiles/profile_impl.cc:632: prefs_.get(), update_net_params_callback); Why can't all this be done in ProfileIOData::InitializeOnUIThread, and destroyed in ProfileIOData::ShutdownOnUIThread? Then only the IOData files need to know about this stuff. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:410: DCHECK_CURRENTLY_ON(BrowserThread::IO); Suggest making this method be called on the UI thread, and do the PostTask itself (And obviously be renamed). The fact that it lives behind the scenes on the IO thread doesn't seem like something NetPrefObserver should know or care about.
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you again for your comments! @Matt: as you've suggested, I've renamed NetPrefObserver to NetHttpSessionParamsObserver and moved it to be only managed in ProfileIOData. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/net/net... File chrome/browser/net/net_pref_observer.h (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/net/net... chrome/browser/net/net_pref_observer.h:30: class NetPrefObserver { On 2017/01/06 16:18:43, mmenke wrote: > NetHttpSessionParamsObserver? There are a number of network prefs, all of the > others are watched directly by ProfileIOData, rather than by this. > > This class also seems to belong in chrome/browser/profiles, since that's where > it's used, particularly since it partially lives on the UI thread. Done. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... File chrome/browser/profiles/off_the_record_profile_impl.h (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_impl.h:128: net::HttpNetworkSession::ParamsUpdate params_update); On 2017/01/06 16:18:43, mmenke wrote: > include http_network_session.h Not necessary anymore as NetHttpSessionParamsObserver instance was moved to ProfileIOData as suggested in your other comment. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... chrome/browser/profiles/off_the_record_profile_impl.h:136: std::unique_ptr<NetPrefObserver> net_pref_observer_; On 2017/01/06 16:18:43, mmenke wrote: > include <memory> Not necessary anymore as NetHttpSessionParamsObserver instance was moved to ProfileIOData as suggested in your other comment. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/profile... chrome/browser/profiles/profile_impl.cc:632: prefs_.get(), update_net_params_callback); On 2017/01/06 16:18:43, mmenke wrote: > Why can't all this be done in ProfileIOData::InitializeOnUIThread, and destroyed > in ProfileIOData::ShutdownOnUIThread? Then only the IOData files need to know > about this stuff. Done. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:249: if (!http_network_session_) On 2017/01/05 17:30:18, Nathan Parker wrote: > nit: could be > if (http_network_session_) > http_network_session_->... Done. https://codereview.chromium.org/2546533003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:410: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/01/06 16:18:43, mmenke wrote: > Suggest making this method be called on the UI thread, and do the PostTask > itself (And obviously be renamed). The fact that it lives behind the scenes on > the IO thread doesn't seem like something NetPrefObserver should know or care > about. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.cc (right): https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2017, new files should not use the (c). https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:20: #include "net/http/http_network_session.h" Not needed - included in header https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:35: bool quic_allowed_on_startup = io_thread->NetworkSessionParams().enable_quic; This isn't actually whether quic was allowed on startup. It's whether quic is allowed on IOThread's NetworkSession Params. In particular, if a call to this method ever turns QUIC off for the IOThread, this will always return true. There's actually a bug here, too: Suppose it's on for the IOThread. Then we create two profiles with QUIC enabled. Then we disable if for on profile. We disable it on the IOThread. Then we disable it on the other. We get false on this check, so early exit without disabling QUIC. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:36: if (!quic_allowed_on_startup) This is weird - if quic_allowed_on_startup is false, we don't apply any of th eparams in params_update. The UpdateNetworkSessionParams call below is similarly problematic. We should either act as if params_update is actually a general purpose set of dynamic params, or we should get rid of the ParamsUpdate class, and just modify the QUIC setting. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:37: return; Hrm...Sorry I missed this when making my earlier suggestion, but now we're allowing re-enabling for SafeBrowsing in contravention of the actual configuration, in this case, aren't we? https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:46: io_thread->UpdateNetworkSessionParams(params_update); I wonder if we could simplify this method to something like: // The value of this never changes, even if params_ does. // Alternatively, don't think it's the end of the world if we do what we're doing here now, // using the current value of IOThread (Does mean once it's turned off in IOThread, it can // never be enabled for a profile, but not sure that's really the end of the world). if (io_thread->QuicCompletelyDisallowed()) params_update.new_quic_enabled = false; callback.Run(params_update); // This is smart enough to not enable if params_update says it's enabled but params_ // says it's not - then the logic is in IOThread consumers, and not ever class that wants to change things. io_thread->UpdateNetworkSessionParams(params_update); https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:80: net::HttpNetworkSession::ParamsUpdate params_update; We're not setting params_update.quic_allowed. Have you tested this code manually? https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.h (right): https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2017, new files should not use the (c). https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.h:29: UpdateNetParamsCallback; Need to include callback header (base/???/callback_forward.h) https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:480: pref_service, update_net_params_callback); I believe this is a bug. We haven't even created the URLRequestContext by this point, so this will call post a task to call ApplySettingsOnIOThread(), which calls ProfileIOData::UpdateNetParamsOnIOThread, which notices that there's no URLRequestContext, so it does nothing. Then we when we actually create the URLRequestContext, it doesn't pick up on the configuration, and if it's enabled in IOThread::GetNetworkSessionParams, we're out of luck. In contravention of what I said over email earlier this morning, I think we're going to need browser tests for this code, if there's a way to set prefs or group policies before profile creation. Too many races, too easy to get wrong.
https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.cc (right): https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:80: net::HttpNetworkSession::ParamsUpdate params_update; On 2017/01/09 16:46:04, mmenke wrote: > We're not setting params_update.quic_allowed. Have you tested this code > manually? Oops, missed that we are, but we're just setting it to quic_allowed_for_profile...But then we're also sending quic_allowed_for_profile per profile to ApplySettingsOnIOThread as well. We shouldn't be sending it twice (See my earlier comment about either treating ParamsUpdate as a first class citizen, or removing it).
Thanks for your reviews. As discussed, I've made it explicit that we are only intending to disable QUIC dynamically, removed the ParamsUpdate struct, and renamed the methods to DisableQuic. I've also added comments to why it is OK if we don't update a not-yet-constructed URLRequestContext, as it will inherit its settings from IOThread's params_ which we will update. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.cc (right): https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/09 16:46:04, mmenke wrote: > 2017, new files should not use the (c). Done. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:20: #include "net/http/http_network_session.h" On 2017/01/09 16:46:04, mmenke wrote: > Not needed - included in header Done. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:35: bool quic_allowed_on_startup = io_thread->NetworkSessionParams().enable_quic; On 2017/01/09 16:46:04, mmenke wrote: > This isn't actually whether quic was allowed on startup. It's whether quic is > allowed on IOThread's NetworkSession Params. In particular, if a call to this > method ever turns QUIC off for the IOThread, this will always return true. > > There's actually a bug here, too: Suppose it's on for the IOThread. Then we > create two profiles with QUIC enabled. Then we disable if for on profile. We > disable it on the IOThread. Then we disable it on the other. We get false on > this check, so early exit without disabling QUIC. You're right about that. See my answer on the comment below. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:36: if (!quic_allowed_on_startup) On 2017/01/09 16:46:04, mmenke wrote: > This is weird - if quic_allowed_on_startup is false, we don't apply any of th > eparams in params_update. The UpdateNetworkSessionParams call below is > similarly problematic. We should either act as if params_update is actually a > general purpose set of dynamic params, or we should get rid of the ParamsUpdate > class, and just modify the QUIC setting. You're right, we should always send out an update, because the semantics of ParamsUpdate are "an update" and it does not care about its contents. I'd rather keep ParamsUpdate in, otherwise SafeBrowsingService would have to know about QUIC again.. Changed to unconditionally send out DisableQuic() when it's being disabled for the profile. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:37: return; On 2017/01/09 16:46:04, mmenke wrote: > Hrm...Sorry I missed this when making my earlier suggestion, but now we're > allowing re-enabling for SafeBrowsing in contravention of the actual > configuration, in this case, aren't we? Oh you're right, I also missed that. Probably that was the reason I had it in here in the first place. I think it's better to do everything from here. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:46: io_thread->UpdateNetworkSessionParams(params_update); On 2017/01/09 16:46:04, mmenke wrote: > I wonder if we could simplify this method to something like: > > // The value of this never changes, even if params_ does. > // Alternatively, don't think it's the end of the world if we do what we're > doing here now, > // using the current value of IOThread (Does mean once it's turned off in > IOThread, it can > // never be enabled for a profile, but not sure that's really the end of the > world). > if (io_thread->QuicCompletelyDisallowed()) > params_update.new_quic_enabled = false; > > callback.Run(params_update); > // This is smart enough to not enable if params_update says it's enabled but > params_ > // says it's not - then the logic is in IOThread consumers, and not ever class > that wants to change things. > io_thread->UpdateNetworkSessionParams(params_update); As discussed, simplified to sending out DisableQuic() as soon as QUIC has already been disabled on IOThread or is being disabled for the current profile. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:80: net::HttpNetworkSession::ParamsUpdate params_update; On 2017/01/09 16:46:04, mmenke wrote: > We're not setting params_update.quic_allowed. Have you tested this code > manually? Actually, it is setting params_update.enable_quic_new = .. in the next line - and yes, I did run this manually and it did take effect. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:80: net::HttpNetworkSession::ParamsUpdate params_update; On 2017/01/09 17:42:47, mmenke wrote: > On 2017/01/09 16:46:04, mmenke wrote: > > We're not setting params_update.quic_allowed. Have you tested this code > > manually? > > Oops, missed that we are, but we're just setting it to > quic_allowed_for_profile...But then we're also sending quic_allowed_for_profile > per profile to ApplySettingsOnIOThread as well. We shouldn't be sending it > twice (See my earlier comment about either treating ParamsUpdate as a first > class citizen, or removing it). As discussed, removing ParamsUpdate and just calling "DisableQuic" methods. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.h (right): https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/09 16:46:04, mmenke wrote: > 2017, new files should not use the (c). Done. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.h:29: UpdateNetParamsCallback; On 2017/01/09 16:46:04, mmenke wrote: > Need to include callback header (base/???/callback_forward.h) Done. https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2546533003/diff/360001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:480: pref_service, update_net_params_callback); On 2017/01/09 16:46:04, mmenke wrote: > I believe this is a bug. We haven't even created the URLRequestContext by this > point, so this will call post a task to call ApplySettingsOnIOThread(), which > calls ProfileIOData::UpdateNetParamsOnIOThread, which notices that there's no > URLRequestContext, so it does nothing. Then we when we actually create the > URLRequestContext, it doesn't pick up on the configuration, and if it's enabled > in IOThread::GetNetworkSessionParams, we're out of luck. > > In contravention of what I said over email earlier this morning, I think we're > going to need browser tests for this code, if there's a way to set prefs or > group policies before profile creation. Too many races, too easy to get wrong. This was very misleading, sorry about that. I've tried to make this more clear now. As we're only disabling QUIC now, and disabling it also disables it for IOThread, both ways work: (1) URLRequestContext already constructed: QUIC disabled through callback (2) URLRequestContext not constructed yet: QUIC disabled in IOThread's params_, which will be used for URLRequestContext construction -> QUIC disabled there. You're absolutely right that this would not work for re-enabling QUIc, and as we've discussed, I've made it obvious by naming now that we are not supporting re-enabling.
LGTM for safe_browsing/. We discussed offline that there's no need for generalization of this since the expectation is that generalization will never be reused. https://codereview.chromium.org/2546533003/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:413: // TODO(ntfschr): componentize this once BaseSafeBrowsingUIManager contains a This looks like a merge confusion.
https://codereview.chromium.org/2546533003/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2546533003/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:413: // TODO(ntfschr): componentize this once BaseSafeBrowsingUIManager contains a On 2017/01/09 20:00:21, Nathan Parker wrote: > This looks like a merge confusion. Good catch, removed.
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added more browser tests to test behavior for: - policy being set and reset - multiple profiles
Tests look really good! Some suggested cleanups, but LGTM (Happy to do a final pass after you response) https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:59: void VerifyProfileQuicEnabled(Profile* profile, bool quic_enabled_expected) { For slightly easier testing, I suggest: bool IsQuicEnabledOnUIThread(net::URLRequestContextGetter request_context) { base::RunLoop run_loop; bool is_quic_enabled= false; PostTaskAndReply(base::Bind(IsQuicEnabled(context, &is_quic_enabled)), run_loop.QuitClosure()); run_loop.Run(); return is_quic_enabled; } (And update IsQuicEnabled to take a bool pointer, which it sets, instead of returning a value). This allows you to run EXPECTS in the test fixture, which is easier to read and tells you exactly which EXPECT failed if you have more than one in a test. Also requires less code, since you don't need the above method. (Suggest using URLRequestContextGetter instead of the profile so we can check the other two contexts, and get rid of the next two methods, and can separate out checks on the three URLRequestContexts) https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:108: class AdditionalProfileCreator { optional: Could get rid of this class, and use a stack variable to track the profile, as I suggested in the quic check case. Main advantage is that you don't have the extra class object hanging around. https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:245: // See TODO. Indent here is weird. Maybe move down a line? https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:245: // See TODO. Also, which TODO? https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:345: // Effectively, QUIC is sitll disabled because QUIC re-enabling is not nit: still https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.cc (right): https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:83: quic_allowed_for_profile = *quic_allowed_; If quic_allowed_for_profile is true, do we even need to do anything? Seems like we could just do: // QUIC will be allowed by default, unless already disabled on the IOThread. // Either way, nothing to do. if (quic_allowed_for_profile) return; ... // quic_allowed_for_profile no longer needed here, and no longer need to // check io_thread->NetworkSessionParams().enable_quic on the IO thread. PostTask(...DisableQuicOnIOThread...
Cool, ready for the final round. https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:59: void VerifyProfileQuicEnabled(Profile* profile, bool quic_enabled_expected) { On 2017/01/10 18:03:38, mmenke wrote: > For slightly easier testing, I suggest: > > bool IsQuicEnabledOnUIThread(net::URLRequestContextGetter request_context) { > base::RunLoop run_loop; > bool is_quic_enabled= false; > PostTaskAndReply(base::Bind(IsQuicEnabled(context, &is_quic_enabled)), > run_loop.QuitClosure()); > run_loop.Run(); > return is_quic_enabled; > } > > (And update IsQuicEnabled to take a bool pointer, which it sets, instead of > returning a value). > > This allows you to run EXPECTS in the test fixture, which is easier to read and > tells you exactly which EXPECT failed if you have more than one in a test. Also > requires less code, since you don't need the above method. > > (Suggest using URLRequestContextGetter instead of the profile so we can check > the other two contexts, and get rid of the next two methods, and can separate > out checks on the three URLRequestContexts) Good idea. As discussed offline, I've named them IsQuicEnabled and IsQuicEnabledOnIOThread for brevity where IsQuicEnabled is used, and added DCHECK_CURRENTLY_ON calls. https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:108: class AdditionalProfileCreator { On 2017/01/10 18:03:38, mmenke wrote: > optional: Could get rid of this class, and use a stack variable to track the > profile, as I suggested in the quic check case. Main advantage is that you > don't have the extra class object hanging around. Done. https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:245: // See TODO. On 2017/01/10 18:03:38, mmenke wrote: > Also, which TODO? My bad, removed. https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:345: // Effectively, QUIC is sitll disabled because QUIC re-enabling is not On 2017/01/10 18:03:38, mmenke wrote: > nit: still Done. https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.cc (right): https://codereview.chromium.org/2546533003/diff/440001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:83: quic_allowed_for_profile = *quic_allowed_; On 2017/01/10 18:03:38, mmenke wrote: > If quic_allowed_for_profile is true, do we even need to do anything? Seems like > we could just do: > > // QUIC will be allowed by default, unless already disabled on the IOThread. > // Either way, nothing to do. > if (quic_allowed_for_profile) > return; > ... > // quic_allowed_for_profile no longer needed here, and no longer need to > // check io_thread->NetworkSessionParams().enable_quic on the IO thread. > PostTask(...DisableQuicOnIOThread... You're right, this is a historical artifact. Done.
The CQ bit was checked by pmarko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, pastarmovj@chromium.org, sclittle@chromium.org, bnc@chromium.org, dimich@chromium.org, nparker@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2546533003/#ps460001 (title: "Cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by pmarko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, sclittle@chromium.org, gab@chromium.org, dimich@chromium.org, nparker@chromium.org, pastarmovj@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2546533003/#ps480001 (title: "Rebase to master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/policy/... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:68: net::URLRequestContextGetter* safe_browsing_service_request_context() { May want to make these return scoped_refptrs, and IsQuicEnabled take a scoped_refptr, just to match the ownership model normally used with these objects. Code is perfectly correct without them, though, just think it's good to make the class's ownership model more explicit wherever it's used https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.cc (right): https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:88: base::Bind(DisableQuicOnIOThread, disable_quic_callback_, io_thread, &DisableQuicOnIOThread?
And this still LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/policy/... File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/policy/... chrome/browser/policy/policy_network_browsertest.cc:68: net::URLRequestContextGetter* safe_browsing_service_request_context() { On 2017/01/10 22:12:19, mmenke wrote: > May want to make these return scoped_refptrs, and IsQuicEnabled take a > scoped_refptr, just to match the ownership model normally used with these > objects. Code is perfectly correct without them, though, just think it's good > to make the class's ownership model more explicit wherever it's used Done. Good point, I was tricked into settling on passing it by raw pointer by system_request_context returning a raw pointer. https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/profile... File chrome/browser/profiles/net_http_session_params_observer.cc (right): https://codereview.chromium.org/2546533003/diff/460001/chrome/browser/profile... chrome/browser/profiles/net_http_session_params_observer.cc:88: base::Bind(DisableQuicOnIOThread, disable_quic_callback_, io_thread, On 2017/01/10 22:12:19, mmenke wrote: > &DisableQuicOnIOThread? Done.
The CQ bit was checked by pmarko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, sclittle@chromium.org, gab@chromium.org, dimich@chromium.org, nparker@chromium.org, pastarmovj@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2546533003/#ps520001 (title: "Revert accidental format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by pmarko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 520001, "attempt_start_ts": 1484129747458260,
"parent_rev": "16e3f3b70eefd21e2377c9f4149d875d3e638d22", "commit_rev":
"6ab8be2474cd5017e22a4027019546a145474207"}
Message was sent while issue was closed.
Description was changed from ========== Respect QuicAllowed policy for new streams Introduce a mechanism for an incoming QuicAllowed cloud policy to control QUIC protocol usage for new streams even after IOThread initialization. NetPrefObserver has been re-introduced to observe the change (after it was removed in crrev.com/2140733002 and crrev.com/2172543003). BUG=658454 TEST=browser_tests --gtest_filter=*QuicAllowed* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Respect QuicAllowed policy for new streams Introduce a mechanism for an incoming QuicAllowed cloud policy to control QUIC protocol usage for new streams even after IOThread initialization. NetPrefObserver has been re-introduced to observe the change (after it was removed in crrev.com/2140733002 and crrev.com/2172543003). BUG=658454 TEST=browser_tests --gtest_filter=*QuicAllowed* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2546533003 Cr-Commit-Position: refs/heads/master@{#442866} Committed: https://chromium.googlesource.com/chromium/src/+/6ab8be2474cd5017e22a40270195... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001) as https://chromium.googlesource.com/chromium/src/+/6ab8be2474cd5017e22a40270195... |
