1553982 - Use a multi-instance lock/semaphore to track the number of running instances of an installation so we can prevent updating a running instance out from under it

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement, P1)

Description

[Robert Strong (they/them - no direct email)]

Some time in the past mhowell suggested using a semaphore to determine whether there are other instances running. This would allow us to lessen the issue where launching an additional instance of the same installation will perform an update and thereby require the other instance to restart. Below is a very simple flow of how I think this could work.

1. Create or open the semaphore on startup.
2. Use the Install hash for the name.
3. Prefix with Global\ on Windows so it is available to all users
4. The security of the semaphore will need to allow other users to work with it (need to figure out which users should be able to). This would allow other processes to DOS the update.
5. Permissions will need to be set on the Linux and OS X semaphores files so other users can use it. This would allow other processes to DOS the update.
6. For exits and crashes it will need to release the semaphore and it should close the handle when the instance count is 0. Does anyone know if this will be an issue in our crash handling code?
7. After X amount of time has passed (channel specific) based on a user pref (prevents other user processes from preventing this) without updating due to the semaphore force the update. This will likely be done with a new update status and this will mitigate the ability to DOS the update.
8. UI will most likely be shown when it is going to force the update.
9. Change the update restart UI or possibly display a warning so the user is informed that clicking the button will force the update.

Note: Some people have had issues with users creating semaphores on Windows Terminal server so it should ignore the semaphore when it can't be created or opened.

I think this method should also be sufficient for the Update Agent to determine if there is a running instance for an update.

Comment 2

[Nick Alexander :nalexander [he/him]]

I did a few minutes digging into what we have in tree that's relevant to this ticket. For context, we want:

1. to interact with the new semaphore across all of our platforms (Windows, macOS, Linux);
2. from at least two different processes -- Firefox and the background update agent -- that do not have any relationship with each other;
3. with one of the processes, the background update agent, being written in Rust and not having the Firefox-y infrastructure to hand.

There are many semaphore implementations in the tree. Most aren't intended to address point 2: they're sem_init (under POSIX). In particular ipc/glue/CrossProcessSemaphore.cpp expects to pass shared memory between processes, which isn't pleasant here, since there's no parent/child relationship. NSPR has an implementation but it's unused in-tree (and NSPR is large for this small use case). There are a many Rust implementations but most aren't intended for global sharing (i.e., with sem_open (under POSIX) semantics).

I conclude that the most sensible thing is to wrap POSIX and Windows semaphores directly. It would be nice to share the code but it's a little awkward to have a little C++ RAII class that can also be used from Rust.

Comment 3

[Molly Howell (she/her) [:mhowell]]

Attachment: Bug 1553982 Part 0 - Fix a few unified build issues that crop up when adding files in this directory. r=bytesized

Comment 4

[Molly Howell (she/her) [:mhowell]]

Attachment: Bug 1553982 Part 1 - Manage "multi-instance locks" that can be created by any component. r=bytesized

Depends on D95623

Comment 5

[Molly Howell (she/her) [:mhowell]]

Attachment: Bug 1553982 Part 2 - Use a multi-instance lock to implement an update sync manager XPCOM component. r=bytesized

Depends on D95626

Comment 6

[Molly Howell (she/her) [:mhowell]]

Attachment: Bug 1553982 Part 3 - Wait for the update lock before performing update operations. r=bytesized

Depends on D95627

Comment 7

[Molly Howell (she/her) [:mhowell]]

Attachment: Bug 1553982 Part 4 - Display a prompt when waiting for the update lock expires. r=bytesized

Depends on D95628

Comment 8

[Molly Howell (she/her) [:mhowell]]

Attachment: Bug 1553982 Part 5 - Add a test for the update lock and make existing tests support it. r=bytesized

Depends on D95629

Comment 9

[Molly Howell (she/her) [:mhowell]]

Attachment: Data review request

Comment 10

[Chris H-C :chutten]

Comment on attachment 9189061 [details]
Data review request

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Molly Howell is responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---

Result: datareview+

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91bc72bf7a1b
Part 0 - Fix a few unified build issues that crop up when adding files in this directory. r=bytesized
https://hg.mozilla.org/integration/autoland/rev/63f834f25945
Part 1 - Manage "global semaphores" that can be created by any component. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/e9e14538b7ff
Part 2 - Use a global semaphore to implement an update sync manager XPCOM component. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/1299e017328e
Part 3 - Wait for the semaphore before performing update operations. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/bfea27666adf
Part 4 - Display a prompt when waiting for the update semaphore expires. r=bytesized,fluent-reviewers,flod
https://hg.mozilla.org/integration/autoland/rev/a207ff8ae135
Part 5 - Add a test for the update semaphore and make existing tests support it. r=bytesized,nalexander

Comment 12

[Razvan Maries]

Backed out for build bustages.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=a207ff8ae135478dac66d7928d41a4b729a7c8f2&selectedTaskRun=PF7Kbdd9TGykMzpUUFPstQ.0

Log: https://treeherder.mozilla.org/logviewer?job_id=323479356&repo=autoland&lineNumber=32308

Backout: https://hg.mozilla.org/integration/autoland/rev/b6232a49dc3921b1f8bb1956e30b312359fdc46b

Comment 13

[Molly Howell (she/her) [:mhowell]]

The failures are on Android in the global semaphore (part 1); the global semaphore is not used on Android in this stack, and it probably never will be used there, so I'm going to try disabling building it. Will reland if that works.

Comment 14

[Molly Howell (she/her) [:mhowell]]

According to https://treeherder.mozilla.org/jobs?repo=try&revision=d4f0ca4009826d0ef1f398ea7b68c54e070da9eb that attempt has succeeded.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5040354e75c2
Part 0 - Fix a few unified build issues that crop up when adding files in this directory. r=bytesized
https://hg.mozilla.org/integration/autoland/rev/01d41760db29
Part 1 - Manage "global semaphores" that can be created by any component. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/9dcf78cd576f
Part 2 - Use a global semaphore to implement an update sync manager XPCOM component. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/71742fced1ba
Part 3 - Wait for the semaphore before performing update operations. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/40d67c6dfdf3
Part 4 - Display a prompt when waiting for the update semaphore expires. r=bytesized,fluent-reviewers,flod
https://hg.mozilla.org/integration/autoland/rev/78dce99516dd
Part 5 - Add a test for the update semaphore and make existing tests support it. r=bytesized,nalexander

Comment 16

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/5040354e75c2
https://hg.mozilla.org/mozilla-central/rev/01d41760db29
https://hg.mozilla.org/mozilla-central/rev/9dcf78cd576f
https://hg.mozilla.org/mozilla-central/rev/71742fced1ba
https://hg.mozilla.org/mozilla-central/rev/40d67c6dfdf3
https://hg.mozilla.org/mozilla-central/rev/78dce99516dd

Comment 17

[Caspy7]

This may have caused bug 1680935.
Seeing multiple reports including one on the #nightly:mozilla.org channel.

Comment 18

[Cristina Coroiu [:ccoroiu]]

Backed out from central for causing bug 1680935: https://hg.mozilla.org/mozilla-central/rev/7167ab3f6e8d7b77439ac1cc7d9356bbaff02510

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5234182f295
Part 0 - Fix a few unified build issues that crop up when adding files in this directory. r=bytesized
https://hg.mozilla.org/integration/autoland/rev/a65058184c20
Part 1 - Manage "multi-instance locks" that can be created by any component. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/b616a1a15f94
Part 2 - Use a multi-instance lock to implement an update sync manager XPCOM component. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/7fe2fcf9dd6b
Part 3 - Wait for the update lock before performing update operations. r=bytesized,nalexander
https://hg.mozilla.org/integration/autoland/rev/cecc2311e348
Part 4 - Display a prompt when waiting for the update lock expires. r=bytesized,fluent-reviewers,flod
https://hg.mozilla.org/integration/autoland/rev/54a9f18d1680
Part 5 - Add a test for the update lock and make existing tests support it. r=bytesized,nalexander

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f5234182f295
https://hg.mozilla.org/mozilla-central/rev/a65058184c20
https://hg.mozilla.org/mozilla-central/rev/b616a1a15f94
https://hg.mozilla.org/mozilla-central/rev/7fe2fcf9dd6b
https://hg.mozilla.org/mozilla-central/rev/cecc2311e348
https://hg.mozilla.org/mozilla-central/rev/54a9f18d1680

Comment 21

[gwarser]

I'm on Manjaro KDE and this fix does not seem to be doing anything.

I have nightly unpacked in home subfolder and I often have my "main" profile running and then start clean copy for testing: /home/rdk/Apps/firefox-nightly/firefox -profile /tmp/nightly. When I close this tmp profile, go back to "main" and try open new tab, most of the time I get blank page and sometimes "one small thing" message https://i.imgur.com/KfavQIp.png