2064188 - Nimbus 6h periodic timer may not be firing

Status: RESOLVED FIXED

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

Description

[Jared Lockhart [:jlockhart]]

While investigating issues around enrollment/unenrollment timing, I discovered that we may never be creating our periodic 6h timer.

Here the timer is setup:

https://github.com/mozilla-firefox/firefox/blob/25d7109bf565c299435dec3dd2b9e79a1ce7c15d/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs#L312-L322

Here it gates on this._enabled

https://github.com/mozilla-firefox/firefox/blob/25d7109bf565c299435dec3dd2b9e79a1ce7c15d/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs#L880-L883

But this._enabled is set true after the call to setup the timer, so does the timer ever enter?

The guard was introduced here:

https://github.com/mozilla-firefox/firefox/commit/7f62c429b15f0c868702776929a5506f37310f67#diff-44fcd7fcbdd6b7d033ec0d931165a35ad75fa2b40bb7b88713a983440f7660dbR504-R507

Which I think corresponds to a drop in the number of 'is_ready' telemetry events from a multiple of the number of sessions down to being approximately equal to the number of sessions:

https://sql.telemetry.mozilla.org/queries/125112/#303126

Please verify if this is actually the case in which case we should repair this, and maybe we can include a 'kind' field in the is_ready telemetry event that shows whether it's in startup, or the timer.

Comment 1

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

Attachment: Bug 2064188 - Ensure Nimbus update timer is set during init r?#nimbus-reviewers!

In bug 1941963, side effects were removed from
RemoteSettingsExperimentLoader.sys.mjs, making it safe to import without
triggering the creation of a global RemoteSettingsExperimentLoader.
However, that patch also gated setting the update timer based on the
RemoteSettingsExperimentLoader's enabled state. Unfortunately,
the timer was registered before the enabled flag was set, which meant
that the timer was never actually registered.

We now ensure that we set the enable state before registering the timer.
This behaviour is tracked by a new test.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/93b3cf0ca22f
https://hg.mozilla.org/integration/autoland/rev/d86f51147a1c
Ensure Nimbus update timer is set during init r=nimbus-reviewers,emcminn

Comment 3

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1941963

Comment 4

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/d86f51147a1c

Comment 5

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected, along with ESR.
:beth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta and ESR approvals.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox155 and the ESR status flag(s) to wontfix.

For more information, please visit BugBot documentation.

Comment 6

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

Attachment: Bug 2064188 - Ensure Nimbus update timer is set during init

In bug 1941963, side effects were removed from
RemoteSettingsExperimentLoader.sys.mjs, making it safe to import without
triggering the creation of a global RemoteSettingsExperimentLoader.
However, that patch also gated setting the update timer based on the
RemoteSettingsExperimentLoader's enabled state. Unfortunately,
the timer was registered before the enabled flag was set, which meant
that the timer was never actually registered.

We now ensure that we set the enable state before registering the timer.
This behaviour is tracked by a new test.

Original Revision: https://phabricator.services.mozilla.com/D319282

Comment 7

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Users will not enroll in recipes until they restart their browser, which could impact high profile experiments on newtab etc
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: Simple patch with tests
• String changes made/needed?: n/a
• Is Android affected?: no

Comment 8

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

Attachment: Bug 2064188 - Ensure Nimbus update timer is set during init

In bug 1941963, side effects were removed from
RemoteSettingsExperimentLoader.sys.mjs, making it safe to import without
triggering the creation of a global RemoteSettingsExperimentLoader.
However, that patch also gated setting the update timer based on the
RemoteSettingsExperimentLoader's enabled state. Unfortunately,
the timer was registered before the enabled flag was set, which meant
that the timer was never actually registered.

We now ensure that we set the enable state before registering the timer.
This behaviour is tracked by a new test.

Original Revision: https://phabricator.services.mozilla.com/D319282

Comment 9

[Phabricator Automation]

firefox-esr153 Uplift Approval Request

• User impact if declined/Reason for urgency: Users will not enroll in recipes until they restart their browser, which could impact high profile experiments on newtab etc
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: Simple patch with tests
• String changes made/needed?: n/a
• Is Android affected?: no

Comment 10

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: Users will not enroll in recipes until they restart their browser, which could impact high profile experiments on newtab etc
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: Simple patch with tests
• String changes made/needed?: n/a
• Is Android affected?: no

Comment 11

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

Attachment: Bug 2064188 - Ensure Nimbus update timer is set during init

In bug 1941963, side effects were removed from
RemoteSettingsExperimentLoader.sys.mjs, making it safe to import without
triggering the creation of a global RemoteSettingsExperimentLoader.
However, that patch also gated setting the update timer based on the
RemoteSettingsExperimentLoader's enabled state. Unfortunately,
the timer was registered before the enabled flag was set, which meant
that the timer was never actually registered.

We now ensure that we set the enable state before registering the timer.
This behaviour is tracked by a new test.

Original Revision: https://phabricator.services.mozilla.com/D319282

Comment 12

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: Users will not enroll in recipes until they restart their browser, which could impact high profile experiments on newtab etc
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: Simple patch with tests
• String changes made/needed?: n/a
• Is Android affected?: no

Comment 13

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

Attachment: Bug 2064188 - Ensure Nimbus update timer is set during init

In bug 1941963, side effects were removed from
RemoteSettingsExperimentLoader.sys.mjs, making it safe to import without
triggering the creation of a global RemoteSettingsExperimentLoader.
However, that patch also gated setting the update timer based on the
RemoteSettingsExperimentLoader's enabled state. Unfortunately,
the timer was registered before the enabled flag was set, which meant
that the timer was never actually registered.

We now ensure that we set the enable state before registering the timer.
This behaviour is tracked by a new test.

Original Revision: https://phabricator.services.mozilla.com/D319282

Comment 14

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/c62b2543d695
https://hg.mozilla.org/releases/mozilla-beta/rev/3fa852842e50

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/5250debffd1a

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/bc138111a32c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out from ESR140 for test failures. Given that it's nearly EOL anyway, I think we can just leave well enough alone there.
https://treeherder.mozilla.org/logviewer?job_id=586796096&repo=mozilla-esr140&task=PcXVaPnZRVGc3LHN7Z5dWQ.0&lineNumber=4969

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/releases/mozilla-esr140/rev/7cc0ffaa09f8
Revert "Bug 2064188 - Ensure Nimbus update timer is set during init" for xpcshell failures.

Comment 19

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/b2f2c41021a8
https://hg.mozilla.org/releases/mozilla-release/rev/3e72245145ac