Some features have a status of "test". [1] Because of the ordering of various calls to enable/disable features, origin trial tests that use --disable-origin-trial-controlled-blink-features to check that a feature is NOT enabled without a token [2] fail if the status of the feature in runtime_enabled_features.json5 is "test".
The following sequences happen when using content_shell --run-layout-test.
RenderThreadImpl::InitializeWebKit() calls SetRuntimeFeaturesDefaultsAndUpdateFromArgs(), which does the following:
* Calls EnableExperimentalFeatures(true) because kDisableOriginTrialControlledBlinkFeatures is set.
* Calls EnableOriginTrialControlledFeatures(false) because kDisableOriginTrialControlledBlinkFeatures is set.
- This eventually calls the generated function RuntimeEnabledFeatures::SetOriginTrialControlledFeaturesEnabled(false).
Next, RenderThreadImpl::InitializeWebKit() calls GetContentClient()->renderer()->SetRuntimeFeaturesDefaultsBeforeBlinkInitialization(). When running layout tests, this is layout_test_content_renderer_client.cc::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization(), which calls EnableTestOnlyFeatures(true). This enables all features with a status of "test", including those that were disabled above.
Thus, any origin trial-controlled feature that has a status of "test" gets re-enabled, causing the test to fail.
For this specific problem, we could either require that features in an origin trial have a status of "experimental" (including code assertions) or change when SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() is called.
More generally, it's unfortunate that features are enabled in different places because it makes it hard to understand and avoid such issues. As an example, Chrome's version of SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() enables a feature (WebShare on Android) in production! If we do need to call out to the client, maybe we should at least move the SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() call inside SetRuntimeFeaturesDefaultsAndUpdateFromArgs() and make it the single function where features are enabled/disabled.
[1] Whether this is correct is a separate discussion. One could argue that all features in an Origin Trial should have a status of "experimental". One reason "test" is used is to avoid being enabled with --enable-experimental-web-platform-features because there is a separate flag for the feature. Maybe they shouldn't be doing that, but there may be cases where it is necessary.
[2] For example, tests in virtual/origin-trials-runtimeflags-disabled/http/tests/origin_trials/webexposed/.
Comment 1 by ddorwin@chromium.org
, Apr 13 2018