Re: New prefs parser has landed

[Nicholas Nethercote]

On Sat, Feb 3, 2018 at 9:01 AM, Boris Zbarsky  wrote:

> On 2/2/18 4:57 PM, Nicholas Nethercote wrote:
>
>> It shouldn't be too hard, because the prefs grammar is very simple. I
>> would
>> just implement "panic mode" recovery, which scans for a synchronizing
>> token
>> like ';' or 'pref' and then continues from there.
>>
>
> OK.  I think that would address my concerns, for sure.
>

I just landed error recovery:
https://bugzilla.mozilla.org/show_bug.cgi?id=107264 (filed in 2001!)

And we had a real-world case where the new strictness (integer overflow
detection) was causing issues:
https://bugzilla.mozilla.org/show_bug.cgi?id=1438878

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[zbraniecki]

On Friday, February 2, 2018 at 1:57:34 PM UTC-8, Nicholas Nethercote wrote:
> It shouldn't be too hard, because the prefs grammar is very simple. I would
> just implement "panic mode" recovery, which scans for a synchronizing token
> like ';' or 'pref' and then continues from there. It's not foolproof but
> works well in many cases.

We do quite heavy error recover in the new l10n format. 

The way we handle it is that if we encounter an error while retrieving an 
entry, we collect it as an error[0], and skip to the start of the next one 
recognized by the first line that starts with an ID[1].

I assume the same would work for prefs (even easier, because the line has to 
start with `pref`).

zb.

[0] https://searchfox.org/mozilla-central/source/intl/l10n/MessageContext.jsm#63
[1] 
https://searchfox.org/mozilla-central/source/intl/l10n/MessageContext.jsm#957
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Boris Zbarsky]

On 2/2/18 4:57 PM, Nicholas Nethercote wrote:

It shouldn't be too hard, because the prefs grammar is very simple. I would
just implement "panic mode" recovery, which scans for a synchronizing token
like ';' or 'pref' and then continues from there.


OK.  I think that would address my concerns, for sure.

-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Nicholas Nethercote]

On Sat, Feb 3, 2018 at 12:42 AM, Boris Zbarsky  wrote:

>
> Perhaps I should implement error recovery, so that ill-formed prefs won't
>> cause subsequent prefs to be lost.
>>
>
> You mean pick up parsing again after hitting an error?   That sounds
> complicated...
>

It shouldn't be too hard, because the prefs grammar is very simple. I would
just implement "panic mode" recovery, which scans for a synchronizing token
like ';' or 'pref' and then continues from there. It's not foolproof but
works well in many cases.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[David Teller]

Pretty complicated in the general case but might be simple in the case
of number overflow.

Also, while we shouldn't depend on the UI in libpref, could we send some
kind of event or observer notification that the UI could use to display
a detailed error message? It would be a shame if Firefox was broken and
impossible-to-diagnose because of a number overflow, for instance.

On 02/02/2018 14:42, Boris Zbarsky wrote:
> You mean pick up parsing again after hitting an error?   That sounds
> complicated...
> 
> -Boris
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Boris Zbarsky]

On 2/1/18 10:27 PM, Nicholas Nethercote wrote:

On Fri, Feb 2, 2018 at 12:50 PM, Boris Zbarsky  wrote:

OK.  I assume we've double-checked that?  And that this was the case all
along, for prefs extensions or about:config might have set in the past?
(Especially the no-embedded-null thing.)



I don't know what you mean by "prefs extensions".


"Prefs extensions might have set".  If anything set via our pref APIs 
was fine, good.



Perhaps I should implement error recovery, so that ill-formed prefs won't
cause subsequent prefs to be lost.


You mean pick up parsing again after hitting an error?   That sounds 
complicated...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Nicholas Nethercote]

On Fri, Feb 2, 2018 at 2:27 PM, Nicholas Nethercote 
wrote:

> It might be possible that you could use about:config or the prefs API to
> set a string pref that contains an invalid escape sequence that the parser
> will subsequently reject. I will check that.
>

Thinking (and testing) some more: I don't think this is possible, because
we escape strings appropriately when writing them.

So I think the only way to get data loss is if a user hand-edits prefs.js
and introduces a syntax error, whereupon all subsequent prefs in the file
will be lost. (Which has always been true; the only thing that's changed is
what constitutes a syntax error.) Error recovery would eliminate that data
loss possibility.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Nicholas Nethercote]

On Fri, Feb 2, 2018 at 12:50 PM, Boris Zbarsky  wrote:

> On 2/1/18 6:40 PM, Nicholas Nethercote wrote:
>
>> - prefs.js, which is written by Firefox. Firefox should always generate
>> data that is accepted by the new parser
>>
>
> OK.  I assume we've double-checked that?  And that this was the case all
> along, for prefs extensions or about:config might have set in the past?
> (Especially the no-embedded-null thing.)
>

I don't know what you mean by "prefs extensions". It might be possible that
you could use about:config or the prefs API to set a string pref that
contains an invalid escape sequence that the parser will subsequently
reject. I will check that.


> We could make a backup copy of prefs.js if an error is discovered in it,
> so that when we write it out again the old thing can still be recovered if
> needed.


Perhaps I should implement error recovery, so that ill-formed prefs won't
cause subsequent prefs to be lost.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Nicholas Nethercote]

On Fri, Feb 2, 2018 at 11:32 AM, Xidorn Quan  wrote:

>
> One approach would probably be showing a warning UI to tell people that we
> fail to parse user.js and ask them to fix it.
>
> Not sure how complicated that approach would be.
>

This suggestion came up in one of the old bugs on this topic. One argument
against it was that libpref shouldn't depend on or be able to call UI code.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Nicholas Nethercote]

On Fri, Feb 2, 2018 at 10:32 AM, Justin Dolske  wrote:

>
> Can I presume that things which now trigger parsing issues are escaped or
> errored by the relevant prefs APIs? e.g. if a caller tries to set a pref
> name or value with an embedded NULL?
>

Only the parser has changed. The other APIs are the same.

If you try to use an embedded NUL via a C++ API function, it'll just be
treated like the string ends at that NUL. I'm not sure what will happen if
you use an embedded NUL via a JS API function, though it should be the same
behaviour as before.

NIck
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Boris Zbarsky]

On 2/1/18 6:40 PM, Nicholas Nethercote wrote:

- prefs.js, which is written by Firefox. Firefox should always generate
data that is accepted by the new parser


OK.  I assume we've double-checked that?  And that this was the case all 
along, for prefs extensions or about:config might have set in the past? 
(Especially the no-embedded-null thing.)



(If users do hand-edit, there might be problems. But that
file has a big "Do not edit this file" warning at the top, so I'm not too
worried about that case.)


People hand-edit somewhat commonly, because the interaction of user.js 
with the rest of the system is terrible...



Do you have suggestions on a better approach?


We could make a backup copy of prefs.js if an error is discovered in it, 
so that when we write it out again the old thing can still be recovered 
if needed.



Silently wrapping was also a footgun that came up in practice in bug
1424030 and bug 1434813. IMO it's better to give a clear error than to
silently do the wrong thing...


Modulo the dataloss issue, I agree.

-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Andrew Swan]

On Thu, Feb 1, 2018 at 4:32 PM, Xidorn Quan  wrote:

> What do we show when we disable legacy addons? We can probably borrow
> whatever we did there.
>

There is no explicit notification for disabled legacy addons (you can see
them in about:addons but you have to know to go look there)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Xidorn Quan]

On Fri, Feb 2, 2018, at 10:40 AM, Nicholas Nethercote wrote:
> [*] One tricky question is what to do with syntax errors. The current
> behaviour is here:
> https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#1000-1011.
> It writes the error to the browser console, but if that fails, prints it to
> stderr instead. And then it also does an NS_WARNING. This means that 
> errors are easy to overlook, especially in user.js. Do you have suggestions
> on a better approach? Do you think the parser should do error recovery?
> Bug 107624 has some context for this.

One approach would probably be showing a warning UI to tell people that we fail 
to parse user.js and ask them to fix it.

Not sure how complicated that approach would be. What do we show when we 
disable legacy addons? We can probably borrow whatever we did there.

- Xidorn
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Nicholas Nethercote]

On Fri, Feb 2, 2018 at 9:47 AM, Boris Zbarsky  wrote:

>
> What happens if a user's prefs file has things that were OK but now fail?
> Is it effectively dataloss in that we will not parse anything after that
> and then write out the modified pref file with all the later things missing?
>

There are two kinds of "user's prefs".

- prefs.js, which is written by Firefox. Firefox should always generate
data that is accepted by the new parser, so I don't think there is a
problem there. (If users do hand-edit, there might be problems. But that
file has a big "Do not edit this file" warning at the top, so I'm not too
worried about that case.)

- user.js, which is hand-written by users. If there is a syntax error in
this file the parser will issue an error message[*], abort, and any prefs
after the syntax error will be ignored, but Firefox will otherwise run
normally.

So I don't think data loss is a concern, but user.js prefs that were
previously accepted might now be ignored.

[*] One tricky question is what to do with syntax errors. The current
behaviour is here:
https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#1000-1011.
It writes the error to the browser console, but if that fails, prints it to
stderr instead. And then it also does an NS_WARNING. This means that errors
are easy to overlook, especially in user.js. Do you have suggestions on a
better approach? Do you think the parser should do error recovery? Bug
107624 has some context for this.

- The old parser allowed integer literals to overflow, silently wrapping
>> them.
>>
>>The new parser treats integer overflow as a parse error.
>>
>
> This seems like a footgun for users who set numeric prefs by hand...
>

Silently wrapping was also a footgun that came up in practice in bug
1424030 and bug 1434813. IMO it's better to give a clear error than to
silently do the wrong thing...

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Justin Dolske]

Wow. Some of that is bizarre enough that I kinda want to go look at the old
parser code, but I value my sanity enough to not do so. Nice work.

Can I presume that things which now trigger parsing issues are escaped or
errored by the relevant prefs APIs? e.g. if a caller tries to set a pref
name or value with an embedded NULL?

Justin

On Thu, Feb 1, 2018 at 1:04 PM, Nicholas Nethercote 
wrote:

> Hi,
>
> I just landed https://bugzilla.mozilla.org/show_bug.cgi?id=1423840, which
> replaces the old prefs parser with a new one.
>
> The new parser is faster, safer, gives better and more accurate error
> messages, and is *much* easier to maintain.
>
> It is also slightly stricter; see the list at the bottom of this email for
> details (copied from the commit message). This increased strictness
> required fixes in a few places (in Firefox, Talos, and Thunderbird) where
> existing prefs files were doing bogus things that the old parser accepted,
> such as junk characters between prefs, and integer literal values that
> silently overflowed because they didn't fit in 32 bits.
>
> Please keep an eye out for any other problems that might arise from this
> change.
>
> Nick
>
>
> The new parser is slightly stricter than the old parser in a few ways.
>
> - Disconcertingly, the old parser allowed arbitrary junk between prefs
>   (including at the start and end of the prefs file) so long as that junk
>   didn't include any of the following chars: '/', '#', 'u', 's', 'p'. I.e.
>   lines like these:
>
> !foo@bar("prefname", true);
> ticky_pref("prefname", true);   // missing 's' at start
> User_pref("prefname", true);// should be 'u' at start
>
>   would all be treated the same as this:
>
> pref("prefname", true);
>
>   The new parser disallows such junk because it isn't necessary and seems
> like
>   an unintentional botch by the old parser.
>
> - The old parser allowed character 0x1a (SUB) between tokens and treated it
>   like '\n'.
>
>   The new parser does not allow this character. SUB was used to indicate
>   end-of-file (*not* end-of-line) in some old operating systems such as
> MS-DOS,
>   but this doesn't seem necessary today.
>
> - The old parser tolerated (with a warning) invalid escape sequences within
>   string literals -- such as "\q" (not a valid escape) and "\x1" and "\u12"
>   (both of which have insufficient hex digits) -- accepting them literally.
>
>   The new parser does not tolerate invalid escape sequences because it
> doesn't
>   seem necessary and would complicate things.
>
> - The old parser tolerated character 0x00 (NUL) within string literals;
> this is
>   dangerous because C++ code that manipulates string values with embedded
> NULs
>   will almost certainly consider those chars as end-of-string markers.
>
>   The new parser treats NUL chars as end-of-file, to avoid this danger and
>   because it facilitates a significant optimization (described within the
>   code).
>
> - The old parser allowed integer literals to overflow, silently wrapping
> them.
>
>   The new parser treats integer overflow as a parse error. This seems
> better,
>   and it caught existing overflows of places.database.lastMaintenance, in
>   testing/profiles/prefs_general.js (bug 1424030) and
>   testing/talos/talos/config.py (bug 1434813).
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New prefs parser has landed

[Boris Zbarsky]

On 2/1/18 4:04 PM, Nicholas Nethercote wrote:

It is also slightly stricter; see the list at the bottom of this email for
details (copied from the commit message).


What happens if a user's prefs file has things that were OK but now 
fail?  Is it effectively dataloss in that we will not parse anything 
after that and then write out the modified pref file with all the later 
things missing?



- The old parser allowed integer literals to overflow, silently wrapping
them.

   The new parser treats integer overflow as a parse error.


This seems like a footgun for users who set numeric prefs by hand...

-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform