Respect QuicAllowed policy for new streams

chrome/browser/safe_browsing/safe_browsing_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 
 #include <stddef.h>
 
 #include <vector>
 
@@ skipping 121 matching lines @@
 
   // Implementation for net::UrlRequestContextGetter.
   net::URLRequestContext* GetURLRequestContext() override;
   scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner()
       const override;
 
   // Shuts down any pending requests using the getter, and sets |shut_down_| to
   // true.
   void ServiceShuttingDown();
 
+  void SetQuicEnabledOnIOThread(bool quic_enabled);
+
  protected:
   ~SafeBrowsingURLRequestContextGetter() override;
 
  private:
   bool shut_down_;
 
   scoped_refptr<net::URLRequestContextGetter> system_context_getter_;
 
   std::unique_ptr<net::CookieStore> safe_browsing_cookie_store_;
 
@@ skipping 80 matching lines @@
 }
 
 void SafeBrowsingURLRequestContextGetter::ServiceShuttingDown() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   shut_down_ = true;
   URLRequestContextGetter::NotifyContextShuttingDown();
   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
 SafeBrowsingServiceFactory* SafeBrowsingService::factory_ = NULL;
 
 // The default SafeBrowsingServiceFactory.  Global, made a singleton so we
 // don't leak it.
 class SafeBrowsingServiceFactoryImpl : public SafeBrowsingServiceFactory {
  public:
   SafeBrowsingService* CreateSafeBrowsingService() override {
@@ skipping 130 matching lines @@
 #else
   return false;
 #endif
 }
 
 net::URLRequestContextGetter* SafeBrowsingService::url_request_context() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   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_;
 }
 
 const scoped_refptr<SafeBrowsingDatabaseManager>&
 SafeBrowsingService::database_manager() const {
   return enabled_v4_only_ ? v4_local_database_manager() : database_manager_;
 }
 
@@ skipping 327 matching lines @@
     ping_manager()->ReportThreatDetails(report);
 }
 
 void SafeBrowsingService::ProcessResourceRequest(
     const ResourceRequestInfo& request) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   services_delegate_->ProcessResourceRequest(&request);
 }
 
 }  // namespace safe_browsing