New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 831869 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Show other hotlists

Hotlists containing this issue:
Feature-Control-Code-Health


Sign in to add a comment

--disable-origin-trial-controlled-blink-features does not affect features with status "test"

Project Member Reported by ddorwin@chromium.org, Apr 11 2018

Issue description

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/.
 
Components: Internals>FeatureControl
Regarding [1], https://chromium.googlesource.com/chromium/src/+/master/docs/origin_trials_integration.md#runtime-enabled-features currently says, "You can have both status: experimental and origin_trial_feature_name if you want your feature to be enabled either by using the --enable-experimental-web-platform-features flag or the origin trial." However, it's not clear what alternatives are allowed. Because of this issue, it appears that "experimental" is currently the only possible status.

Comment 2 by cha...@chromium.org, Apr 13 2018

Status: Available (was: Untriaged)

Comment 3 by cha...@chromium.org, Jan 16 (6 days ago)

Owner: arobins@google.com
Status: Assigned (was: Available)
Allen, a potential starter task.

Sign in to add a comment