Respect QuicAllowed policy for new streams

chrome/browser/safe_browsing/safe_browsing_service.cc

Index: chrome/browser/safe_browsing/safe_browsing_service.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc
index eafb8f1b42837b235832259d4c25d2f25cf5c6b1..29fe9e89ef91c466c1ac32b0019ca38991ebbd26 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service.cc
@@ -139,6 +139,8 @@ class SafeBrowsingURLRequestContextGetter
   // true.
   void ServiceShuttingDown();
 
+  void SetQuicEnabledOnIOThread(bool quic_enabled);
+
  protected:
   ~SafeBrowsingURLRequestContextGetter() override;
 
@@ -239,6 +241,16 @@ void SafeBrowsingURLRequestContextGetter::ServiceShuttingDown() {
   safe_browsing_request_context_.reset();
 }
 
+void SafeBrowsingURLRequestContextGetter::SetQuicEnabledOnIOThread(
+    bool quic_enabled) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (!http_network_session_)
+    return;
+
+  http_network_session_->SetQuicEnabled(quic_enabled);

[Nathan Parker, 2017/01/04 20:06:04]

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.

[pmarko, 2017/01/04 20:18:23]

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?

[mmenke, 2017/01/04 21:36:02]

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.

[Nathan Parker, 2017/01/04 21:54:10]

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.

[mmenke, 2017/01/04 21:56:56]

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)

[mmenke, 2017/01/04 22:02:26]

It stores the connections, that is, not the cookie jars.

[Nathan Parker, 2017/01/04 22:57:38]

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.

+}
+
 SafeBrowsingURLRequestContextGetter::~SafeBrowsingURLRequestContextGetter() {}
 
 // static
@@ -389,6 +401,17 @@ net::URLRequestContextGetter* SafeBrowsingService::url_request_context() {
   return url_request_context_getter_.get();
 }
 
+void SafeBrowsingService::UpdateNetParams(
+    NetPrefObserver::NetParamsChange net_params_change) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&SafeBrowsingURLRequestContextGetter::SetQuicEnabledOnIOThread,
+                 url_request_context_getter_,
+                 net_params_change.quic_enabled_new));
+}
+
 const scoped_refptr<SafeBrowsingUIManager>&
 SafeBrowsingService::ui_manager() const {
   return ui_manager_;