Correct multiple levels of depends_on and implied_by of runtime enabled features

Previously RuntimeEnabledFeatures::*Enabled() was incorrect when there
were multiple levels of depends_on or implied_by.

Now call *Enabled() of the depends_on/implied_by features instead of
getting their bool variables (which don't reflect the features' status
if there are depends_on/implied_by).

Add cycle detection of implied_by because that situation would otherwise
cause infinite recursion with this CL.

Add detection of non-origin-trial feature implied by origin trial
feature because that situation is invalid (see crbug.com/1061959)
and would otherwise cause compilation error with this CL.

Bug: 1061443, 1061959
Change-Id: I7b6cda1076f0c2a81edb5567a745d237c88259ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2104019Reviewed-by: Jason Chase <chasej@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751544}

third_party/blink/renderer/build/scripts/make_runtime_features.py

@@ -62,18 +62,6 @@ class BaseRuntimeFeatureWriter(json5_generator.Writer):
                 feature['name']) in origin_trial_set
             feature['data_member_name'] = self._data_member_name(
                 feature['name'])
-            # Most features just check their is_foo_enabled_ bool
-            # but some depend on or are implied by other bools.
-            enabled_condition = feature['data_member_name']
-            assert not feature['implied_by'] or not feature[
-                'depends_on'], 'Only one of implied_by and depends_on is allowed'
-            for implied_by_name in feature['implied_by']:
-                enabled_condition += ' || ' + self._data_member_name(
-                    implied_by_name)
-            for dependant_name in feature['depends_on']:
-                enabled_condition += ' && ' + self._data_member_name(
-                    dependant_name)
-            feature['enabled_condition'] = enabled_condition
             # If 'status' is a dict, add the values for all the not-mentioned platforms too.
             if isinstance(feature['status'], dict):
                 feature['status'] = self._status_with_all_platforms(

third_party/blink/renderer/build/scripts/make_runtime_features_utilities.py

@@ -5,40 +5,55 @@
 from collections import defaultdict
 
 
-def _runtime_features_graph_sanity_check(features):
+def _error_message(message, feature, other_feature=None):
+    message = 'runtime_enabled_features.json5: {}: {}'.format(feature, message)
+    if other_feature:
+        message += ': {}'.format(other_feature)
+    return message
+
+
+def _validate_runtime_features_graph(features):
     """
     Raises AssertionError when sanity check failed.
-    @param features: a List[Dict]. Each Dict must have key 'depends_on' and 'name'
+    @param features: a List[Dict]. See origin_trials().
     @returns None
     """
     feature_pool = {str(f['name']) for f in features}
+    origin_trial_pool = {
+        str(f['name'])
+        for f in features if f['origin_trial_feature_name']
+    }
     for f in features:
+        assert not f['implied_by'] or not f['depends_on'], _error_message(
+            'Only one of implied_by and depends_on is allowed', f['name'])
         for d in f['depends_on']:
-            assert d in feature_pool, "{} not found in runtime_enabled_features.json5".format(
-                d)
+            assert d in feature_pool, _error_message(
+                'Depends on non-existent-feature', f['name'], d)
+        for i in f['implied_by']:
+            assert i in feature_pool, _error_message(
+                'Implied by non-existent-feature', f['name'], i)
+            assert f['origin_trial_feature_name'] or i not in origin_trial_pool, \
+                _error_message(
+                    'A feature must be in origin trial if implied by an origin trial feature',
+                    f['name'], i)
 
-    def cyclic(features):
-        """
-        Returns True if the runtime features graph contains a cycle
-        @returns bool
-        """
-        graph = {
-            str(feature['name']): feature['depends_on']
-            for feature in features
-        }
-        path = set()
+    graph = {
+        str(feature['name']): feature['depends_on'] + feature['implied_by']
+        for feature in features
+    }
+    path = set()
 
-        def visit(vertex):
-            path.add(vertex)
-            for neighbor in graph[vertex]:
-                if neighbor in path or visit(neighbor):
-                    return True
-            path.remove(vertex)
-            return False
+    def has_cycle(vertex):
+        path.add(vertex)
+        for neighbor in graph[vertex]:
+            if neighbor in path or has_cycle(neighbor):
+                return True
+        path.remove(vertex)
+        return False
 
-        return any(visit(str(f['name'])) for f in features)
-
-    assert not cyclic(features), "Cycle found in dependency graph"
+    for f in features:
+        assert not has_cycle(str(f['name'])), _error_message(
+            'Cycle found in depends_on/implied_by graph', f['name'])
 
 
 def origin_trials(features):
@@ -48,10 +63,12 @@ def origin_trials(features):
     or any of its dependencies are in origin trial. Propagate dependency
     tag use DFS can find all features that are in origin trial.
 
-    @param features: a List[Dict]. Each Dict much has key named 'depends_on'
+    @param features: a List[Dict]. Each Dict must have keys 'name',
+           'depends_on', 'implied_by' and 'origin_trial_feature_name'
+           (see runtime_enabled_features.json5).
     @returns Set[str(runtime feature name)]
     """
-    _runtime_features_graph_sanity_check(features)
+    _validate_runtime_features_graph(features)
 
     origin_trials_set = set()
 

third_party/blink/renderer/build/scripts/make_runtime_features_utilities_unittest.py

@@ -8,81 +8,80 @@ import make_runtime_features_utilities as util
 from blinkbuild.name_style_converter import NameStyleConverter
 
 
+def _feature(name,
+             depends_on=[],
+             implied_by=[],
+             origin_trial_feature_name=None):
+    return {
+        'name': name,
+        'depends_on': depends_on,
+        'implied_by': implied_by,
+        'origin_trial_feature_name': origin_trial_feature_name
+    }
+
+
 class MakeRuntimeFeaturesUtilitiesTest(unittest.TestCase):
-    def test_cycle_in_dependency(self):
+    def test_cycle(self):
         # Cycle: 'c' => 'd' => 'e' => 'c'
-        graph = {'a': ['b'], 'b': [], 'c': ['a', 'd'], 'd': ['e'], 'e': ['c']}
-        with self.assertRaises(AssertionError):
-            util.origin_trials([{
-                'name': name,
-                'depends_on': deps
-            } for name, deps in graph.items()])
+        with self.assertRaisesRegexp(
+                AssertionError, 'Cycle found in depends_on/implied_by graph'):
+            util.origin_trials([
+                _feature('a', depends_on=['b']),
+                _feature('b'),
+                _feature('c', implied_by=['a', 'd']),
+                _feature('d', depends_on=['e']),
+                _feature('e', implied_by=['c'])
+            ])
 
     def test_bad_dependency(self):
-        with self.assertRaises(AssertionError):
-            util.origin_trials([{'name': 'a', 'depends_on': 'x'}])
+        with self.assertRaisesRegexp(AssertionError,
+                                     'a: Depends on non-existent-feature: x'):
+            util.origin_trials([_feature('a', depends_on=['x'])])
+
+    def test_bad_implication(self):
+        with self.assertRaisesRegexp(AssertionError,
+                                     'a: Implied by non-existent-feature: x'):
+            util.origin_trials([_feature('a', implied_by=['x'])])
+        with self.assertRaisesRegexp(
+                AssertionError,
+                'a: A feature must be in origin trial if implied by an origin trial feature: b'
+        ):
+            util.origin_trials([
+                _feature('a', implied_by=['b']),
+                _feature('b', origin_trial_feature_name='b')
+            ])
+
+    def test_both_dependency_and_implication(self):
+        with self.assertRaisesRegexp(
+                AssertionError,
+                'c: Only one of implied_by and depends_on is allowed'):
+            util.origin_trials([
+                _feature('a'),
+                _feature('b'),
+                _feature('c', depends_on=['a'], implied_by=['b'])
+            ])
 
-    def test_in_origin_trials_flag(self):
+    def test_origin_trials(self):
         features = [
-            {
-                'name': NameStyleConverter('a'),
-                'depends_on': [],
-                'origin_trial_feature_name': None
-            },
-            {
-                'name': NameStyleConverter('b'),
-                'depends_on': ['a'],
-                'origin_trial_feature_name': 'OriginTrials'
-            },
-            {
-                'name': NameStyleConverter('c'),
-                'depends_on': ['b'],
-                'origin_trial_feature_name': None
-            },
-            {
-                'name': NameStyleConverter('d'),
-                'depends_on': ['b'],
-                'origin_trial_feature_name': None
-            },
-            {
-                'name': NameStyleConverter('e'),
-                'depends_on': ['d'],
-                'origin_trial_feature_name': None
-            },
+            _feature(NameStyleConverter('a')),
+            _feature(
+                NameStyleConverter('b'),
+                depends_on=['a'],
+                origin_trial_feature_name='b'),
+            _feature(NameStyleConverter('c'), depends_on=['b']),
+            _feature(NameStyleConverter('d'), depends_on=['b']),
+            _feature(NameStyleConverter('e'), depends_on=['d'])
         ]
         self.assertSetEqual(util.origin_trials(features), {'b', 'c', 'd', 'e'})
 
-        features = [{
-            'name': 'a',
-            'depends_on': ['x'],
-            'origin_trial_feature_name': None
-        },
-                    {
-                        'name': 'b',
-                        'depends_on': ['x', 'y'],
-                        'origin_trial_feature_name': None
-                    },
-                    {
-                        'name': 'c',
-                        'depends_on': ['y', 'z'],
-                        'origin_trial_feature_name': None
-                    },
-                    {
-                        'name': 'x',
-                        'depends_on': [],
-                        'origin_trial_feature_name': None
-                    },
-                    {
-                        'name': 'y',
-                        'depends_on': ['x'],
-                        'origin_trial_feature_name': 'y'
-                    },
-                    {
-                        'name': 'z',
-                        'depends_on': ['y'],
-                        'origin_trial_feature_name': None
-                    }]
-
+        features = [
+            _feature('a'),
+            _feature('b', depends_on=['x', 'y']),
+            _feature('c', depends_on=['y', 'z']),
+            _feature('x', depends_on=['a']),
+            _feature('y', depends_on=['x'], origin_trial_feature_name='y'),
+            _feature('z', depends_on=['y'])
+        ]
         self.assertSetEqual(util.origin_trials(features), {'b', 'c', 'y', 'z'})
 
 

third_party/blink/renderer/build/scripts/templates/runtime_enabled_features.cc.tmpl

@@ -87,15 +87,19 @@ bool RuntimeEnabledFeatures::{{feature.name}}Enabled(const FeatureContext* conte
   if (!RuntimeEnabledFeatures::{{depends_on}}Enabled(context))
     return false;
   {% endfor %}
-  if (RuntimeEnabledFeatures::{{feature.name}}EnabledByRuntimeFlag())
+  {% for implied_by in feature.implied_by %}
+  if (RuntimeEnabledFeatures::{{implied_by}}Enabled(context))
     return true;
-{% if not feature.origin_trial_feature_name %}
+  {% endfor %}
+  if ({{feature.data_member_name}})
+    return true;
+  {% if not feature.origin_trial_feature_name %}
   // The feature does not have an origin trial name and its runtime flag
   // is not enabled.
   return false;
-{% else %}
+  {% else %}
   return context && context->FeatureEnabled(OriginTrialFeature::k{{feature.name}});
-{% endif %}
+  {% endif %}
 }
 
 {% endfor %}

third_party/blink/renderer/build/scripts/templates/runtime_enabled_features.h.tmpl

@@ -63,8 +63,18 @@ class PLATFORM_EXPORT RuntimeEnabledFeatures {
   {% for feature in features %}
   {% if not feature.in_origin_trial %}
   static void Set{{feature.name}}Enabled(bool enabled) { {{feature.data_member_name}} = enabled; }
-  static bool {{feature.name}}Enabled() { return {{feature.enabled_condition}}; }
-  static bool {{feature.name}}Enabled(const FeatureContext*) { return {{feature.enabled_condition}}; }
+  static bool {{feature.name}}Enabled() {
+    {% for depends_on in feature.depends_on %}
+    if (!{{depends_on}}Enabled())
+      return false;
+    {% endfor %}
+    {% for implied_by in feature.implied_by %}
+    if ({{implied_by}}Enabled())
+      return true;
+    {% endfor %}
+    return {{feature.data_member_name}};
+  }
+  static bool {{feature.name}}Enabled(const FeatureContext*) { return {{feature.name}}Enabled(); }
 
   {% endif %}
   {% endfor %}
@@ -81,7 +91,7 @@ class PLATFORM_EXPORT RuntimeEnabledFeatures {
 
   {% for feature in origin_trial_controlled_features %}
   static void Set{{feature.name}}Enabled(bool enabled) { {{feature.data_member_name}} = enabled; }
-  static bool {{feature.name}}EnabledByRuntimeFlag() { return {{feature.enabled_condition}}; }
+  static bool {{feature.name}}EnabledByRuntimeFlag() { return {{feature.name}}Enabled(nullptr); }
   static bool {{feature.name}}Enabled(const FeatureContext*);
 
   {% endfor %}

third_party/blink/renderer/platform/BUILD.gn

@@ -1897,6 +1897,7 @@ jumbo_source_set("blink_platform_unittests_sources") {
     "peerconnection/two_keys_adapter_map_unittest.cc",
     "peerconnection/webrtc_audio_sink_test.cc",
     "peerconnection/webrtc_video_track_source_test.cc",
+    "runtime_enabled_features_test.cc",
     "text/bidi_resolver_test.cc",
     "text/bidi_test_harness.h",
     "text/capitalize_test.cc",

third_party/blink/renderer/platform/runtime_enabled_features.json5

@@ -1638,6 +1638,20 @@
       name: "SystemWakeLock",
       status: "experimental",
     },
+    // For unit tests.
+    {
+      name: "TestFeature",
+    },
+    // For unit tests.
+    {
+      name: "TestFeatureDependent",
+      depends_on: ["TestFeatureImplied"],
+    },
+    // For unit tests.
+    {
+      name: "TestFeatureImplied",
+      implied_by: ["TestFeature"],
+    },
     {
       name: "TextDetector",
       status: "experimental",