If you look at enum ContentSetting, there are two special values:
- SESSION_ONLY - used by cookies, for which Android does not support exceptions (technically it does, but the menu is not editable and there is no permission prompt for cookies, so no exception can be added)
- DETECT_IMPORTANT_CONTENT - used by plugins, which don't exist on Android

So we only have the usual values: ALLOW, ASK, BLOCK. ASK is only used for default settings, so we only have ALLOW / BLOCK exceptions.

Even if I missed something, treating != ALLOW as BLOCK sounds like a safe thing to do, privacy-wise :)

The bottom line is that Android's content settings code has a very simplified point of view on content settings, and will need a revamp (for example - you should be able to block cookies).

So looks like where these content settings are retrieved in native we strip out any default content settings, but there are no extra checks to ensure the values are only ALLOW or BLOCK:

https://cs.chromium.org/chromium/src/chrome/browser/android/preferences/website_preference_bridge.cc?type=cs&q=GetOrigins

So it seems we are relying on only ALLOW or BLOCK values being stored in the map in the first place, for settings that end up being displayed with an Allow/Block list preference in Android. Seems it would be better if we had something to actually guarantee this - maybe treat content settings that can only be allow/block/default as their own simpler type? It would be ideal if we could guarantee at compile-time that the assumption holds! However, I appreciate this would involve a non-trivial refactor.

In terms of runtime checks for the assumption holding, we could simply DCHECK that the value is ALLOW/BLOCK, or throw an explicit exception in that case (Slightly more helpful than allowing the code to nullpointer when attempting to get a string resource for a value other than allow/block).

A softer approach would be to remove the preference (or continue adding it as BLOCK) and log UMA if the value is not ALLOW/BLOCK. This would avoid crashing the app if ever the assumption is broken, while still alerting us to it happening. I think my preference would be to initially go with this approach and if this proves there's a problem fix this & refactor so it can't happen again.. Interested in others' views though!

Ah, just saw #3, thanks for your thoughts!

Good to hear you think the current behaviour should be safe, since the values *should* only ever be ALLOW/BLOCK, by that logic. 

However in that case seems like we could at least put in a runtime check, maybe keeping the current behaviour unchanged but at least adding UMA to see if any content setting types in list preferences are ever anything but ALLOW/BLOCK at that point. Would this be ok to log from a privacy perspective?

DCHECK sounds reasonable, since apart from manually editing the Preferences file, there really shouldn't be anything else than ALLOW/BLOCK. I would not use a release check for now (if the code was wrong, it would lead to a more privacy preserving behavior, not less, no need to crash Chrome for that).

I'm fine with adding UMA, but I'm not sure it's useful for decision making. Even if UMA shows that there are no values other than ALLOW/BLOCK, it's still possible that they exist outside of UMA population.

I think a comment is needed if it has confused an engineer working with the code :)

DCHECKing to make sure the assumption is valid also makes sense.