1455464 - Normandy.uninit() is async, but does not block shut down

Status: RESOLVED FIXED

(Firefox :: Normandy Client, enhancement, P2)

Description

[Michael Cooper [:mythmon]]

In bug 1440782, Gijs notices that Normandy shutdown process doesn't block Firefox's shutdown. This means that if Normandy takes a while, it could lose data. Not great.

To quote Gijs:

> I guess the correct thing to do would be to ensure we add a shutdown blocker
> (see https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/AsyncShutdown.jsm)
> that gets resolved when we've finished saving state.

Comment 1

[:Gijs (he/him)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1614681#c13 suggests we could drop the shutdown sync of the prefs, but not some of the other tasks.

Comment 2

[:Gijs (he/him)]

There's actually a shutdown blocker in the CleanupManager, and everything else is synchronous, and the observer service notification removals are actually probably no-ops, tbh. So I don't think we need to do anything here besides removing the preference rollouts sync code...

Comment 3

[:Gijs (he/him)]

(In reply to Michael Cooper [:mythmon] from bug 1614681 comment #11)

We have an incremental sync of these prefs as the related values change. The on-shutdown sync is a backup in case they get out of sync.

Can you elaborate on this? I don't see anything observing the prefs for changes, and the only caller of saveStartupPrefs is uninit, but maybe I'm misunderstanding what you mean?

It does seem like the preference experiment code registers its own saveStartupPrefs stuff via the cleanup manager, which has a shutdown blocker, but the preference rollout code doesn't, and uses a hardcoded call from Normandy.uninit. Perhaps the right fix is switching the rollout code to also use the cleanup manager and removing the direct call from Normandy.uninit ?

Comment 4

[Michael Cooper [:mythmon]]

Can you elaborate on this? I don't see anything observing the prefs for changes, and the only caller of saveStartupPrefs is uninit, but maybe I'm misunderstanding what you mean?

When we start an experiment, we call saveStartupPrefs. For rollouts, we call saveStartupPrefs at the end of every Normandy session (this would could probably be a bit more effective).

It does seem like the preference experiment code registers its own saveStartupPrefs stuff via the cleanup manager, which has a shutdown blocker, but the preference rollout code doesn't, and uses a hardcoded call from Normandy.uninit. Perhaps the right fix is switching the rollout code to also use the cleanup manager and removing the direct call from Normandy.uninit ?

This seems like a good idea, yeah. It makes the preference actions more self-contained, which seems nice. I'll do this.

Comment 5

[:Gijs (he/him)]

Hey, how are you getting on with this bug - did the plan from comment #3 / comment #4 not work out? Is there anything I can do to help? The intermittent in bug 1614681 seems to basically be caused by this issue, and has gone up in frequency for some reason...

Comment 6

[Michael Cooper [:mythmon]]

Attachment: Bug 1455464 - Use Normandy's cleanup manager for Preference Rollouts r=gijs

Comment 7

[Michael Cooper [:mythmon]]

Apologies for the delays. This just got delayed by other work.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c6d456eb406
Use Normandy's cleanup manager for Preference Rollouts r=Gijs

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/6c6d456eb406