Keep track of ChannelIDService in CookieStore

net/url_request/url_request_context_builder.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 "net/url_request/url_request_context_builder.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
@@ skipping 323 matching lines @@
     http_auth_handler_factory_ =
         HttpAuthHandlerRegistryFactory::CreateDefault(context->host_resolver());
   }
 
   storage->set_http_auth_handler_factory(std::move(http_auth_handler_factory_));
 
   if (cookie_store_) {
     storage->set_cookie_store(std::move(cookie_store_));
     storage->set_channel_id_service(std::move(channel_id_service_));
   } else {
-    storage->set_cookie_store(
-        make_scoped_ptr(new CookieMonster(nullptr, nullptr)));
+    scoped_ptr<CookieStore> cookie_store(new CookieMonster(nullptr, nullptr));
     // TODO(mmenke):  This always creates a file thread, even when it ends up
     // not being used.  Consider lazily creating the thread.
-    storage->set_channel_id_service(make_scoped_ptr(new ChannelIDService(
-        new DefaultChannelIDStore(NULL), context->GetFileTaskRunner())));
+    scoped_ptr<ChannelIDService> channel_id_service(new ChannelIDService(
+        new DefaultChannelIDStore(NULL), context->GetFileTaskRunner()));
+    cookie_store->SetChannelIDServiceID(channel_id_service->GetUniqueID());
+    storage->set_cookie_store(std::move(cookie_store));
+    storage->set_channel_id_service(std::move(channel_id_service));

[mmenke, 2016/03/18 22:12:56]

Suggestion:  Replace URLRequestContext::set_cookie_store and
URLRequestContext::set_channel_id_service with a single method to set both.

Have it dcheck that the channel store ID either matches that of the
ChannelIDService, or is not yet set (And if not set, set it).   Make a similar
method in URLRequestContextStorage.

Then have a separate set_cookie_store_borked method (Or whatever) that just
takes a cookie store and sets has_known_mismatched_cookie_store_ to true.

[nharper, 2016/03/18 22:31:09]

The problem with this is that resetting the cookie store on an URLRequestContext
will result in using the new cookie store, but resetting the channel id service
will result in the URLRequestContext using whatever channel id service was in
the HttpNetworkSession (assuming that's already been set). I tried calling
set_channel_id_service in the safe browsing code, but because the
HttpNetworkSession had already been constructed with the system channel id
service, the call to URLRequestContext::set_channel_id_service didn't do
anything useful.

[mmenke, 2016/03/18 22:36:02]

This is true...But this code has the exact same problem.

The change I suggested makes sure that people who are setting just the cookie
store at least run into the problem, though they may try and incorrect fix (You
could also make the same DCHECK check the HttpNetworkSession, if we have one,
and then you can catch everything, if you add a DCHECK to the method to set the
HttpNetworkSession as well).

[nharper, 2016/03/18 23:09:01]

Ok, I think I'm understanding how this could work.

For the common case where a new URLRequestContext is being made, and
set_cookie_store and set_channel_id_service are being called in sequence, change
those to a new URLRequestContext::set_cookie_store_and_channel_id_service
method, replacing the previous two.

In the places where a cookie store is being changed, either the new
set_cookie_store_and_channel_id_service method should be used (and the developer
needs to figure out how to fix the HttpNetworkSession as well), or use an escape
hatch URLRequestContext:set_cookie_store_borked.

I don't think we can add DCHECKs to catch everything (i.e. checking that the
CookieStore's channel id service matches the HttpNetworkTransaction's channel id
service), because in the case where we're setting new objects, one of them has
to be set first (or we need a variant of set_cookie_store_and_channel_id_service
that also takes a HttpTransactionFactory).

[mmenke, 2016/03/22 15:46:04]

We could, if we required the HttpNetworkSession be cleared and then set...but
yea, that's rather ugly.

   }
 
   if (sdch_enabled_) {
     storage->set_sdch_manager(scoped_ptr<net::SdchManager>(new SdchManager()));
   }
 
   storage->set_transport_security_state(
       make_scoped_ptr(new TransportSecurityState()));
   if (!transport_security_persister_path_.empty()) {
     context->set_transport_security_persister(
@@ skipping 146 matching lines @@
     }
     url_request_interceptors_.clear();
   }
   storage->set_job_factory(std::move(top_job_factory));
   // TODO(willchan): Support sdch.
 
   return std::move(context);
 }
 
 }  // namespace net