Collect UMA data for Origin Trials

third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp

Index: third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
diff --git a/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp b/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
index 1bae6e6b34edaaddee0088a05dd94140ad3f86bc..10dfde89ae25bfe9d2d36dee949ed6a9386c50b1 100644
--- a/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
+++ b/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
@@ -5,6 +5,7 @@
 #include "core/origin_trials/OriginTrialContext.h"
 
 #include "core/dom/ExecutionContext.h"
+#include "platform/Histogram.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "public/platform/Platform.h"
@@ -15,6 +16,44 @@ namespace blink {
 
 namespace {
 
+// The enum entries below are written to histograms and thus cannot be deleted
+// or reordered.
+// New entries must be added immediately before the end.
+enum EnableResult {

[Marijn Kruisselbrink, 2016/04/21 18:18:43]

I would make these enum class EnableResult { Success, NotSupported, ...
(also below)

It's also a bit unfortunate that we need a new enum separate from
TokenValidationResult for this, just because there are a few error conditions
that aren't included in TokenValidationResult. It might be cleaner to just add
all the failure modes to the one main enum.

[chasej, 2016/04/22 18:50:06]

Yes, it's not ideal to have the separate enum. I considered renaming
TokenValidationResult to TokenStatus, like on the content side. However, it seem
clearer to keep the naming, as the enum was used exclusively by the
WebTrialTokenValidator. Also, it seemed strange to have values in the enum (i.e.
NoTokens, Insecure) that didn't apply at all to the WebTrialTokenValidator.

I see the options as:
1) Rename TokenValidationResult to WebTrialTokenStatus with additional values
(in a separate file like WebCachePolicy.h)
2) Leave as is, with separate EnableResult enum

I'll hold off on the enum class changes, until the above is resolved.

[Marijn Kruisselbrink, 2016/04/22 21:51:06]

My preference would be to just have one central enum with all possible values.
But then I've been known to try to future-proof code too much (in this case one
potential future thing I could imagine would be if/when we check tokens in the
browser process for some features we'll probably want to collect the same (or
similar) UMA stats for those checks. At that point we'll need some enum
somewhere that also includes the NoTokens and Insecure values). But since it is
easy enough to merge any of these enums later, I don't feel particularly
strongly about that either; other than the general less code to write is less
code to maintain. And having three separate enums for essentially the same thing
seems quite a bit of unnecessary (but at least trivial) overhead.

[chasej, 2016/04/25 20:32:52]

As in earlier comment, I've changed to use a central enum, with all possible
values.

+    EnableResultSuccess = 0,

[Marijn Kruisselbrink, 2016/04/21 18:18:42]

Either give all enum members explicit values, or rely on the compiler to do the
right thing. It doesn't really make sense to explicitly set the first member to
zero but still rely on the compiler to do the right thing for the rest.

[chasej, 2016/04/22 18:50:06]

Done.

+    EnableResultNotSupported,
+    EnableResultInsecure,
+    EnableResultNoTokens,
+    EnableResultExpired,
+    EnableResultWrongFeature,
+    EnableResultWrongOrigin,
+    EnableResultInvalidSignature,
+    EnableResultMalformed,
+    EnableResultLast = EnableResultMalformed
+};
+
+// The enum entries below are written to histograms and thus cannot be deleted
+// or reordered.
+// New entries must be added immediately before the end.
+enum MessageGeneratedResult {
+    MessageGeneratedResultNotRequested = 0,
+    MessageGeneratedResultYes,
+    MessageGeneratedResultNo,
+    MessageGeneratedResultLast = MessageGeneratedResultNo
+};
+
+static EnumerationHistogram& enabledResultHistogram()
+{
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("OriginTrials.FeatureEnabled", EnableResultLast));

[Marijn Kruisselbrink, 2016/04/21 18:18:43]

Since we actually want to support trials in workers as well, these should
probably be DEFINE_THREAD_SAFE_STATIC_LOCAL? DEFINE_STATIC_LOCAL is not thread
safe after all.

[chasej, 2016/04/22 18:50:06]

Done.

+    return histogram;
+}
+
+static EnumerationHistogram& messageGeneratedResultHistogram()
+{
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("OriginTrials.FeatureEnabled.MessageGenerated", MessageGeneratedResultLast));
+    return histogram;
+}
+
 String getDisabledMessage(const String& featureName)
 {
     return "The '" + featureName + "' feature is currently enabled in limited trials. Please see https://bit.ly/OriginTrials for information on enabling a trial for your site.";
@@ -25,6 +64,83 @@ String getInvalidTokenMessage(const String& featureName)
     return "The provided token(s) are not valid for the '" + featureName + "' feature.";
 }
 
+String getNotSupportedMessage()
+{
+    return "This browser does not support origin trials.";
+}
+
+int getTokenValidationResultPriority(
+    WebTrialTokenValidator::TokenValidationResult validationResult)
+{
+    switch (validationResult) {
+    case WebTrialTokenValidator::TokenValidationResultSuccess:
+        // This function should only be used for validation failures
+        ASSERT_NOT_REACHED();
+        return 99;
+    case WebTrialTokenValidator::TokenValidationResultExpired:
+        return 1;
+    case WebTrialTokenValidator::TokenValidationResultWrongFeature:
+        return 2;
+    case WebTrialTokenValidator::TokenValidationResultWrongOrigin:
+        return 3;
+    case WebTrialTokenValidator::TokenValidationResultInvalidSignature:
+        return 4;
+    case WebTrialTokenValidator::TokenValidationResultMalformed:
+        return 5;
+    case WebTrialTokenValidator::TokenValidationResultNotSupported:
+        return 6;
+    }
+
+    ASSERT_NOT_REACHED();

[Marijn Kruisselbrink, 2016/04/21 18:18:42]

ASSERT_NOT_REACHED() is deprecated. Use NOTREACHED() instead.

[chasej, 2016/04/22 18:50:06]

Done.

+}
+
+EnableResult mapTokenValidationResultToEnableResult(
+    WebTrialTokenValidator::TokenValidationResult validationResult)
+{
+    switch (validationResult) {
+    case WebTrialTokenValidator::TokenValidationResultSuccess:
+        return EnableResultSuccess;
+    case WebTrialTokenValidator::TokenValidationResultExpired:
+        return EnableResultExpired;
+    case WebTrialTokenValidator::TokenValidationResultWrongFeature:
+        return EnableResultWrongFeature;
+    case WebTrialTokenValidator::TokenValidationResultWrongOrigin:
+        return EnableResultWrongOrigin;
+    case WebTrialTokenValidator::TokenValidationResultInvalidSignature:
+        return EnableResultInvalidSignature;
+    case WebTrialTokenValidator::TokenValidationResultMalformed:
+        return EnableResultMalformed;
+    case WebTrialTokenValidator::TokenValidationResultNotSupported:
+        return EnableResultNotSupported;
+    }
+
+    ASSERT_NOT_REACHED();
+}
+
+WebTrialTokenValidator::TokenValidationResult UpdateResultFromValidationFailure(
+    WebTrialTokenValidator::TokenValidationResult newResult,
+    WebTrialTokenValidator::TokenValidationResult currentResult)
+{
+    // There could be zero, one or multiple trial tokens provided for a given
+    // context, and any number of them could be invalid. The tokens could be
+    // invalid for a variety of reasons. For multiple invalid tokens, the
+    // failures are collected into a single result for collecting metrics. At a
+    // high level, the idea is to report on the tokens that were closest to
+    // being valid for enabling a feature. This gives the priority as:
+    //   1. Expired, but otherwise valid
+    //   2. Wrong feature, but correct origin
+    //   3. Wrong origin, but correct format for token data
+    //   4. Invalid signature for token
+    //   5. Invalid data in token (can be before/after validating signature)
+    //   6. Embedder does not support origin trials
+    // See this document for details:
+    // https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTtmg/edit#bookmark=id.k1j0q938so3b
+    if (currentResult == WebTrialTokenValidator::TokenValidationResultSuccess || getTokenValidationResultPriority(newResult) < getTokenValidationResultPriority(currentResult)) {
+        return newResult;
+    }
+    return currentResult;
+}
+
 } // namespace
 
 OriginTrialContext::OriginTrialContext(ExecutionContext* host) : m_host(host)
@@ -56,11 +172,59 @@ void OriginTrialContext::addToken(const String& token)
 bool OriginTrialContext::isFeatureEnabled(const String& featureName, String* errorMessage, WebTrialTokenValidator* validator)
 {
     if (!RuntimeEnabledFeatures::experimentalFrameworkEnabled()) {
-        // TODO(iclelland): Set an error message here, the first time the
-        // context is accessed in this renderer.
+        // Do not set an error message. When the framework is disabled, it
+        // should behave the same as when only runtime flags are used.
         return false;
     }
 
+    EnableResult result = static_cast<EnableResult>(checkFeatureEnabled(featureName, errorMessage, validator));
+    bool isEnabled = (result == EnableResultSuccess);
+
+    // Record metrics for the enabled result, but only once per context.
+    if (!m_enabledResultCountedForFeature.contains(featureName)) {
+        enabledResultHistogram().count(result);
+        m_enabledResultCountedForFeature.add(featureName);
+    }
+
+    // If an error message has already been generated in this context, for this
+    // feature, do not generate another one. This avoids cluttering the console
+    // with error messages on every attempt to access the feature.
+    // Metrics are also recorded, to track whether too many messages are being
+    // generated over time.
+    MessageGeneratedResult generateMessage = MessageGeneratedResultYes;
+    if (!errorMessage) {
+        generateMessage = MessageGeneratedResultNotRequested;
+    } else if (m_errorMessageGeneratedForFeature.contains(featureName)) {
+        *errorMessage = "";
+        generateMessage = MessageGeneratedResultNo;
+    }
+    messageGeneratedResultHistogram().count(result);
+
+    if (generateMessage != MessageGeneratedResultYes) {
+        return isEnabled;
+    }
+
+    // Generate an error message, only if one was not already provided by the
+    // enabled check.
+    if (!errorMessage->length()) {
+        switch (result) {
+        case EnableResultNotSupported:
+            *errorMessage = getNotSupportedMessage();
+            break;
+        case EnableResultNoTokens:
+            *errorMessage = getDisabledMessage(featureName);
+            break;
+        default:
+            *errorMessage = getInvalidTokenMessage(featureName);

[Marijn Kruisselbrink, 2016/04/21 18:18:43]

Would it make sense to refine the error message based on the result of
checkFeatureEnabled? 

[chasej, 2016/04/22 18:50:06]

I supposed we could now - before it wasn't possible with just a boolean result.
Also, the new token format isn't human readable, so you can't visually inspect
tokens for some the problems like wrong feature/origin.

On the other hand, if we're able to make the desired bindings changes, there
could be no need for detailed error messages. Ditto, if/when there is devtools
support.

I'll consider that with other changes in the next patch.

[Marijn Kruisselbrink, 2016/04/22 21:51:06]

sounds good (not sure how bindings changes would eliminate the need for detailed
error messages though?)

[chasej, 2016/04/25 20:32:52]

Today, features are still available to JavaScript, even when disabled. While the
bindings return undefined, the enabled check still happens on each access (which
generates the messages). The goal is to have disabled features hidden completely
from the bindings. If that is achieved, any access attempts would not perform
the enabled check - so no message generated.

[Marijn Kruisselbrink, 2016/04/25 21:03:44]

Ah, so it's not like bindings changes would eliminate the need for detailed
error messages, but rather that it becomes harder/impossible to actually emit
error messages? I think it would be unfortunate to get rid of messages to point
out why features aren't working... I wonder what the plans are around
secure-context only features, where at spec level feature are also supposed to
be completely hidden from bindings rather then the current exceptions that are
being thrown; do you know if they (whoever is working on that, presumably some
of the same people from the bindings side) are also planning on getting rid of
error messages for accessing secure context only APIs from an insecure context?

+            break;
+        }
+    }
+    m_errorMessageGeneratedForFeature.add(featureName);
+    return isEnabled;
+}
+
+int OriginTrialContext::checkFeatureEnabled(const String& featureName, String* errorMessage, WebTrialTokenValidator* validator)

[Marijn Kruisselbrink, 2016/04/21 18:18:43]

Why is this returning "int" rather than the correct enum type?

[chasej, 2016/04/22 18:50:06]

The EnableResult enum is declared local to the source file. My intent was to
keep the enum out of the header file, since it is purely an implementation
detail. Having this function return int was the one issue with that approach.

This may be a moot point, depending on the resolution of the larger questions
about the various enum declarations. If not, what makes the most sense here? Add
a comment explaining the int return value, or move the EnableResult to the
header file?

[Marijn Kruisselbrink, 2016/04/22 21:51:06]

I think anything that results in having the method return the correct type
rather than an int would be better (so probably define the enum as private in
the header file). Everything that is "private" is purely an implementation
detail, yet we don't go out of our way (in chromium) to try to hide all of that
from header files, so not sure why this would be different.

But if you do want to keep implementation details out of the header file, you
could just get rid of the entirely separate checkFeatureEnabled method. It's
kind of confusing to have to methods with nearly identical names, taking
identical arguments that do subtly different things, even if one is only a
private method. I'm not entirely sure what we gain from having a separate method
(if you do want a separate methods, maybe you could have a file local non-member
method that does all of the current checkFeatureEnabled minus the secure context
check by passing the list of tokens to said method. That way you could even have
correct return type without defining the enum anywhere else).

[chasej, 2016/04/25 20:32:52]

The return type is a non-issue, now that I'm using a central enum.

With regards to the two similar named methods, I didn't think the differences
were that subtle (at least based on the implementation). I figured only one
method is public, so there shouldn't be much confusion about which to use. The
private method does the actual enabled check. The public method wraps the core
check in handling for UMA and message generation. Granted there is some overlap
in that either method could generate the error message. I had it all in one
method at one point, but it was too convoluted because I could no longer use
simple return statements to exit early.

I updated the comments on the method declarations. Suggestions welcome if there
is better naming to make the differences more obvious.

[Marijn Kruisselbrink, 2016/04/25 21:03:44]

I think personally I would have made the split roughly the other way around:
most of the code that is currently in isFeatureEnabled seems to be related to
possibly generating the correct error message, so move that to a separate method
and keep all the token checks in one method. But with the comments this works
too. So either way fine with me.

+{
     // Feature trials are only enabled for secure origins
     bool isSecure = errorMessage
         ? m_host->isSecureContext(*errorMessage)
@@ -71,45 +235,31 @@ bool OriginTrialContext::isFeatureEnabled(const String& featureName, String* err
         // not, and decide whether the OriginTrialContext should be using its
         // own error messages for this case.
         DCHECK(!errorMessage || !errorMessage->isEmpty());
-        return false;
+        return EnableResultInsecure;
     }
 
     if (!validator) {
         validator = Platform::current()->trialTokenValidator();
         if (!validator) {
-            // TODO(iclelland): Set an error message here, the first time the
-            // context is accessed in this renderer.
-            return false;
+            return EnableResultNotSupported;
         }
     }
 
-
+    WebTrialTokenValidator::TokenValidationResult failedValidationResult = WebTrialTokenValidator::TokenValidationResultSuccess;
     WebSecurityOrigin origin(m_host->getSecurityOrigin());
     for (const String& token : m_tokens) {
-        // Check with the validator service to verify the signature.
-        if (validator->validateToken(token, origin, featureName)) {
-            return true;
+        // Check with the validator service to verify the signature and that
+        // the token is valid for the combination of origin and feature.
+        WebTrialTokenValidator::TokenValidationResult tokenResult = validator->validateToken(token, origin, featureName);
+        if (tokenResult == WebTrialTokenValidator::TokenValidationResultSuccess) {
+            return EnableResultSuccess;
         }
+        failedValidationResult = UpdateResultFromValidationFailure(tokenResult, failedValidationResult);
     }
 
-    if (!errorMessage)
-        return false;
-
-    // If an error message has already been generated in this context, for this
-    // feature, do not generate another one. (This avoids cluttering the console
-    // with error messages on every attempt to access the feature.)
-    if (m_errorMessageGeneratedForFeature.contains(featureName)) {
-        *errorMessage = "";
-        return false;
-    }
-
-    if (m_tokens.size()) {
-        *errorMessage = getInvalidTokenMessage(featureName);
-    } else {
-        *errorMessage = getDisabledMessage(featureName);
-    }
-    m_errorMessageGeneratedForFeature.add(featureName);
-    return false;
+    return m_tokens.size()
+        ? mapTokenValidationResultToEnableResult(failedValidationResult)
+        : EnableResultNoTokens;
 }
 
 DEFINE_TRACE(OriginTrialContext)