Experiments vs. RuntimeEnabledFeatures

[Reilly Grant]

I'm putting together a patch to enable a WebUSB API Experiemental launch and I'm having trouble because if our WebUSB flag is enabled (which it is by default so we can disable it in Finch if something goes wrong) then RuntimeEnabledFeatures::webUSBEnabled() is true which overrides whether or not the site has a valid key. Is this intentional? What is the recommended way to have a flag to control the backend for a feature that is separate from the flag that controls whether it is exposed to sites?

[rei...@chromium.org]

On Thursday, March 24, 2016 at 11:51:54 AM UTC-7, Reilly Grant wrote:

I'm putting together a patch to enable a WebUSB API Experiemental launch and I'm having trouble because if our WebUSB flag is enabled (which it is by default so we can disable it in Finch if something goes wrong) then RuntimeEnabledFeatures::webUSBEnabled() is true which overrides whether or not the site has a valid key. Is this intentional? What is the recommended way to have a flag to control the backend for a feature that is separate from the flag that controls whether it is exposed to sites?

The problematic logic is in the generated code here where we return true immediately if the corresponding RuntimeEnabledFeature is enabled:

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blink/core/origin_trials/OriginTrials.cpp&sq=package:chromium&l=47&rcl=1458833226

 

[Marijn Kruisselbrink]

I think that is intentional. The thought being that we want to allow enabling these features both using the experimental web platform features flag (for local testing/development of new features without having to get a token) and through the experimental framework.

[Reilly Grant]

Then it sounds like we need to have the kill switch be a separate feature flag so we can gate backend setup on that but API availability on the normal feature flag. The problem being that we always need the backend bits to be enabled because we don't know until the page loads if the origin trial is enabled but can't use the normal feature flag for that because it will be "disabled" when being controlled by an origin trial.

[Jason Chase]

Reilly,

I believe you could make the feature and kill switch work with a single feature flag. The caveat is that Ian and I had discussed how this could work, but haven't tested end-to-end with an actual feature.

Obviously, you're correct that the feature needs to be enabled by default so that it can be turned off via Finch, and to have the backend bits always be enabled. And as Marijn said, it is intentional that the EF first checks if the RuntimeEnabledFeature is turned on.

The proposed approach is to change how the feature flag is wired up with the RuntimeFeature in SetRuntimeFeaturesDefaultsAndUpdateFromArgs(). In the current implementation, the kWebUsb feature flag is wired up to only turn off the runtime flag. This supports the kill switch, but doesn't allow a user to manually turn on WebUsb (via about://flags or command line). I'm assuming that you still want to allow the feature to be turned on manually (e.g. for testing, without providing an EF token).

The idea is to use the FeatureList::IsFeatureOverriddenFromCommandLine() functionality to check if the feature flag was explicitly turned on, vs just enabled by default. In SetRuntimeFeaturesDefaultsAndUpdateFromArgs(), if the feature flag was explicitly turned on (overridden), then enable the RuntimeFeature, otherwise set it to disabled.  This should allow the EF to control when the feature in enabled on the Blink side, when not turned off by the Finch kill switch, or manually turned on by the user.

Hopefully, my explanation makes sense.

Thanks,

Jason

[Reilly Grant]

Pinging this thread as promised at BlinkOn to get an update on the best way to set up a kill switch for an Origin Trial.

[Owen Campbell-Moore]

Friendly ping :)

--
You received this message because you are subscribed to the Google Groups "experimentation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to experimentation...@chromium.org.
To post to this group, send email to experimen...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/experimentation-dev/CAEmk%3DMaK55UBthNw1zDpMDVW_%3DDs4p_hR5dVDGX1FoUEn_WJWQ%40mail.gmail.com.

[Ian Clelland]

Sorry for the delay -- Short term, I think that Jason's proposed solution will work for this; the idea is to use FeatureList::IsFeatureOverriddenFromCommandLine to effectively make a tri-state that can differentiate between the feature being on-by default and the feature being on by user-declaration.

So, in runtime_features.cc, instead of this:

  if (!base::FeatureList::IsEnabled(features::kWebUsb))

    WebRuntimeFeatures::enableWebUsb(false);

We propose something like this:

  if (!base::FeatureList::

   IsFeatureOverriddenFromCommandLine(

     features::kWebUsb,

     FeatureList::OVERRIDE_ENABLE_FEATURE))

    WebRuntimeFeatures::enableWebUsb(false); // Allow origin trials to turn on as needed

I'd also move that to the top of the function, so that the ExperimentalWebPlatformFeatures flag can also turn WebUSB on, as it should. (I think)

Then when you need to determine in browser code whether the feature should be active (including whether the kill switch has been used), just test base::FeatureList::IsEnabled(features::kWebUsb).

There's still a possibility that--even when the kill switch is thrown--the bindings still get generated if a valid trial token is present. (Depending on the implementation, that may not be a serious problem; if you're always assuming that the renderer is untrusted, then you've probably already dealt with the fact that the renderer might try to call into the browser side even if the feature is disabled there)

We can still mitigate that -- we might have to tie the new disabled-feature-list in chrome/common/origin_trials/origin_trial_policy to the kill switch / FeautreList as well, to ensure that doesn't happen. That's on me, though.

Let me know if that makes sense, or if I've still missed something important.

Thanks,

Ian

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/experimentation-dev/CACjoXu5pc5Pc7RaMnWEz2WC75-rZOAboQH%3DsOT8WH7H5S18JdA%40mail.gmail.com.

[Marijn Kruisselbrink]

That largely makes sense (to me at least), but there are still a few cases it doesn't seem to cover. The major one being what you already mentioned, that even after the kill switch for a feature is triggered the bindings for that feature will still be generated. Now code using the experiment has to deal with three situations: 1) the API being present and working, 2) the API being present but not working (it's unclear what "not working" even means here) and 3) the API not being present. Especially since it is unclear what the state in possibility 2 would even look like, this seems very undesirable. 

In particular I'm not sure how this would allow a kill switch to also kill blink-side processing of a feature. Should all the code in blink that would check RuntimeEnabledFeatures::fooEnabled be changed to check both OriginTrials::fooEnabled and in addition also check base::FeatureList::IsEnabled() (which isn't actually possible, since the features are defined in content so blink can't check for them). It seems we really need a way for content to tell blink that the kill switch for a particular feature has been triggered to have any hope of being able to terminate experiments?