Yeah, I think we should add an enumeration here. As-is, this failure is not very actionable.

My guess is that this is coming mostly from easyunlock, but I *think* this should stabilize more over time as the "grandfathering" phones start to re-enroll. I agree with the suggesting to break this metric down more.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b057832fb30243485d71f67b36dfe08ff83666a1

commit b057832fb30243485d71f67b36dfe08ff83666a1
Author: Josh Nohle <nohle@google.com>
Date: Thu Oct 25 22:59:47 2018

[CrOS MultiDevice] Add metrics for features that are enabled but not supported

Currently, when a device is retrieved by CryptAuth, a boolean histogram
"CryptAuth.DeviceSyncSoftwareFeaturesResult" logs "Success" for each
feature that is supported and enabled, and logs "Failure" for each
feature that is enabled but not supported.

Here, we provide a more granular look into the latter case by adding
another histogram, "CryptAuth.DeviceSyncSoftwareFeaturesResultFailures",
that buckets each failure by feature type, i.e., by the enum
cryptauth::SoftwareFeatures.

This was tested manually; however, we could only get EASY_UNLOCK_HOST to
be in a state where it was enabled but not supported.

Bug:  896082 
Change-Id: Ib4fae768f203d637caa87d0a9e0baa5bf596f515
Tested: Manual; added unit test
Reviewed-on: https://chromium-review.googlesource.com/c/1297285
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Josh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602915}
[modify] https://crrev.com/b057832fb30243485d71f67b36dfe08ff83666a1/components/cryptauth/cryptauth_device_manager_impl.cc
[modify] https://crrev.com/b057832fb30243485d71f67b36dfe08ff83666a1/components/cryptauth/cryptauth_device_manager_impl_unittest.cc
[modify] https://crrev.com/b057832fb30243485d71f67b36dfe08ff83666a1/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/b057832fb30243485d71f67b36dfe08ff83666a1/tools/metrics/histograms/histograms.xml

I'll loop back on this bug in 2 or 3 weeks once M72 has soaked in dev-channel.

Jeremy was correct, Smart Lock is the culprit [1].

There's not enough data here yet to tell if this is stabilizing over time.

We could prevent this from becoming a problem on clients by glossing over a feature being unsupported but enabled here [2]. Kyle, is there any danger in doing this?

1) https://uma.googleplex.com/p/chrome/timeline_v2/?sid=743e6518bba3208ba4f628a46ffc0bf7
2) https://cs.chromium.org/chromium/src/components/cryptauth/cryptauth_device_manager_impl.cc?sq=package:chromium&g=0&l=153

Can you clarify what "glossing over" the feature would mean? What value would we set for the feature in this case?

My main worry is that this would interact badly with Josh's GrandfatheredEasyUnlockHostDisabler [1] class. That class continually tries to disable EasyUnlock under certain circumstances. If the server is in a different state from what we tell clients that it is in, these server calls could fail, resulting in an infinite loop of retries.

[1] https://cs.chromium.org/chromium/src/chromeos/services/multidevice_setup/grandfathered_easy_unlock_host_disabler.h

I imagine "glossing over" means removing "continue" in [1].

The GrandfatheredEasyUnlockHostDisabler only cares about trying to disable EASY_UNLOCK_HOST after BETTER_TOGETHER_HOST is toggled from enabled to disabled. I don't think what Ryan is proposing would prevent a call to SetSoftwareFeatureState(..., EASY_UNLOCK_HOST, false /*enabled*/, ...) to fail. Also, GrandfatheredEasyUnlockHostDisabler doesn't care about the supported state.

I'm not advocating for Ryan's change necessarily, but if I understand correctly, his proposal would make the enabled states _more_ consistent between the CryptAuth server and the Chrome OS client.

[1] https://cs.chromium.org/chromium/src/components/cryptauth/cryptauth_device_manager_impl.cc?sq=package:chromium&g=0&l=153

I'm more concerned about Chrome OS using a device as an EasyUnlock host if the feature is not supported. What's the systemic issue here, i.e., how does a device ever get into this state in the CryptAuth server to begin with?

In Comment 8, replace "prevent" with "cause" :-)

Yep, +1 to everything Josh said. My real concern is the issue raised in #9.

Discussed offline. The server special-cases to allow for EASY_UNLOCK_HOST to be kEnabled but not kSupported for legacy reasons, and so should the Chrome client. I'll spin up a fix.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa36646314ab31090a2fc15d084881f64909e624

commit aa36646314ab31090a2fc15d084881f64909e624
Author: Ryan Hansberry <hansberry@chromium.org>
Date: Wed Nov 21 21:37:15 2018

[CrOS MultiDevice] Make exemption for no EASY_UNLOCK_HOST support.

To support legacy Smart Lock hosts which did not mark themselves as
supporting EASY_UNLOCK_HOST (because they couldn't have), simply
ignore lack of a kSupported bit if kEnabled is present, and set the
host device as an enabled Smart Lock host.

Bug:  896082 
Change-Id: Id8fb1ade25113fcb108a7658738e7bf9e0c996a0
Reviewed-on: https://chromium-review.googlesource.com/c/1345123
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610222}
[modify] https://crrev.com/aa36646314ab31090a2fc15d084881f64909e624/components/cryptauth/cryptauth_device_manager_impl.cc
[modify] https://crrev.com/aa36646314ab31090a2fc15d084881f64909e624/components/cryptauth/cryptauth_device_manager_impl_unittest.cc