X-Chrome-Variations logic refactoring

chrome/browser/chrome_metrics_helper.cc

Index: chrome/browser/chrome_metrics_helper.cc
diff --git a/chrome/browser/chrome_metrics_helper.cc b/chrome/browser/chrome_metrics_helper.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b6132f7e3cead4e703bc00b73ad70d19bb9f2a0c
--- /dev/null
+++ b/chrome/browser/chrome_metrics_helper.cc
@@ -0,0 +1,163 @@
+// 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/chrome_metrics_helper.h"
+
+#include "base/base64.h"
+#include "base/memory/singleton.h"
+#include "chrome/browser/google/google_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_io_data.h"
+#include "chrome/common/metrics/proto/chrome_experiments.pb.h"
+#include "chrome/common/metrics/variations/variations_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/http/http_request_headers.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request.h"
+
+using base::AutoLock;
+using content::BrowserThread;

[Peter Kasting, 2012/12/11 21:08:48]

Nit: Normally we avoid using directives unless they save a significant number of
lines; here it seems like you could eliminate both of these.

[Bart N., 2012/12/11 23:18:56]

Done for AutoLock, however the existing BrowserThread makes actually the code
more readable, especially after you asked me to add IsWellKnownThread.

+
+ChromeMetricsHelper* ChromeMetricsHelper::GetInstance() {
+  return Singleton<ChromeMetricsHelper>::get();
+}
+
+ChromeMetricsHelper::ChromeMetricsHelper()

[Peter Kasting, 2012/12/11 21:08:48]

Nit: Function definition order must match declaration order.

[Bart N., 2012/12/11 23:18:56]

Done.

+    : variation_ids_cache_initialized_(false) {
+}
+
+ChromeMetricsHelper::~ChromeMetricsHelper() {
+}
+
+void ChromeMetricsHelper::AppendHeadersToRequest(

[Peter Kasting, 2012/12/11 21:08:48]

Nit: Based on the implementations of these two functions, it seems like this
class' API could just expose AppendHeaders() directly, and leave it up to
callers to provide an appropriate HttpRequestHeaders* and other data.  This
would mean this class wouldn't have to know anything about URLFetchers or
URLRequests, which seems like a win to me.

[Bart N., 2012/12/11 23:18:56]

I think it's a good suggestion. I made this class as close to the original
implementation as possible (to avoid too much shuffling), but if Steve has no
objections, I'd also prefer to get rid of the other 2 methods and leave it up to
the caller.

[SteveT, 2012/12/13 16:24:08]

+1 to this class not knowing about Fetches vs Requests.

The implementation may have been a bit specialized to support the RDH, but I
think if we're going to refactor it out here, we'll want to simplify the API as
much as possible and reduce dependencies.

[Bart N., 2012/12/14 18:21:42]

Done.

+    net::URLRequest* request,
+    content::ResourceContext* resource_context) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

[Peter Kasting, 2012/12/11 21:08:48]

You may need to add a call to IsWellKnownThread() here so you don't break in
tests that don't name particular threads -- see similar checks at other places
in our codebase.

[Bart N., 2012/12/11 23:18:56]

Good to know. Anyway, this the old code.

Done.

+
+  // Don't attempt to append headers to requests that have already started.
+  // TODO(stevet): Remove this once the request ordering issues are resolved
+  // in crbug.com/128048.
+  if (request->is_pending())
+    return;
+
+  net::HttpRequestHeaders headers;
+  headers.CopyFrom(request->extra_request_headers());
+  ProfileIOData* io_data = ProfileIOData::FromResourceContext(resource_context);
+  AppendHeaders(request->url(), io_data->is_incognito(),
+                io_data->GetMetricsEnabledStateOnIOThread(), &headers);
+  request->SetExtraRequestHeaders(headers);
+}
+
+void ChromeMetricsHelper::AppendHeadersToFetcher(net::URLFetcher* fetcher,
+                                                 const Profile* profile) {
+  net::HttpRequestHeaders headers;
+  AppendHeaders(fetcher->GetOriginalURL(), profile->IsOffTheRecord(), false,
+                &headers);
+  fetcher->SetExtraRequestHeaders(headers.ToString());
+}
+
+void ChromeMetricsHelper::AppendHeaders(
+    const GURL& url,
+    bool incognito,
+    bool uma_enabled,
+    net::HttpRequestHeaders* headers) {
+
+  // Note the criteria for attaching Chrome experiment headers:
+  // 1. We only transmit to *.google.<TLD> domains. NOTE that this use of
+  //    google_util helpers to check this does not guarantee that the URL is
+  //    Google-owned, only that it is of the form *.google.<TLD>. In the future
+  //    we may choose to reinforce this check.
+  // 2. Only transmit for non-Incognito profiles.
+  // 3. For the X-Chrome-UMA-Enabled bit, only set it if UMA is in fact enabled
+  //    for this install of Chrome.
+  // 4. For the X-Chrome-Variations, only include non-empty variation IDs.
+  if (incognito ||
+      !google_util::IsGoogleDomainUrl(url.spec(),
+                                      google_util::ALLOW_SUBDOMAIN,
+                                      google_util::ALLOW_NON_STANDARD_PORTS))
+    return;
+
+  if (uma_enabled)
+    headers->SetHeaderIfMissing("X-Chrome-UMA-Enabled", "1");
+
+  // Lazily initialize the header, if not already done, before attempting to
+  // transmit it.
+  InitVariationIDsCacheIfNeeded();
+  AutoLock scoped_lock(lock_);
+  if (!variation_ids_header_.empty())
+    headers->SetHeaderIfMissing("X-Chrome-Variations", variation_ids_header_);
+}
+
+void ChromeMetricsHelper::OnFieldTrialGroupFinalized(

[Peter Kasting, 2012/12/11 21:08:48]

Is this supposed to be called exclusively on one particular thread?  If so add a
DCHECK to that effect.  If not, add comments as to what threads we could see
this on and why.  (This is actually good advice for all these functions when
your class is multithreaded.)

[Bart N., 2012/12/11 23:18:56]

I think it can be called only from the IO thread (Steve, can you confirm?).

One thing I've noticed is that the existing implementation  calls
base::FieldTrialList::AddObserver(this) from InitVariationIDsCacheIfNeeded which
in turn may be called from different threads.

[SteveT, 2012/12/13 16:24:08]

So this method can be called on this class after this class adds itself as an
Observer of the FieldTrialList. The FTL knows to call this method on the same
thread as the one it was registered on.

Back in ChromeResourceDispatchHostDelegate, this would only happen on the IO
thread. Today, it appears that it could happen on different threads, given your
above comment.

Does that make sense?

[Bart N., 2012/12/14 18:21:42]

I think so. However, I've realized that the existing and new implementation have
a flaw:

(1) AddObserver will not register the given observer if the current thread has
no MessageLoop
(2) AddObserver will be called at most once (because it's guarded by
variation_ids_cache_initialized_
(3) If the first call to AddObserver is called on a thread w/o Message loop, we
will never receive notifications!

In practices, both UI/IO threads have MessageLoops, but I'm wondering if we
should a DCHECK (before a call to AddObserver) to make sure that the current
thread has MessageLoop. Something like: 

DCHECK(MessageLoop::current());

Another option is to make AddObserver return whether it registered an observer
or not and then DCHECK on it.

[SteveT, 2012/12/14 18:35:41]

This seems like a general issue with the pattern, then. I'm surprised there
isn't some warning about this (perhaps it's in the docs and I missed it).

Anyway, we definitely do want to be aware of such cases, so let's DCHECK as you
suggested?  Note that I looked through code search a bit and there appears to be
cases where people DCHECK(MessageLoop::current()), but not necessary for the
same reasons as discussed here.

But also - what Chrome thread doesn't have a MessageLoop object associated with
it?

[Bart N., 2012/12/16 00:09:09]

No idea. In our case, both IO/UI have it.

Done.

+    const std::string& trial_name,
+    const std::string& group_name) {
+  chrome_variations::VariationID new_id =
+      chrome_variations::GetGoogleVariationID(
+          chrome_variations::GOOGLE_WEB_PROPERTIES, trial_name, group_name);
+  if (new_id == chrome_variations::kEmptyID)
+    return;
+  AutoLock scoped_lock(lock_);
+  variation_ids_set_.insert(new_id);
+  UpdateVariationIDsHeaderValue();
+}
+
+void ChromeMetricsHelper::InitVariationIDsCacheIfNeeded() {
+  AutoLock scoped_lock(lock_);
+  if (variation_ids_cache_initialized_)
+    return;
+
+  // Register for additional cache updates. This is done first to avoid a race
+  // that could cause registered FieldTrials to be missed.
+  base::FieldTrialList::AddObserver(this);
+
+  base::FieldTrial::ActiveGroups initial_groups;
+  base::FieldTrialList::GetActiveFieldTrialGroups(&initial_groups);
+  for (base::FieldTrial::ActiveGroups::const_iterator it =
+       initial_groups.begin(); it != initial_groups.end(); ++it) {
+    const chrome_variations::VariationID id =
+        chrome_variations::GetGoogleVariationID(
+            chrome_variations::GOOGLE_WEB_PROPERTIES, it->trial_name,
+            it->group_name);
+    if (id != chrome_variations::kEmptyID)
+      variation_ids_set_.insert(id);
+  }
+  UpdateVariationIDsHeaderValue();
+
+  variation_ids_cache_initialized_ = true;
+}
+
+void ChromeMetricsHelper::UpdateVariationIDsHeaderValue() {

[Peter Kasting, 2012/12/11 21:08:48]

Nit: While it's technically not necessary since both the callers do this, I
suggest taking the lock here.  This is harmless and will keep you safe if you
change the callers.

[Bart N., 2012/12/11 23:18:56]

Doesn't it incur extra CPU cost? This method is private, so I'm not sure if I
buy your argument.

[Peter Kasting, 2012/12/12 00:01:06]

I don't know about the cost, but I can't imagine it's material; this is getting
called occasionally as opposed to tens of thousands of time in some loop.

You're right that privacy helps -- we're not going to add outside-class callers,
unless we expose this publicly -- but even so, other within-class functions
could change in the future.  It seems like a small price to pay for clarity and
safety.

Another possibility would be to instead DCHECK that the lock is already held on
entry.  That's just as safe and cheaper, at the cost of causing debug-only
runtime failures that might not be noticed if someone ever updates the code in a
way that breaks things.

[Bart N., 2012/12/12 02:06:17]

OK. Two things:
(1) The cost of acquiring a lock is low, but it is definitely non-zero and
should be avoided if possible, especially in cases like this
(2) I'm not sure if you are aware of that, but by default locks are not
recursive; I didn't mention it in first place, since Chrome uses Lock class
(which I don't know about). I checked the implementation and in fact, the locks
are non-recursive. To put it simple, you are asking me to introduce a deadlock.
Is that what you want?
This suggestion actually makes sense, although you probably meant to a call to:
lock_.AssertAcquired() rather than a DCHECK, right?

+  // The header value is a serialized protobuffer of Variation IDs which is
+  // base64 encoded before transmitting as a string.
+  if (variation_ids_set_.empty())
+    return;
+
+  // This is the bottleneck for the creation of the header, so validate the size
+  // here. Force a hard maximum on the ID count in case the Variations server
+  // returns too many IDs and DOSs receiving servers with large requests.
+  DCHECK_LE(variation_ids_set_.size(), 10U);
+  if (variation_ids_set_.size() > 20) {

[Peter Kasting, 2012/12/11 21:08:48]

You cannot handle DCHECK failure.  A DCHECK means "this cannot ever happen even
in the face of a hostile server".  I suspect what you want here is to simply
leave the conditional and remove the DCHECK.

It's also not clear where you're picking these values from or why the right
response is to clear entirely rather than clamping.  Some commentary would help.

[Bart N., 2012/12/11 23:18:56]

I'll leave it up to Steve to answer this question; most of the code here is
simply moved from another class.

[SteveT, 2012/12/13 16:24:08]

Yeah so this was added as a double safety:

 * The DCHECK is meant to make developers aware that we're seeing more Variation
IDs being transmitted than we'd like.
 * The hard > 20 check is meant to be a defense against bad server payloads that
cause the client to transmit too many IDs to Google.

I believe 10 and 20 were chosen somewhat arbitrarily. We basically believed that
we should probably not have more than 10 such experiments running (though we
could re-evaluate that if demand increases).

Happy to change this if you think the strategy we chose here is ineffective or
wrong.

+    variation_ids_header_.clear();
+    return;
+  }
+
+  metrics::ChromeVariations proto;
+  for (std::set<chrome_variations::VariationID>::const_iterator it =
+      variation_ids_set_.begin(); it != variation_ids_set_.end(); ++it)
+    proto.add_variation_id(*it);
+
+  std::string serialized;
+  proto.SerializeToString(&serialized);
+
+  std::string hashed;
+  if (base::Base64Encode(serialized, &hashed)) {
+    // If successful, swap the header value with the new one.
+    // Note that the list of IDs and the header could be temporarily out of sync
+    // if IDs are added as the header is recreated. The receiving servers are OK
+    // with such descrepancies.
+    variation_ids_header_ = hashed;
+  } else {
+    DVLOG(1) << "Failed to base64 encode Variation IDs value: " << serialized;

[Peter Kasting, 2012/12/11 21:08:48]

Is logging really what you want here?  Note that it's often hard to collect good
logs from clients.

If Base64Encode() should never ever fail, you could just DCHECK and proceed
unconditionally.

[Bart N., 2012/12/11 23:18:56]

Steve?

[SteveT, 2012/12/13 16:24:08]

Hmm, yeah, I believe we were just trying to follow precedence of what other
users of Base64Encode did upon failure... But looking through the codebase
today, it appears that most folks LOG(FATAL) or DCHECK/NOTREACHED... we should
do the same here.

So could you change the DVLOG to NOTREACHED(), here?

+  }
+}