Avoid the early/late prefs split

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

We have two ways of sending pref values from the parent process to content processes:

(1) Via the command-line: the -intPrefs, -boolPrefs, -stringPrefs, and -schedulerPrefs arguments.

(2) Via the first IPC message to the content process.

We need (1) for the subset of prefs used very early on, i.e. those that are used before the first IPC message arrives. But it has various problems.

- Having two distinct mechanisms is a pain. More code, more complexity.

- Maintenance of the list of early prefs is annoying and error-prone, e.g. see bug 1432979 for a case where some prefs are only used early in obscure circumstances. Omissions from the list can lead to pref values not being observed correctly early in a content process's life.

- It makes the command line for the content processes ugly. Especially prefs with unusual chars. See bug 1373157.

- The use of indices (rather than names) to identify prefs is fragile. See bug 1419432, where it appears that we can end up with different binaries for parent and content processes, in which the indices don't match, which causes crashes.

- The early pref values are set as default values, even when they are actually user values. (This inaccuracy is fixed when the late prefs arrive.)

- It's interfering with various ideas I have to refactor libpref.

I can see two ways to avoid the early/late split. The first is to get rid of (1), by delaying things in the content process until the first IPC message arrives. In bug 1373157 comment 6, mrbkap said:

> This is because we don't want to have to send synchronous messages on content
> process startup. During XPCOM startup, we read a bunch of prefs before a
> second (asynchronous) message could get from the parent to the child. Those
> pref values have to get to the child process somehow and we have relatively
> few mechanisms to do so.

But I talked to Bill about this in December and he seemed to think it was possible.

The second is to get rid of (2) by sending all the changed prefs via the command-line (or possibly an environment variable). Encoding them in a sufficiently compact fashion would be the challenge there.
> The second is to get rid of (2) by sending all the changed prefs via the
> command-line (or possibly an environment variable). Encoding them in a
> sufficiently compact fashion would be the challenge there.

Privacy is potentially an issue here -- some prefs may have privacy-sensitive names or values, and command lines are unprotected. Environment variables are more protected, at least on Unix.

As for size: I did some investigation, and I get about 70 changed prefs shortly after startup. These are almost entirely from system extensions which set default pref values: Shield, pdf.js, activity-stream, Pocket.
> As for size: I did some investigation, and I get about 70 changed prefs
> shortly after startup. These are almost entirely from system extensions
> which set default pref values: Shield, pdf.js, activity-stream, Pocket.

Currently we identify early prefs by indices, but we'd need to switch to pref names. The names of these 70 prefs are about 2700 bytes, with a lot of repetition due to common prefixes.
I wonder if we could use shared memory to pass the changed prefs to the child. I think that would be safe from a privacy point of view, and any size limits should be much higher than the size limits on the command line or environment variables.
The tricky part with shared memory is having data structures that don't rely on pointers, because each process is likely to have the shared memory at a different address. OTOH, that also makes it possible to use less memory, since, as you're not using pointers, you can use offsets of a type < 64 bits.
(In reply to Nicholas Nethercote [:njn] from comment #3)
> I wonder if we could use shared memory to pass the changed prefs to the
> child.

I'm working on a proposal to use LMDB in Firefox. One way of thinking about LMDB, which is a multiprocess-safe mmap-backed store, is as a nice abstraction over shared memory, with isolation provided via coordination primitives, and with optional persistence for free.

In your specific case here, the set of prefs that content processes are allowed to read (and, if you wish, write) could be stored in an LMDB database that's created and written by the main process, then opened and read from each content process. That DB could be persistent (like prefs) or transient (like the prefs copies that content processes hold). It could conceivably reduce our need for IPC to just change notifications, with no data being passed around at all.

Naturally there are a whole bunch of obstacles for approaches like these (NFS file locking behavior, content process permissions, crashed readers, address space limitations, and more), but if you're interested in this direction, I think there's a lot of overlap.
(In reply to Richard Newman [:rnewman] from comment #5)
> …and with optional…

To expand on this: you can open an LMDB store with MDB_NOSYNC and assorted other flags that cause it to fsync very rarely, if at all, so if you don't care about durability you can use it as an in-memory store.
(In reply to Mike Hommey [:glandium] from comment #4)
> The tricky part with shared memory is having data structures that don't rely
> on pointers, because each process is likely to have the shared memory at a
> different address.

I'm only talking about the passing of the prefs to content processes -- i.e. replacing -intPrefs/-boolPrefs/-stringPrefs with a short-lived shared memory segment (and also passing all changed prefs, instead of early prefs.) So it would just be strings.
(In reply to Richard Newman [:rnewman] from comment #5)
> 
> I'm working on a proposal to use LMDB in Firefox. One way of thinking about
> LMDB, which is a multiprocess-safe mmap-backed store, is as a nice
> abstraction over shared memory, with isolation provided via coordination
> primitives, and with optional persistence for free.

In the long-term this could be a good solution for prefs storage. (Though libpref has enough quirks that dropping in a new storage system could be harder than it first appears.)

But it sounds like it's some distance off. In the meantime, removing this early/late split will help with various other changes to libpref that don't directly involve the in-memory representation of prefs.
(In reply to Nicholas Nethercote [:njn] from comment #8)

> But it sounds like it's some distance off.

We expect the design review to be completed this month. I don't have a good sense of implementation timeframe yet.


> In the meantime, removing this
> early/late split will help with various other changes to libpref that don't
> directly involve the in-memory representation of prefs.

Yepyep.
Depends on: 1436863
Depends on: 1373157
Blocks: 1436655
Depends on: 1438678
No longer depends on: 1373157
Blocks: 1419432
Blocks: 1432979
Blocks: 1444275
Comment on attachment 8958334 [details]
Bug 1436911 - Avoid the early/late prefs split.

https://reviewboard.mozilla.org/r/227264/#review234942

::: modules/libpref/Preferences.cpp:829
(Diff revision 1)
> +  // Examples of unlocked string prefs:
> +  // - "S-:10/my.string1:3/abc:4/wxyz\n"
> +  // - "S-:10/my.string2:5/1.234:\n"
> +  // - "S-:10/my.string3::7/string!\n"
> +
> +  void SerializeAndAppend(nsCString& aStr)

SerializeAndAppendTo, maybe?

::: modules/libpref/Preferences.cpp:845
(Diff revision 1)
> +      case PrefType::String: {
> +        aStr.Append('S');
> +        break;
> +      }
> +
> +      case PrefType::None:

should this return instead of falling through to crashing?
Attachment #8958334 - Flags: review?(mh+mozilla) → review+
> ::: modules/libpref/Preferences.cpp:845
> (Diff revision 1)
> > +      case PrefType::String: {
> > +        aStr.Append('S');
> > +        break;
> > +      }
> > +
> > +      case PrefType::None:
> 
> should this return instead of falling through to crashing?

Crashing is the right thing. It should be impossible to have a live pref whose type is None, so we should never need to serialize PrefType::None.
Assignee: nobody → n.nethercote
https://hg.mozilla.org/mozilla-central/rev/6cffa8738ca5
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1447604
You need to log in before you can comment on or make changes to this bug.