1416622 - Tighten up libpref's process checking

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

libpref's "parent process only" checking is a bit inconsistent at the moment.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1416622 - Tighten up libpref's process checking.

libpref only allows pref modifications in the parent process. This patch
tightens up the checking, which is a bit inconsistent.

- It removes ENSURE_MAIN_PROCESS_WITH_WARNING, which does NS_WARNING on
  failure, and replaces its uses with ENSURE_MAIN_PROCESS, which does NS_ERROR
  on failure. This required adding an XRE_IsParentProcess() check to one place
  in editor/.

- It converts XRE_IsContentProcess() tests to !XRE_IsParentProcess(), because
  we now have multiple kinds of non-parent process.

- It uses ENSURE_MAIN_PROCESS to replace other checking code in a few places.

- It improves a comment in HandleDirty().

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

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8927705 [details]
Bug 1416622 - Tighten up libpref's process checking.

https://reviewboard.mozilla.org/r/198980/#review204410

::: modules/libpref/Preferences.cpp:3079
(Diff revision 1)
> -    // TODO: this should really assert because you can't set prefs in a
> -    // content process. But so much code currently does this that we just
> -    // ignore it for now.
> +    // This path is hit a lot when setting up prefs for content processes. Just
> +    // ignore it in that case, because content processes aren't responsible for
> +    // saving prefs.

It seems the todo item should be kept in some form (it's still a worthwhile goal to assert here)

Comment 3

[Nicholas Nethercote [inactive]]

> It seems the todo item should be kept in some form (it's still a worthwhile
> goal to assert here)

I deliberately removed it because I thought it wasn't a worthwhile goal -- this function is low-level enough that I can't see a better way to handle things than with this check...

Comment 4

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3328527cb7d5a4fc3df2abe1a613e13faaad364f
Bug 1416622 - Tighten up libpref's process checking. r=glandium

Comment 5

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3328527cb7d5