Close #26041: Re-set TrackingProtectionPolicy after Nimbus SDK is initialized

[jonalmeida]

There are three issues here that we have uncovered while investigating
this bug:

1. Settings.kt has a lazy block around enabledTotalCookieProtection
   which ends up caching the first result it evaluates.
2. The FeatureHolder within FxNimbus caches the incorrectly
   evaluated value and returns this value hence forth.
3. Nimbus is not ready to return a result for an engine experiment
   when we need it early on in the dependency tree initialization.

There are multiple systems that require engine to be initialized for
them to work (e.g. Glean, Profiler, concept-fetch). In our TCP,
experiment, we need to apply these engine settings during the engine
initialization. So when we try and evaluate Nimbus that early on, it
has not had time to initialize itself correctly or even use the
engine's concept-fetch client to return the correct experiment result.
This bug is made worse because of the first two caching bugs where we
are always holding onto a cached value of the wrong result.

Our temporary solution is to:

1. Remove the lazy around Settings.enabledTotalCookieProtection.
2. Set the FxNimbus.api value right after we are done initializing
   FxNimbus and NimbusApi so that all future queries to FxNimbus
   will be made against a real instance of NimbusApi. This is a
   short-term fix for the FeatureHolder caching bug.
3. Set a new TrackingProtectionPolicy that will evaluate Nimbus now
   that it is in the correct state when receive the
   NimbusInterface.Observer.onUpdatesApplied.

Co-authored-by: jhugman
Co-authored-by: Christian Sadilek

Pull Request checklist

• Tests: This PR includes thorough tests or an explanation of why it does not
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

• QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

1. Click on Checks at the top of the PR page.
2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
3. Click on the build-debug task.
4. Click on View task in Taskcluster in the new DETAILS section.
5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #26041

[jhugman]

I think this is a good minimal fix for TCP. I might also recommend we flip the default for TCP in the nimbus.fml.yaml to true. I've asked bugs to be filed if they haven't already for the tabs prioritization.

[jhugman]

This is fixed mozilla/application-services#4999 first released in v93.6.0

It should already be in main, and releases_v104.0.0.

Added mozilla-mobile/android-components#12567 to land on main, this should be uplifted to releases_v104.0.0.

[jhugman]

The FeatureHolder within FxNimbus has a lazy block around it
which also ends up caching the first result it evaluates.

This isn't quite accurate: prior to v93.6.0, the feature holder didn't get to hear about a call to connect the Nimbus SDK (the Rust code that connects to the server and calculates enrollment) if it had been used already.

So if the feature was used before the very earliest steps to initialize the SDK had been started, then we return the default values because if the SDK isn't available, the feature just returns that defaults every time its called.

[jonalmeida]

This is fixed mozilla/application-services#4999 first released in v93.6.0

It should already be in main, and releases_v104.0.0.

Added mozilla-mobile/android-components#12567 to land on main, this should be uplifted to releases_v104.0.0.

@jhugman Upgrading to the latest version (that is in mozilla-mobile/android-components#12592), I'm still not able to get the experiment to work without adding this line.

At this point, I'm more inclined to leaving this line in here and filing a bug to have it fixed.

[jonalmeida]

Filed mozilla/application-services#5075

[csadilek]

@jonalmeida @jhugman I don't think we can land the FxNimbus.api = this assignment because it's not thread-safe. It's a public var without any visibility guarantees, i.e., it's not volatile or synchronized, and also executed on an IO thread here.

Maybe that's also part of the problem? I know @jhugman made FeatureHolder thread-safe but this assignment isn't (to be fair I'd think it's also not meant to be called like this.)

[jhugman]

to be fair I'd think it's also not meant to be called like this

This is correct. This is a workaround, using the old API, which we've deprecated, and are trying to remove.

As discussed, the fix is to upgrade the FML tooling to at least v93.6.0 (currently on v93.5.0).

That said, there is an issue with upgrading to latest (currently v93.8.0) due to mozilla/application-services#5079 .

[csadilek]

@jhugman Above @jonalmeida mentioned that upgrading to the latest version didn't fix the problem. We should take another look together, please.

[jhugman]

Added further comments in the review.

[jhugman]

@jonalmeida asks: why does setting FxNimbus.api = this work?

FeatureHolders are instantiated lazily, which is why they get the right getSdk when they’re used after the initialize is called.

However, if they’re instantiated before the initialize method is called, they get the initial value of getSdk.

The initial value of getSdk is { api }.

So changing the value of api allowed a FeatureHolder that the initial value getSdk to get a different result when calling the same getSdk later on.

For more context: FxNimbus.api = nimbus was the in first version of wiring the generated code to the Nimbus SDK. It wasn't satisfactory, precisely for the reasons @csadilek raises, so we deprecated it in favour of a newer threadsafe mechanism.

That it still works is a lucky accident for us, but definitely the not something we want anyone to rely on.

[csadilek]

FeatureHolders are instantiated lazily, which is why they get the right getSdk when they’re used after the initialize is called. However, if they’re instantiated before the initialize method is called, they get the initial value of getSdk. The initial value of getSdk is { api }.

@jhugman But this workaround here calls FxNimbus.api = this after initialize is called, and after the FeatureHolder (for tracking protection) was instantiated. Why does this fix it? You mentioned the problem occurs when the FeatureHolders are initialized before initialize is called.

[jonalmeida]

We've landed the 83.6.0 Nimbus plugin change, so we can safely land this change as well.

[jonalmeida]

QA verified the fix we have in Nightly, so let's uplift this to the 104.0 release; the AC uplift has already landed (albeit it needs a new AC release for it to be used).

@Mergifyio backport releases_v104.0.0

[jonalmeida]

@Mergifyio backport releases_v104.0.0

[mergify]

backport releases_v104.0.0

✅ Backports have been created

• #26426 Close #26041: Re-set TrackingProtectionPolicy after Nimbus SDK is initialized (backport #26228) has been created for branch releases_v104.0.0