Keep track of ChannelIDService in CookieStore

net/url_request/url_request_http_job.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_http_job.h"
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 47 matching lines @@
 static const char kAvailDictionaryHeader[] = "Avail-Dictionary";
 
 namespace {
 
 // True if the request method is "safe" (per section 4.2.1 of RFC 7231).
 bool IsMethodSafe(const std::string& method) {
   return method == "GET" || method == "HEAD" || method == "OPTIONS" ||
          method == "TRACE";
 }
 
 void LogChannelIDAndCookieStores(const net::URLRequestContext* context,

[mmenke, 2016/03/28 17:54:00]

This function really needs a comment., or the histogram fields do.

[nharper, 2016/03/29 00:30:20]

Done. Comments added for both.

                                  const net::SSLInfo& ssl_info) {
   if (!ssl_info.channel_id_sent)
     return;
   // This enum is used for an UMA histogram - don't reuse or renumber entries.
   enum {
     // Value 0 was removed (CID_EPHEMERAL_COOKIE_EPHEMERAL)
     CID_EPHEMERAL_COOKIE_PERSISTENT = 1,
     CID_PERSISTENT_COOKIE_EPHEMERAL = 2,
     // Value 3 was removed (CID_PERSISTENT_COOKIE_PERSISTENT)
     NO_COOKIE_STORE = 4,
     NO_CHANNEL_ID_STORE = 5,
     KNOWN_MISMATCH = 6,
     EPHEMERAL_MATCH = 7,
     EPHEMERAL_MISMATCH = 8,
     PERSISTENT_MATCH = 9,
     PERSISTENT_MISMATCH = 10,
+    EPHEMERAL_UNKNOWN = 11,
+    PERSISTENT_UNKNOWN = 12,
     EPHEMERALITY_MAX
   } ephemerality;
   const net::HttpNetworkSession::Params* params =
       context->GetNetworkSessionParams();
   net::CookieStore* cookie_store = context->cookie_store();

[mmenke, 2016/03/28 17:54:00]

We allow NULL cookie stores.

[nharper, 2016/03/29 00:30:20]

I check for a null cookie store a few lines down.

   if (params == nullptr || params->channel_id_service == nullptr) {
     ephemerality = NO_CHANNEL_ID_STORE;
   } else if (cookie_store == nullptr) {
     ephemerality = NO_COOKIE_STORE;
   } else if (params->channel_id_service->GetChannelIDStore()->IsEphemeral()) {
     if (cookie_store->IsEphemeral()) {
-      if (context->channel_id_service() &&
-          params->channel_id_service->GetUniqueID() ==
-              context->channel_id_service()->GetUniqueID()) {
+      if (cookie_store->GetChannelIDServiceID() == -1) {
+        ephemerality = EPHEMERAL_UNKNOWN;
+      } else if (cookie_store->GetChannelIDServiceID() ==
+                 params->channel_id_service->GetUniqueID()) {
         ephemerality = EPHEMERAL_MATCH;
       } else {
         ephemerality = EPHEMERAL_MISMATCH;

[mmenke, 2016/03/28 17:54:00]

If this happens, it is a bug.  Should we really be logging bugs in histograms
instead of, say, DCHECKing when this happens?  If we can't DCHECK on
URLRequestContext initialization (Though I'd really prefer if we figure out a
way to do that), maybe DCHECK in URLRequestContext::CreateRequest?

I'm fine with doing both, but I really don't think we want to keep a histogram
about whether a URLRequestContext was created correctly, long term it's just
weird, and puts the burden of ensuring proper URLRequestContext creation on the
wrong team.

[nharper, 2016/03/29 00:30:20]

I don't see any reason why we can't DCHECK.
I'm assuming that DCHECKs don't fire on canary/dev builds, so I've added them to
the 4 cases that should never happen. I think doing both would be a good
short-term step, but I think long-term this histogram should go away. (Ideally
this whole method would go, but then there's no place to leave the DCHECKs.)

       }
     } else if (context->has_known_mismatched_cookie_store()) {
       ephemerality = KNOWN_MISMATCH;
     } else {
       ephemerality = CID_EPHEMERAL_COOKIE_PERSISTENT;
     }
   } else if (cookie_store->IsEphemeral()) {
     ephemerality = CID_PERSISTENT_COOKIE_EPHEMERAL;
-  } else if (context->channel_id_service() &&
-             params->channel_id_service->GetUniqueID() ==
-                 context->channel_id_service()->GetUniqueID()) {
+  } else if (cookie_store->GetChannelIDServiceID() == -1) {
+    ephemerality = PERSISTENT_UNKNOWN;
+  } else if (cookie_store->GetChannelIDServiceID() ==
+             params->channel_id_service->GetUniqueID()) {
     ephemerality = PERSISTENT_MATCH;
   } else {
     ephemerality = PERSISTENT_MISMATCH;
   }
   UMA_HISTOGRAM_ENUMERATION("Net.TokenBinding.StoreEphemerality", ephemerality,
                             EPHEMERALITY_MAX);
 }
 
 }  // namespace
 
@@ skipping 1478 matching lines @@
   return override_response_headers_.get() ?
              override_response_headers_.get() :
              transaction_->GetResponseInfo()->headers.get();
 }
 
 void URLRequestHttpJob::NotifyURLRequestDestroyed() {
   awaiting_callback_ = false;
 }
 
 }  // namespace net