1875331 - [meta] Deprecate and remove isEarlyStartup and the C++ feature API

Status: NEW

(Firefox :: Nimbus Desktop Client, task)

Description

[Beth Rennie [:beth] (she/her) - OOO until Sept 14]

The isEarlyStartup flag was added to Nimbus so that feature values could be (a) read early on in startup and (b) read by C++ consumers. It accomplished this via setting specific prefs containing experiment metadata (i.e., the entire recipe) and for each variable in a feature.

However as of bug 1782150 we have setPref support for variables, which allows experiments and rollouts to set specific prefs with the values of Nimbus variables at enrollment time.

The setPref feature is more powerful and should be used instead of isEarlyStartup so that only the variables required to be read during startup can be written to prefs and we can avoid unnecessary pref writes. This also satisfies the use case of the C++ API, so the majority of the use cases can be replaced by direct pref reads. For the case of OnUpdate, OffUpdate, and RecordExposureEvent, these can be replaced by a small XPCOM shim in the general case, or pref listeners for OnUpdate and OffUpdate.

Comment 1

[Punam Dahiya [:pdahiya]]

@barret aboutwelcome recipes are too long to be stored reliably in a pref e.g. adding JSON object here as string value of browser.aboutwelcome.screens throws error in browser console and fails to get picked on opening about:welcome.

Warning: attempting to write 13170 bytes to preference browser.aboutwelcome.screens. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file.

about:welcome heavily relies on getting JSON recipe values in time during first startup before about:welcome opens, my concerns here
do you think removing isEarlyStartup API in favor of setPref will be able to reliably provide JSON values on first and subsequent startups. Thanks

Comment 2

[Beth Rennie [:beth] (she/her) - OOO until Sept 14]

This warning is not coming from Nimbus, but from the pref library itself. There is a warning at 4kB and a hard error at 1mB. Using setPref here instead of isEarlyStartup would still cause large pref writes and still cause these warnings.

You should investigate if you actually need prefs for this use case. It is quite possible that Nimbus will have already loaded the data store before we open about:welcome. isEarlyStartup was only intended for things that need to read before this point, which is usually very early stuff that occurs before first paint of the browser window.

Comment 3

[Punam Dahiya [:pdahiya]]

(In reply to Barret Rennie [:barret] (they/them) from comment #2)

You should investigate if you actually need prefs for this use case. It is quite possible that Nimbus will have already loaded the data store before we open about:welcome. isEarlyStartup was only intended for things that need to read before this point, which is usually very early stuff that occurs before first paint of the browser window.

My understanding about:welcome experiments are read before first paint of browser window. Is the plan to keep features in list here continue to use isEarlyStartup ?

Comment 4

[Beth Rennie [:beth] (she/her) - OOO until Sept 14]

(In reply to Punam Dahiya [:pdahiya] from comment #3)

(In reply to Barret Rennie [:barret] (they/them) from comment #2)

You should investigate if you actually need prefs for this use case. It is quite possible that Nimbus will have already loaded the data store before we open about:welcome. isEarlyStartup was only intended for things that need to read before this point, which is usually very early stuff that occurs before first paint of the browser window.

My understanding about:welcome experiments are read before first paint of browser window.

isEarlyStartup wont affect experiments that are fetched during first startup (--first-startup) because Nimbus starts up and fetches experiments before first paint (i.e., the nimbus api is already available). isEarlyStartup only matters in the case where the nimbus API is not ready.

Is the plan to keep features in list here continue to use isEarlyStartup ?

No, the plan is to migrate every feature on that list to use setPref variables and remove the isEarlyStartup flag. That list is there so we cannot add new features that use isEarlyStartup while the migration is occuring.

Comment 5

[Ed Lee :Mardak]

Bug 1680371 introduced const SYNC_ACCESS_FEATURES = ["newtab", "aboutwelcome"]; if that's useful to determine if aboutwelcome nimbus feature still wants to be isEarlyStartup where maybe it doesn't need to anymore given that aboutwelcome experiments always(?) use isFirstStartup targeting?