1915216 - Implement file write locking for profiles.ini

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, enhancement)

Description

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

As part of the work to allow multiple instances to write to profiles.ini (bug 1893687), we will need to figure out how to manage multiple instances attempting to write to profiles.ini.

Today, when an instance of Firefox starts up, it gets the file stats (last-modified time and size) of profiles.ini. It then loads all the profiles.ini data into memory, possibly making modifications in memory, and then at shutdown time, only flushes changes back to the profiles.ini file if the file stats are unchanged.

:Mossop notes that we already basically have an nsProfileLock on profiles.ini at startup time, that could be extended. We should dig into this as part of this bug.

The tool we have at hand is nsProfileLock, which is a cross-profile implementation of a directory lock (not a file lock).

We could explore extending nsProfileLock to allow for individual file locking, or we could explore creating a dummy directory which we could directly use with nsProfileLock, and treat that as the write lock on the profiles.ini file.

Open questions:

• is nsProfileLock is a read-write lock, or just a write lock?
• can we extend nsProfileLock such that multiple reads, but only one write, are allowed, across all supported platforms?
• what happens at startup time if profiles.ini is being written? will the OS throw?
• does nsProfileLock behave nicely in a networked filesystem, or are there obscure currently low-priority bugs we might need to fix before multiple profiles support makes them much more noticeable?

A note on backwards version compatibility: existing Firefox versions record the file stats at startup time, and at shutdown time, verify the file stats have not changed before they flush changes to disk; if the file has been modified, they give up and do not write to the file. The patch in 1893867 eagerly updates the file stats as soon as Flush is called, so as soon as any instance with that patch applied attempts an async flush, older versions should be effectively locked out. (TBD what happens with pre-67 Firefoxes which do naively overwrite profiles.ini: I'm not sure if they do the file stats check before writing, or how they would handle an error thrown by the OS if the file is locked for writing. We'll need to investigate that, perhaps in a separate bug.)

Comment 1

[Dave Townsend [:mossop]]

(In reply to Jared Hirsch [:jhirsch] (he/him) (Needinfo please) from comment #0)

As part of the work to allow multiple instances to write to profiles.ini (bug 1893687), we will need to figure out how to manage multiple instances attempting to write to profiles.ini.

Today, when an instance of Firefox starts up, it gets the file stats (last-modified time and size) of profiles.ini. It then loads all the profiles.ini data into memory, possibly making modifications in memory, and then at shutdown time, only flushes changes back to the profiles.ini file if the file stats are unchanged.

:Mossop notes that we already basically have an nsProfileLock on profiles.ini at startup time, that could be extended. We should dig into this as part of this bug.

This lock is nsRemoteService::LockStartup. and since bug 1892400 we can do so at any point during runtime. We should basically use that as then we will correctly lock against other instances of Firefox starting up, but we may want to do something smarter than just carrying on anyway if we can't acquire the lock and some protection against re-entrency. Possibly also making the polling async.

The tool we have at hand is nsProfileLock, which is a cross-profile implementation of a directory lock (not a file lock).

It is, strictly speaking, a file lock. It creates a file in the directory and then locks it.

We could explore extending nsProfileLock to allow for individual file locking, or we could explore creating a dummy directory which we could directly use with nsProfileLock, and treat that as the write lock on the profiles.ini file.

Open questions:

• is nsProfileLock is a read-write lock, or just a write lock?

Neither really, it's just a conceptual lock that only one process can acquire at a time.

• can we extend nsProfileLock such that multiple reads, but only one write, are allowed, across all supported platforms?

No, but I also don't think we need to.

• what happens at startup time if profiles.ini is being written? will the OS throw?

It waits for five seconds and then carries on anyway. We shouldn't be spending more than 5 seconds writing to the disk.

• does nsProfileLock behave nicely in a networked filesystem, or are there obscure currently low-priority bugs we might need to fix before multiple profiles support makes them much more noticeable?

Comment 2

[Dave Townsend [:mossop]]

Attachment: Bug 1915216: Add a method to asynchronously write the important data about the current profile to the INI file on disk. r=glandium!,jhirsch!,pehrsons!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46f4c195d34a
Add a method to asynchronously write the important data about the current profile to the INI file on disk. r=glandium,jhirsch,pehrsons,backup-reviewers,mconley

Comment 5

[Norisz Fay [:noriszfay]]

Backed out for causing async related xpcshell failures

Backout link

Push with failures

Failure log

Comment 6

[Dave Townsend [:mossop]]

Attachment: Bug 1915216: Use a thread safe weak pointer for safe release of nsStartupLock. r=pehrsons!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe441360b591
Add a method to asynchronously write the important data about the current profile to the INI file on disk. r=glandium,jhirsch,pehrsons,backup-reviewers,mconley

Comment 8

[Atila Butkovits]

Backed out for causing leaks at ThreadSafeWeakReference.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d9805f1059e476496c4c050099543e7e310bfd95

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

Failure log: https://treeherder.mozilla.org/logviewer?job_id=478735997&repo=autoland&lineNumber=2429

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d04c781d28a4
Add a method to asynchronously write the important data about the current profile to the INI file on disk. r=glandium,jhirsch,pehrsons,backup-reviewers,mconley

Comment 10

[Cristina Horotan [:chorotan]]

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