Support upstream features override (in code) with ability to Griffin-override on top

[goodov]

The variations seed doesn't support features state apply at the first start. We can implement some chromium_src-based change to alter the default state of Chromium features and still be able to control it via Griffin. Our current approach with appending --enable/disable-features doesn't allow Griffin overrides.

Reasoning from @pes10k:

Could we change our deployment to make the features enabled by default in code, and then use griffin to disable only if needed? I know that for users it wont make a difference, but it’ll be increasingly important to make sure researchers / bloggers / measurers accurately understand and can replicate the privacy protections we’re providing.

Note: this does not replace master_preferences. The problem with master_preferences is that it's not supported by selenium-webdriver for example. In reality, all experiments and research made with selenium-webdriver won't get the actual features state we're using in production.

[goodov]

Any feature override should be spread across all processes. Currently the command-line approach passes the command line everywhere and this logic works properly. If we want to drop the command-line approach but still keep the feature override uniformity across all processes, we have these options:

1. Make base::Featured::default_state mutable and change it at init phase (when global objects are constructed). - Pros:

• zero runtime overhead
• almost-zero startup overhead
• override directly references a variable (compile-time feature existence check)

• Cons:
  • default_state will be mutable
  • each override should be placed next to the feature (in the same .cc file)
  • need to enable global constructors:

  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wglobal-constructors"
  ...
  #pragma clang diagnostic pop
  

const_cast cannot be used instead of mutable because it's UB doing it on const object:

Except that any class member declared mutable can be modified, any attempt to modify a const object during its lifetime results in undefined behavior.

The override will look like this:

brave/chromium_src/net/base/features.cc:

#include "../../../../net/base/features.cc"

OVERRIDE_FEATURE_DEFAULT_STATE(
    net::features::kSplitHostCacheByNetworkIsolationKey,
    base::FEATURE_ENABLED_BY_DEFAULT)

2. Add a container (flat_map) which we fill at init phase with feature references and overridden states. Then lookup into this map each time we check for default_state.

• Pros:
  • default_state stays const
  • override directly references a variable
• Cons:
  • non-zero runtime overhead
  • non-zero startup overhead
  • each override should be placed next to the feature (in the same .cc file)
  • need to enable global constructors

The override will look identical to the 1-st option.

3. Add a custom base::Feature constructor which will lookup into predefined const char* arrays with enabled and disabled features and set default_state accordingly.

• Pros:
  • default_state stays const
  • overrides can be put in one place
  • zero runtime overhead
  • zero startup overhead (constexpr)
• Cons:
  • overrides are written using "string" representation of features
  • no direct variable reference, no compile-time check if a feature exists at all
  • all overrides should be placed in feature_list.h, because constexpr constructor should see all code, each change → long rebuild

The override will look like like this:

brave/chromium_src/base/feature_list.h:

constexpr FeatureState GetDefaultState(const char* feature_name,
                                       FeatureState default_state) {
  constexpr const base::StringPiece kEnabledFeatures[] = {
      "SplitHostCacheByNetworkIsolationKey",
  };
  for (size_t i = 0; i < base::size(kEnabledFeatures); ++i) {
    if (kEnabledFeatures[i] == feature_name)
      return FEATURE_ENABLED_BY_DEFAULT;
  }
  // same for disabled features
  return default_state;
}

struct Feature {
  ...
  constexpr Feature(const char* name, FeatureState default_state)
      : name(name), default_state(GetDefaultState(name, default_state)) {}
}

#include "../../../base/feature_list.h"

Other options

I also thought about other options like redefining the feature variable name and adding something to it, for ex.:

#define kSplitHostCacheByNetworkIsolationKey something_something_magic_here

but it all breaks when we have a FeatureParam that references the feature.

[goodov]

Tests revealed that some parts of Chromium has enabled Wglobal-constructors warning which forbids any non trivial global constructors. We can avoid it by doing #pragma clang diagnostic stuff, but this is not the best solution.

I think that the best approach (zero overhead at runtime + feature existence validation) is a hybrid which includes constexpr string-based approach + some python linter on top which will validate strings existence in files. This can be implemented by adding a comment with a filepath which will be used to lint the feature name:

  constexpr const base::StringPiece kEnabledFeatures[] = {
      "SplitHostCacheByNetworkIsolationKey",  // net/base/features.cc
  };

[goodov]

For QA: please find few easy-to-check features from the list and make sure they are in the same state we want it to be.

[LaurenWags]

@brave/legacy_qa this is QA/Blocked until additional information is obtained from @kjozwiak

[LaurenWags]

Reference: https://bravesoftware.slack.com/archives/C01LKMP6X36/p1640713240108000

[goodov]

Test cases

(1) Feature is enabled by default in Chromium, but disabled by default in Brave. Can be overridden via cmd line.

1. Ensure Brave doesn't have Copy link to highlight context menu item when a text on a website is selected and a context menu is triggered (mouse right click).
2. Start Brave with --enable-features=CopyLinkToText
3. Expect the menu item is available.

(2) Feature is disabled by default in Chromium, but enabled by default in Brave. Can be overridden via cmd line.

1. Ensure there is Import passwords item on brave://settings/passwords in Saved Passwords section (click the three-dots button on the right).
2. Start Brave with --disable-features=PasswordImport.
3. Expect the item has disappeared.

(3) Control Brave-overridden feature using brave://flags.

1. Ensure there is Import passwords item on brave://settings/passwords in Saved Passwords section, make sure to cleanup command line after running test case #2.
2. Open brave://flags, change Password import flag state, relaunch browser.
3. Expect the item exists or doesn't exist depending on the flag state.
4. Expect Default flag state shows the item.

[GeetaSarvadnya]

Verification PASSED on

Brave | 1.34.78 Chromium: 97.0.4692.71 (Official Build) (64-bit)
-- | --
Revision | adefa7837d02a07a604c1e6eff0b3a09422ab88d-refs/branch-heads/4692@{#1247}
OS | Windows 10 Version 21H2 (Build 19044.1415)

• Verified the test plan from Support upstream features override (in code) with ability to Griffin-override on top #18829 (comment)

Case 1: `Copy link to highlight` context menu

• Confirmed by default brave doesn't have a Copy link to highlight context menu item when a text is selected on any website
• Confirmed Copy link to highlight context menu item is shown when a text is selected on any website when brave is launched using --enable-features=CopyLinkToText

brave default	brave with cmd line

Case 2: Import passwords

• Confirmed by default brave has Import option displayed in 3dot menu under Saved Passwords in brave://settings/passwords
• Confirmed 3dot menu with Import option is disappeared when brave is launched via cmd line --disable-features=PasswordImport

brave default	brave with cmd line

Case 3: Control `Import password` via brave://flags

• Confirmed enable/disable Import password option in brave://settings/passwords is controlled via brave://flags

Password import flag	Default or Enable	Disable

---

Verified using:

Brave | 1.34.78 Chromium: 97.0.4692.71 (Official Build) (x86_64)
-- | --
Revision | adefa7837d02a07a604c1e6eff0b3a09422ab88d-refs/branch-heads/4692@{#1247}
OS | macOS Version 11.6.1 (Build 20G224)

• Verified the test plan from Support upstream features override (in code) with ability to Griffin-override on top #18829 (comment)

Case 1: `Copy link to highlight` context menu - PASSED

• Confirmed by default Brave doesn't have a Copy link to highlight context menu item when a text is selected on any website
• Confirmed Copy link to highlight context menu item is shown when a text is selected on any website when Brave is launched using --enable-features=CopyLinkToText

brave default	brave with cmd line

Case 2: Import passwords - PASSED

• Confirmed by default Brave has Import option displayed in 3dot menu under Saved Passwords in brave://settings/passwords
• Confirmed 3dot menu with Import option is disappeared when Brave is launched via cmd line --disable-features=PasswordImport

brave default	brave with cmd line

Case 3: Control `Import password` via brave://flags - PASSED

• Confirmed enable/disable Import password option in brave://settings/passwords is controlled via brave://flags

Password import flag - Default	Password import flag - Enable	Password import flag - Disable