1931110 - Update async flush behavior to flush the group profile, not the current profile

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

In the code review discussion for Bug 1918813, it was discovered that the "last writer wins" semantics may not be correctly implemented for updating the default profile for a group.

In the case where a selectable profile has received application focus and is trying to become the default for the profile group, it appears that SelectableProfileService.#attemptFlushProfileService() can fail:

1. if the profiles.ini file has been updated since the selectable profile was launched, the call to nsToolkitProfileService.asyncFlush will fail, because that function throws if GetIsListOutdated returns true.
2. as a backup, we then attempt to call nsToolkitProfileService.asyncFlushCurrentProfile , but the call to nsToolkitProfileService.asyncFlushCurrentProfile will fail, as the C++ function throws if mCurrent is null, and mCurrent is always null for a selectable profile.

I have a fix in mind here: we can adjust the null-check in nsToolkitProfileService.asyncFlush to skip the GetIsListOutdated check if we are in a selectable profile, and we can adjust the unnecessary write check in SelectableProfileService.#attemptFlushProfileService to bail out only if both the group toolkit profile's rootDir equals the current profile's rootDir, but also (new condition) that the file is not outdated--if the file is outdated, it's possible someone else updated profiles.ini but we haven't read in the latest contents. Pushing a patch now with these fixes added.

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Bug 1931110 - Update async flush behavior to flush the group profile, not the current profile. r?mossop!,niklas

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mossop, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Note that in the review :mossop suggested an alternative fix:

So this basically undoes bug 1915216 which means we start clobbering the profiles.ini file.

How about instead we change AsyncFlushCurrentProfile to AsyncFlushGroupProfile so it is checking mGroupProfile instead of mCurrentProfile?

Updating the bug title accordingly

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/172d1320ab60
Update async flush behavior to flush the group profile, not the current profile. r=mossop

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/172d1320ab60