Respect QuicAllowed policy for new streams

chrome/browser/profiles/net_http_session_params_observer.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[mmenke, 2017/01/09 16:46:04]

2017, new files should not use the (c).

[pmarko, 2017/01/09 19:53:12]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/profiles/net_http_session_params_observer.h"
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/io_thread.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/browser/browser_thread.h"
+
+#include "net/http/http_network_session.h"

[mmenke, 2017/01/09 16:46:04]

Not needed - included in header

[pmarko, 2017/01/09 19:53:12]

Done.

+
+using content::BrowserThread;
+
+namespace {
+
+void ApplySettingsOnIOThread(
+    net::HttpNetworkSession::ParamsUpdate params_update,
+    NetHttpSessionParamsObserver::UpdateNetParamsCallback callback,
+    IOThread* io_thread,
+    bool quic_allowed_for_profile) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Never re-enable QUIC if it was disallowed on startup, e.g. due to command
+  // line flags.
+  bool quic_allowed_on_startup = io_thread->NetworkSessionParams().enable_quic;

[mmenke, 2017/01/09 16:46:04]

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.

[pmarko, 2017/01/09 19:53:12]

You're right about that. See my answer on the comment below.

+  if (!quic_allowed_on_startup)

[mmenke, 2017/01/09 16:46:04]

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.

[pmarko, 2017/01/09 19:53:12]

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.

+    return;

[mmenke, 2017/01/09 16:46:04]

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?

[pmarko, 2017/01/09 19:53:12]

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.

+
+  // Make the profile adhere to its current setting by invoking its callback.
+  callback.Run(params_update);
+
+  // Globally-owned HttpNetworkSessions should only be updated if QUIC is being
+  // turned off. This "kill switch"-like behavior prevents multiple Profiles
+  // fighting over QUIC enablement for global HttpNetworkSessions.
+  if (!quic_allowed_for_profile)
+    io_thread->UpdateNetworkSessionParams(params_update);

[mmenke, 2017/01/09 16:46:04]

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);

[pmarko, 2017/01/09 19:53:12]

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.

+}
+
+}  // namespace
+
+NetHttpSessionParamsObserver::NetHttpSessionParamsObserver(
+    PrefService* prefs,
+    UpdateNetParamsCallback update_net_params_callback)
+    : update_net_params_callback_(update_net_params_callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(prefs);
+
+  base::Closure prefs_callback = base::Bind(
+      &NetHttpSessionParamsObserver::ApplySettings, base::Unretained(this));
+  quic_allowed_.Init(prefs::kQuicAllowed, prefs, prefs_callback);
+
+  ApplySettings();
+}
+
+NetHttpSessionParamsObserver::~NetHttpSessionParamsObserver() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
+
+void NetHttpSessionParamsObserver::ApplySettings() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  IOThread* io_thread = g_browser_process->io_thread();
+  if (!io_thread)
+    return;
+
+  bool quic_allowed_for_profile = true;
+  if (quic_allowed_.IsManaged())
+    quic_allowed_for_profile = *quic_allowed_;
+
+  net::HttpNetworkSession::ParamsUpdate params_update;

[mmenke, 2017/01/09 16:46:04]

We're not setting params_update.quic_allowed.  Have you tested this code
manually?

[mmenke, 2017/01/09 17:42:47]

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).

[pmarko, 2017/01/09 19:53:12]

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.

[pmarko, 2017/01/09 19:53:12]

As discussed, removing ParamsUpdate and just calling "DisableQuic" methods.

+  params_update.enable_quic_new = quic_allowed_for_profile;
+
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(ApplySettingsOnIOThread, params_update,
+                                     update_net_params_callback_, io_thread,
+                                     quic_allowed_for_profile));
+
+  // Globally-owned HttpNetworkSessions should only be updated if QUIC is being
+  // turned off. This "kill switch"-like behavior prevents multiple Profiles
+  // fighting over QUIC enablement for global HttpNetworkSessions.
+  if (!quic_allowed_for_profile && g_browser_process->safe_browsing_service())
+    g_browser_process->safe_browsing_service()->UpdateNetParams(params_update);
+}
+
+// static
+void NetHttpSessionParamsObserver::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterBooleanPref(prefs::kQuicAllowed, true);
+}