1941963 - Importing RemoteSettingsExperimentLoader has side effects

Status: RESOLVED FIXED

(Firefox :: Nimbus Desktop Client, defect, P2)

Description

[Beth Rennie [:beth] (she/her)]

Importing RemoteSettingsExperimentLoader.sys.mjs presently creates a new _RemoteSettingsExperimentLoader(), which has the side effect of registering observers (https://searchfox.org/mozilla-central/rev/cbc10b90d92e10fd695cd75f966756ea28596308/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs#144-167). Because unit tests often make their own test RemoteSettingExperimentLoaders, this means that at any given time there can be two active RSEL listening for events. This has lead to intermittent and hard to debug test failures when making seemingly innocuous changes.

IMO this is enough of a footgun to warrant refactoring and such a refactor should make it easier for non-experts to write unit tests in Nimbus.

Comment 1

[Beth Rennie [:beth] (she/her)]

Attachment: Bug 1941963 - Remove messaging-system.rsexperimentloader.enabled pref r?chumphreys,emcminn

This pref disables the RemoteSettingsExperimentLoader, which is
something we don't ever want to do in practice. It doesn't make sense to
Nimbus enabled but not use the RemoteSettingsExperimentLoader.

Comment 2

[Beth Rennie [:beth] (she/her)]

Attachment: Bug 1941963 - Rename RemoteSettingsExperimentLoader.init/uninit to enable/disable r?chumphreys,emcminn

This wording makes it more clear that the
RemoteSettingsExperimentLoader is only ever temporarily
enabled/disabled and never actually uninitialized (because even when it
is disabled it is still waiting for notifications that would enable it).

Comment 3

[Beth Rennie [:beth] (she/her)]

Attachment: Bug 1941963 - Only register RemoteSettingsExperimentLoader timer if enabled r?chumphreys,emcminn

Previously, if the RemoteSettingsExperimentLoader was disabled (e.g.,
because one of the required prefs was false), updating the
app.normandy.run_inverval_seconds pref would still trigger timer
registration.

Additionally, if the timer was enabled and then
app.normandy.run_interval_seconds was set to 0, the existing timer
would not be removed. We now remove the existing timer when setting the
interval pref to 0.

TODO: Add tests

Comment 4

[Beth Rennie [:beth] (she/her)]

Attachment: Bug 1941963 - Do not send nimbus:studies-enabled-changed when initializing ExperimentManager r?chumphreys,emcminn

Previously in ExperimentManager.onStartup, we were always firing the
nimbus:studies-enabled-changed notification. This would cause
RemoteSettingsExperimentLoader to initialize if it was not yet
initialized, which was leading to races in startup order (e.g., the
RemoteSettingsExperimentLoader could get initialized and immediately
do call updateRecipes while we were still inside
ExperimentManager.onStartup()).

The logic has been updated so that we never fire this notification
unless the prefs have actually changed.
RemoteSettingsExperimentLoader.enable() already handles the case where
the prefs are disabled when the method is called.

This mostly affects writing unit tests, since in ExperimentAPI.init()
we enable the RemoteSettingsExperimentLoader immeidately after calling
ExperimentManager.onStartup()

Now that there is no longer a race between the
RemoteSettingsExperimentLoader and the ExperimentManager, Nimbus
unit tests have been re-ordered to always enable the loader after the
manager, which mirrors actual startup behaviour.

Comment 5

[Beth Rennie [:beth] (she/her)]

Attachment: Bug 1941963 - Disable RemoteSettingsExperimentLoader timer via pref in Nimbus unit tests r?emcminn,chumphreys

We were disabling the actual timer by stubbing it out in most unit
tests. Any test that needs to use the timer can set the interval pref to
a non-zero value.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/218a7bdac56f
Remove messaging-system.rsexperimentloader.enabled pref r=emcminn
https://hg.mozilla.org/integration/autoland/rev/07aaac9970db
Rename RemoteSettingsExperimentLoader.init/uninit to enable/disable r=emcminn
https://hg.mozilla.org/integration/autoland/rev/d79c2eab373c
Only register RemoteSettingsExperimentLoader timer if enabled r=emcminn
https://hg.mozilla.org/integration/autoland/rev/c642da2ca414
Do not send nimbus:studies-enabled-changed when initializing ExperimentManager r=emcminn
https://hg.mozilla.org/integration/autoland/rev/b45dcec204c9
Disable RemoteSettingsExperimentLoader timer via pref in Nimbus unit tests r=emcminn

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bc740a1d122
Backed out 5 changesets for causing xpcshell failures in test_backgroundtask_experiments.js.

Comment 8

[Serban Stanca [:SerbanS]]

Backed out for causing xpcshell failures in test_backgroundtask_experiments.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js | xpcshell return code: 0

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64638a653579
Remove messaging-system.rsexperimentloader.enabled pref r=emcminn
https://hg.mozilla.org/integration/autoland/rev/9678d8314c05
Rename RemoteSettingsExperimentLoader.init/uninit to enable/disable r=emcminn,nalexander
https://hg.mozilla.org/integration/autoland/rev/6b7e4a611c14
Only register RemoteSettingsExperimentLoader timer if enabled r=emcminn
https://hg.mozilla.org/integration/autoland/rev/47086df9c43f
Do not send nimbus:studies-enabled-changed when initializing ExperimentManager r=emcminn
https://hg.mozilla.org/integration/autoland/rev/9a252f3776d1
Disable RemoteSettingsExperimentLoader timer via pref in Nimbus unit tests r=emcminn

Comment 10

[amarc]

https://hg.mozilla.org/mozilla-central/rev/64638a653579
https://hg.mozilla.org/mozilla-central/rev/9678d8314c05
https://hg.mozilla.org/mozilla-central/rev/6b7e4a611c14
https://hg.mozilla.org/mozilla-central/rev/47086df9c43f
https://hg.mozilla.org/mozilla-central/rev/9a252f3776d1