1436655 - Introduce a mechanism for prefs to be defined entirely in the binary

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

I think this will improve things greatly.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1436655 - Rename pref_SetPref()'s aFromFile argument as aFromInit.

Currently all pref initialization is done from file, but soon we will also be
initializing prefs from code compiled into the binary. The new name encompasses
both cases.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

[XXX: The draft patch implements the mechanism and converts a subset of
representative prefs to the new mechanism. For landing I will have one patch
that implements the mechanism, multiple patches converting prefs, and then a
final clean-up patch that removes stuff no longer needed.]

Currently VarCache prefs are setup in two parts:

- The vanilla pref part, installed via a data file such as all.js, or via an
  API call.

- The VarCache variable part, setup by an Add*VarCache() call.

Both parts are needed for the pref to actually operate as a proper VarCache
pref. (There are various prefs for which we do one but not the other unless a
certain condition is met.)

This patch introduces a new way of doing things. There is a new file,
modules/libpref/init/All.h, which defines prefs like this:

> PRF("layout.accessiblecaret.width",
>     layout__accessiblecaret__width,
>     float, "34.0");

This replaces both the existing parts.

The preprocessor is used to generate multiple things from this single
definition:

- A global variable (the VarCache itself).

- A getter for that global variable.

- A call to a registration function that unconditionally installs the pref in
  the prefs hash table at startup.

C++ files can include the new StaticPrefs.h file to access the getter.

Rust code cannot use the getter, but can access the global variable directly
via structs.rs. This is similar to how things currently work for Rust code.

The new approach has the following advantages.

+ It eliminates the duplication (in all.js and the Add*VarCache() call) of the
  pref name and default value, preventing potential mismatches. (This is a real
  problem in practice!)

+ There is now a single initialization point for these VarCache prefs.
  + This avoids need to find a place to insert the Add*VarCache() calls, which
    are currently spread all over the place.
  + It also eliminates the common pattern whereby these calls are wrapped in a
    execute-once block protected by a static boolean (see bug 1346224).
  + And it's no longer possible to have a VarCache pref for which only one of
    the pieces has been setup.

+ It encapsulates the VarCache global variable, so there is no need to declare
  them separately.

+ VarCache reads are done via a getter (e.g. StaticPrefs::foo__bar__baz())
  instead of a raw global variable read.
  + This makes it clearer that you're reading a pref value.
  + This provides a single chokepoint in the code for such accesses, which make
    adding checking and instrumentation feasible.

+ It completely replaces MediaPrefs.h.

+ [XXX: not yet prototyped] It completely replaces gfxPrefs.h.

+ Once all VarCache prefs are migrated to the new approach, the VarCache
  mechanism will be better encapsulated, with fewer details publicly visible.

+ (Future work) This will allow the pref names to be stored statically, saving
  memory in every process.

The main downside of the new approach is that all of these prefs are in a
single header that is included in quite a few places, so any changes to this
header will cause a fair amount of recompilation.

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

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

I'm casting a wide feedback net here because it's a big change! Thanks.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

https://reviewboard.mozilla.org/r/218672/#review224610

This seems like a step in the right direction!

::: modules/libpref/init/All.h:10
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This file defines VarCache prefs.
> +//
> +// Prefs defined in this file should *not* be listed in a prefs data file such
> +// as all.js.

What happens if they are? Can we check for these named varcache prefs and err or warn appropriately if they're present when reading a prefs file?

::: modules/libpref/init/All.h:22
(Diff revision 1)
> +// functions.
> +//
> +// - A direct global variable access is faster than a hash table lookup.
> +//
> +// - A global variable can be accessed off the main thread. If a pref *is*
> +//   accessed off the main thread, it should use an atomic type. (But note that

Can we add a debugging thread check to the generated accessor (excepting `Atomic<float>`, of course)?

::: modules/libpref/init/All.h:24
(Diff revision 1)
> +// - A direct global variable access is faster than a hash table lookup.
> +//
> +// - A global variable can be accessed off the main thread. If a pref *is*
> +//   accessed off the main thread, it should use an atomic type. (But note that
> +//   many VarCaches that should be atomic are not, in particular because
> +//   Atomic<float> is not available, alas.)

Does that mean we currently do cross-thread reads that can return garbage values? Can you file a bug if necessary, and point to it from here?

::: modules/libpref/init/All.h:35
(Diff revision 1)
> +//       <c++-type>, <default-value>)
> +//
> +// - The macro name is PRF because then subsequent lines can be indented by 4
> +//   spaces, which is nice.
> +//
> +// - <pref-name-string> is the name of the pref, as it appears in about:config,

Are there any C++ users of the name?

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

(ReviewBoard)

Comment 6

[Nicholas Nethercote [inactive]]

Thank you for the comments.

> > +// Prefs defined in this file should *not* be listed in a prefs data file such
> > +// as all.js.
> 
> What happens if they are? Can we check for these named varcache prefs and
> err or warn appropriately if they're present when reading a prefs file?

Currently I think the value from the file will be installed normally, overwriting the value obtained from the VarCache installation. It would be possible to add a check to detect this case.


> > +// - A global variable can be accessed off the main thread. If a pref *is*
> > +//   accessed off the main thread, it should use an atomic type. (But note that
> 
> Can we add a debugging thread check to the generated accessor

I think so. The exception is for VarCache variables accessed via Rust code, because they do raw variable accesses instead of using the C++ getter.


> (excepting `Atomic<float>`, of course)?

(BTW, in the same way that floats are actually stored as strings in the hash table and then converted at use points, I'm wondering if float VarCache values could actually be stored as integers, in order to get atomicity.)


> > +// - A global variable can be accessed off the main thread. If a pref *is*
> > +//   accessed off the main thread, it should use an atomic type. (But note that
> > +//   many VarCaches that should be atomic are not, in particular because
> > +//   Atomic<float> is not available, alas.)
> 
> Does that mean we currently do cross-thread reads that can return garbage
> values? Can you file a bug if necessary, and point to it from here?

Currently: the main thread writes the global variables, and other threads can read them. If other threads do read them and the global variable is non-atomic, then the consequences are... unpredictable. Possibly garbage.

I filed bug 1436916 for this.


> > +// - <pref-name-string> is the name of the pref, as it appears in about:config,
> 
> Are there any C++ users of the name?

The next line in the comment says "and as is used in most libpref API functions." I will reword the comment to indicate this covers both C++ and JS API functions.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

https://reviewboard.mozilla.org/r/218672/#review225358

::: modules/libpref/StaticPrefs.h:53
(Diff revision 1)
> +private:                                                                       \
> +  static cpp_type sVarCache_##id;                                              \
> +                                                                               \
> +public:                                                                        \
> +  static StripAtomic<cpp_type> id() { return sVarCache_##id; }
> +#include "mozilla/All.h"

Nit: can we call this something else other than `All.h`?

::: modules/libpref/init/All.h:67
(Diff revision 1)
> +PRF("layout.accessiblecaret.width",
> +    layout__accessiblecaret__width,

Are there any plans to auto-generate the C++-side names?  I can imagine that people will complain about the naming algorithm and start using some more palatable in some cases, which makes things less understandable for everybody.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

This seems reasonable.

Comment 9

[Nicholas Nethercote [inactive]]

> > +#include "mozilla/All.h"
> 
> Nit: can we call this something else other than `All.h`?

Sure. Any suggestions? Prefs.h?


> > +PRF("layout.accessiblecaret.width",
> > +    layout__accessiblecaret__width,
> 
> Are there any plans to auto-generate the C++-side names?

AFAICT that would require switching to using a script to do the code generation instead of the C preprocessor, which I was hoping to avoid because it's a whole extra step. Unless you can think of another way?

Comment 10

[Richard Newman [:rnewman]]

(In reply to Nathan Froyd [:froydnj] from comment #7)

> Are there any plans to auto-generate the C++-side names?

I also thought this would be nice, but didn't mention it in my comment for two reasons:

- I didn't think it was possible without extra work, and
- The explicit way keeps the names in the tree, right in the definition, which will help searchers.

To address the second, we'd need a macro at point of use, too. E.g., Rust:

   let foo = pref!(layout.foo.bar);

which would expand to

   let foo = StaticPrefs::layout__foo__bar();

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8949289 [details]
Bug 1436655 - Rename pref_SetPref()'s aFromFile argument as aFromInit.

https://reviewboard.mozilla.org/r/218670/#review225508

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

https://reviewboard.mozilla.org/r/218672/#review226312

::: commit-message-de2a3:23
(Diff revision 1)
> +
> +This patch introduces a new way of doing things. There is a new file,
> +modules/libpref/init/All.h, which defines prefs like this:
> +
> +> PRF("layout.accessiblecaret.width",
> +>     layout__accessiblecaret__width,

Please do not use double underscores. They are always reserved for compilers.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

https://reviewboard.mozilla.org/r/218672/#review226298

Some notes inline, mainly I think we should go ahead and add a script for generating the code rather than just using macros. f=me

::: caps/nsScriptSecurityManager.cpp:677
(Diff revision 1)
>          // A null principal can target its own URI.
>          if (sourceURI == aTargetURI) {
>              return NS_OK;
>          }
>      }
> -    else if (sViewSourceReachableFromInner &&
> +    else if (StaticPrefs::security__view_source__reachable_from_inner_protocol() &&

The ergonomics of this aren't great, it's more verbose than before (the need to specify the full path of the pref is unfortunate) and the user can't define the var name as they've been used to.

I'm not saying you shouldn't do this, but maybe ping dev-platform for alternative ideas? Or maybe this is as good as it gets.

I *do* think it's nice that we're preventing direct access to var so you can't accidentally modify it and of course avoiding a lot of boilerplate is nice as well.

::: modules/libpref/Preferences.cpp:4708
(Diff revision 1)
> +
> +  Preferences::AddUintVarCache(aCache, aName, aDefaultValue, true);
> +}
> +
> +static void
> +RegisterStaticPref(const char* aName, float* aCache, const char* aDefaultValue)

Seems like `aDefaultValue` shouldn't need to be a string. Either we could pass in just the float value and stringify via AppendFloat or we could pass in the float value *and* a macro-fu stringified value.

::: modules/libpref/StaticPrefs.h:49
(Diff revision 1)
> +// njn: could assert in the getter that it's been initialized...
> +// [but this would require moving it back into the .cpp file to access the
> +// isInitialized data...]
> +#define PRF(str, id, cpp_type, value)                                          \
> +private:                                                                       \
> +  static cpp_type sVarCache_##id;                                              \

One argument for generating this with a script is that we could attempt to order the prefs so that the struct packs well.

We should also be able to initialize the values right?

::: modules/libpref/init/All.h:44
(Diff revision 1)
> +//   the StaticPrefs class. For consistency, the identifier for every pref
> +//   should be created like so:
> +//   - start with <pref-name-string>;
> +//   - convert any '.' chars to '__';
> +//   - convert any '-' chars to '_'.
> +//   For example, "foo.bar_baz" becomes |foo__bar_baz|. Ugly, but clear!

The ergonomics of this are rough, but I don't have a better solution. I agree with previous comments that this adding a transform step rather than just macros would help with folks not adhearing to the rules.

Comment 14

[Nicholas Nethercote [inactive]]

> > +static void
> > +RegisterStaticPref(const char* aName, float* aCache, const char* aDefaultValue)
> 
> Seems like `aDefaultValue` shouldn't need to be a string.

True. I'm planning to extend this scheme to non-VarCache prefs as well, and there's a danger case where a person may specify an integer literal for a pref that's meant to be a float and the system could install it as an int pref instead of a float-as-string pref, and then lookups won't work properly. But I think I can defend against that case if I'm careful.

> We should also be able to initialize the values right?

An argument against that is that if they're all initially zero then they should end up in the .bss section, which doesn't take up any disk space.

Comment 15

[Nicholas Nethercote [inactive]]

> Please do not use double underscores. They are always reserved for compilers.

Good point! Thank you. I'll switch to using single underscores to replace both '.' and '-'. It's a lossy conversion (i.e. you can't get the original string back from the identifer) but I think it's good enough.

Comment 16

[Nicholas Nethercote [inactive]]

> I *do* think it's nice that we're preventing direct access to var so you
> can't accidentally modify it

Yes -- see bug 1438433 where I am fixing exactly such a modification :)

Comment 17

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> An argument against that is that if they're all initially zero then they
> should end up in the .bss section, which doesn't take up any disk space.

But in that case the values are still inline in the code. They still take space. Sometimes even more.

#include <stdlib.h>

size_t foo;
char* bar;

__attribute__((constructor))
void init() {
    foo = 1;
    bar = "bar";
}


00000000000004e0 <init>:
 4e0:	48 8d 05 fe 00 00 00 	lea    0xfe(%rip),%rax        # 5e5 <_fini+0x9>
 4e7:	48 c7 05 3e 0b 20 00 	movq   $0x1,0x200b3e(%rip)        # 201030 <foo>
 4ee:	01 00 00 00 
 4f2:	48 89 05 2f 0b 20 00 	mov    %rax,0x200b2f(%rip)        # 201028 <bar>
 4f9:	c3                   	retq   
 4fa:	66 0f 1f 44 00 00    	nopw   0x0(%rax,%rax,1)


First instruction (7 bytes) calculates the pointer to "bar", and the third (7) stores it in bar. That's 14 bytes for a string literal.
Second instruction (11 bytes) puts 1 in foo.

That's 25 bytes of code, 4 bytes for the literal string, and 16 bytes of .bss.

Compare with:
#include <stdlib.h>

size_t foo = 1;
char* bar = "bar";

No code, still 4 bytes for the literal string, and 16 bytes of .data.

... but there's also one relocation to fill the pointer that didn't exist before, and on Linux, those are not cheap in space (24 bytes on 64-bits iirc ; except with elfhack, if they're next to other relocated data).

So filling strings with a function can win in space, but not integers.

Comment 18

[Mike Hommey [:glandium]]

well, except if you do char bar[] = "bar", because then you don't get a relocation at all.

Comment 19

[Mike Hommey [:glandium]]

(except you can't reassign to that, obviously)

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

Looks great to me too!

The one thing I disagree is about defining the default value here, instead of the existing prefs files. You say that it was to consolidate these in one place instead of two, but I believe it's the other way around: now there's one extra place to look for prefs when trying to find where a specific pref is defined, and what is its default value

I believe this feature should only be about mapping (pref name) -> (variable), without including its value.

Which brings the question: what happens _if_ the pref is also defined in all.js or firefox.js?

  - If all.js and firefox.js is able to correctly override the value, then in
    reality you don't really need to have it defined here too

  - If they are not able to override it, then this will break the standard
    behavior and hierarchy of pref definitions, and it's guaranteed that this
    will lead to bugs. E.g. when someone does a last-minute pref flip in
    firefox.js (to disable a feature that is not ready to ship), but that
    definition doesn't take effect because it's defined here.

(just my 2 cents)

Comment 21

[Nicholas Nethercote [inactive]]

>  - If all.js and firefox.js is able to correctly override the value, then in
>    reality you don't really need to have it defined here too

From comment 6:

> > > +// Prefs defined in this file should *not* be listed in a prefs data file such
> > > +// as all.js.
> > 
> > What happens if they are? Can we check for these named varcache prefs and
> > err or warn appropriately if they're present when reading a prefs file?
> 
> Currently I think the value from the file will be installed normally,
> overwriting the value obtained from the VarCache installation. It would be
> possible to add a check to detect this case.

One overarching goal is to define each pref in a single place. For C++ prefs (esp. VarCache ones) that will be in the new C++ file (All.h, or whatever name we end up using). It may be useful to allow people to override those default values via all.js -- it can saves a lot of recompiling when experimenting with different values. But that should be an exceptional case, perhaps one that triggers a warning that would be detected on automation.

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: Bug 1436655 - Tweak the comment at the top of init/all.js.

- The first two SYNTAX HINTS are wrong, and citing the syntax is more useful
  than specifying a single example of what isn't allowed.

- The sentence about #ifdefs is wrong. (#ifdefs appear all throughout, and
  prefs are only specified once.)

- I chose a better example file.

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

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: Bug 1436655 - Tweak assertions in Add*VarCache() functions.

This avoids some repetition.

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

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8949289 [details]
Bug 1436655 - Rename pref_SetPref()'s aFromFile argument as aFromInit.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218670/diff/1-2/

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218672/diff/1-2/

Comment 26

[Nicholas Nethercote [inactive]]

Some notable changes from the old version, largely in response to comments:

- init/All.h has been renamed init/StaticPrefList.h.

- No use of double underscores in static pref identifiers.

- It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file such
  as all.js.

- Float pref default values are now specified as floats in StaticPrefList.h,
  rather than strings.

- All VarCache variables for static prefs are now initialized in the binary,
  rather than at registration time.

- It still uses the preprocessor rather than code generation. It's simpler, and
  nsCSSPropList.h is precedent for macros of this style that require both a
  string form and an identifier form :)

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

There are several concerns about this approach:
1. we would not be able to access prefs from Rust code like what we currently do in stylo (via bindings of static member fields generated by bindgen), which may lead to regressed performance and code scalability
2. any change to that list would trigger a rebuild on a much wider range than current

Comment 28

[Nicholas Nethercote [inactive]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #27)
> There are several concerns about this approach:
> 1. we would not be able to access prefs from Rust code like what we
> currently do in stylo (via bindings of static member fields generated by
> bindgen), which may lead to regressed performance and code scalability

Pref access from Rust code will be very similar to what we already have. See bug 1448225.

> 2. any change to that list would trigger a rebuild on a much wider range
> than current

Yes. That's the main downside of the patch, as described in the commit message. I think it's a worthwhile trade-off given the large number of benefits.

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8961633 [details]
Bug 1436655 - Tweak the comment at the top of init/all.js.

https://reviewboard.mozilla.org/r/230502/#review236850

::: modules/libpref/init/all.js:8
(Diff revision 1)
> - * for example xpfe/bootstrap/browser-prefs.js
> + * for example browser/defaults/preferences/firefox.js.
>   *
> - * Platform-specific #ifdefs at the end of this file override the generic
> - * entries at the top.
> + * For the syntax used by this file, consult the comments at the top of
> + * modules/libpref/parser/src/lib.rs.

nit: one of those paths is a path in the build, and the other in the source tree.

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 8961634 [details]
Bug 1436655 - Tweak assertions in Add*VarCache() functions.

https://reviewboard.mozilla.org/r/230504/#review236852

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8949290 [details]
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary.

https://reviewboard.mozilla.org/r/218672/#review236860

Comment 32

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c34cc8156a71b1feab6cf4351b80ac60cd12cc78
Bug 1436655 - Rename pref_SetPref()'s aFromFile argument as aFromInit. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/8509ebf8fbb3af7e580c85aa73d5bb4a9b78c61a
Bug 1436655 - Tweak the comment at the top of init/all.js. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/08af6778dd6a669e896d61ef7599a75d7024d5fd
Bug 1436655 - Tweak assertions in Add*VarCache() functions. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/16ac5bd4e50c50a62316828b586c5e7a60b28e99
Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. r=glandium

Comment 33

[:Felipe Gomes (needinfo for replies!)]

(In reply to Nicholas Nethercote [:njn] from comment #26)
> - It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file
> such
>   as all.js.

Can we make a test that will fail when this situation happens?


Two nits I noticed when reading the documentation comments:

> +// Prefs defined in this file should *not* be listed in a prefs data file such
> +// as all.js. If they are, CheckForExistence() will issue an NS_ERROR.

s/CheckForExistence/CheckForDoubleDefinition/

> +// ... Note that float prefs are
> +//   stored internally as floats.

as strings

Comment 34

[Nicholas Nethercote [inactive]]

> > - It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file
> > such
> >   as all.js.
> 
> Can we make a test that will fail when this situation happens?

There's no need -- if it happens the presence of the NS_ERROR will cause various tests to fail. (I did this by accident with one try push while developing the patch!)

> Two nits I noticed when reading the documentation comments:

Thanks, I will fix.

Comment 35

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dabcac07c6835679aaeb585b3d991f5a805a3ffb
Bug 1436655 - Fix up two minor mistakes in comments. r=me

Comment 36

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c34cc8156a71
https://hg.mozilla.org/mozilla-central/rev/8509ebf8fbb3
https://hg.mozilla.org/mozilla-central/rev/08af6778dd6a
https://hg.mozilla.org/mozilla-central/rev/16ac5bd4e50c
https://hg.mozilla.org/mozilla-central/rev/dabcac07c683

Comment 37

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #34)
> > > - It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file
> > > such
> > >   as all.js.
> > 
> > Can we make a test that will fail when this situation happens?
> 
> There's no need -- if it happens the presence of the NS_ERROR will cause
> various tests to fail. (I did this by accident with one try push while
> developing the patch!)

Unfortunately, this caused bustage for Thunderbird. The prefs network.auth.subresource-img-cross-origin-http-auth-allow and network.auth.non-web-content-triggered-resources-http-auth-allow are now defined in StaticPrefList.h, but Thunderbird overrides their values in all-thunderbird.js. (See bug 1449143.)

The checking fundamentally isn't compatible with Thunderbird operating in this way. I think I'm just going to have to disable the checking :(

Comment 38

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #37)
> Unfortunately, this caused bustage for Thunderbird. The prefs
> network.auth.subresource-img-cross-origin-http-auth-allow and
> network.auth.non-web-content-triggered-resources-http-auth-allow are now
> defined in StaticPrefList.h, but Thunderbird overrides their values in
> all-thunderbird.js. (See bug 1449143.)
> 
> The checking fundamentally isn't compatible with Thunderbird operating in
> this way. I think I'm just going to have to disable the checking :(

Could we disable the check only if we're building Thunderbird?  Set a define in moz.build based on the value of MOZ_BUILD_APP or something?

Comment 39

[Nicholas Nethercote [inactive]]

> Could we disable the check only if we're building Thunderbird?  Set a define
> in moz.build based on the value of MOZ_BUILD_APP or something?

An "is this Firefox?" check would be safer than "is this Thunderbird?", because of Seamonkey and possibly other products.

Even then, it's possible that people who distribute Firefox (e.g. Linux packagers) would add their own .js files containing pref overrides. Maybe an NS_ERROR doesn't matter to them; I'm not sure.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Should we add a deprecation comment to Add*VarCache functions and/or post on .platform about this new way of doing things?

Comment 42

[Boris Zbarsky [:bzbarsky]]

And just to make sure I understand how this works, can we still ship pref overrides to the static prefs without shipping a new binary, via whatever mechanism we have for deploying pref flips on release if needed?

Comment 43

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #41)
> Should we add a deprecation comment to Add*VarCache functions and/or post on
> .platform about this new way of doing things?

Probably. I've been meaning to file a bunch of bugs about converting the existing Add*VarCache calls.

> And just to make sure I understand how this works, can we still ship pref
> overrides to the static prefs without shipping a new binary, via whatever
> mechanism we have for deploying pref flips on release if needed?

Yes. If a pref value is given in one of the data files (e.g. all.js) that will override the statically-provided value.(Thunderbird relies on this.)