1436911 - Avoid the early/late prefs split

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

> 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.

Comment 2

[Nicholas Nethercote [inactive]]

> 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.

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Richard Newman [:rnewman]]

(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.

Comment 6

[Richard Newman [:rnewman]]

(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.

Comment 7

[Nicholas Nethercote [inactive]]

(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.

Comment 8

[Nicholas Nethercote [inactive]]

(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.

Comment 9

[Richard Newman [:rnewman]]

(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.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Bug 1436911 - Avoid the early/late prefs split.

All prefs that need to be sent to a new content process are now put into the
shared memory segment, and they are identified by the pref name instead of an
index into a list. The old IPC used at process startup (in XPCOMInitData) is
removed.

Benefits:

- It removes the need for the early prefs list
  (dom/ipc/ContentProcesses.{h,cpp}) and the associated checking, which is ugly
  and often trips people up (e.g. bug 1432979, bug 1439406).

- Using prefnames instead of indices fixes some fragility (fixing bug 1419432).

- It fixes the problem of early prefs being installed as unlocked default
  values even if they are locked and/or have user values.

Review commit: https://reviewboard.mozilla.org/r/227264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227264/

Comment 11

[Mike Hommey [:glandium]]

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?

Comment 12

[Nicholas Nethercote [inactive]]

> ::: 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.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cffa8738ca53d246a39536d18a0c19b8c602e37
Bug 1436911 - Avoid the early/late prefs split. r=glandium

Comment 14

[Stefan Hindli [:stefan_hindli]]

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