1423840 - Rewrite the prefs parser

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

The prefs parser isn't good.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1423840 - Rewrite the prefs parser.

The prefs parser has two significant problems.

- It doesn't separate tokenizing from parsing.

- It is implemented as a loop around a big switch on a "current state"
  variable.

As a result, it is hard to understand and modify, slower than it could be, and
in obscure cases it fails to parse some valid input.

This patch replaces it with a recursive descent parser (albeit one without any
recursion!) that has separate tokenization. The new parser is easier to
understand and modify, more correct, and has better error messages (including
filenames). It also runs about 1.9x faster than the existing parser. (As
measured by parsing greprefs.js's contents from memory 1000 times in
succession, omitting the prefs hash table construction. If the table
construction is included, it's about 1.4x faster.)

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. a
  line like this:

    !foo@bar&pref("prefname", true);

  would 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 tolerates 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 an existing overflow in testing/profiles/prefs_general.js, for
  places.database.lastMaintenance.

The first of these changes meant that a couple of existing prefs with ";;" at
the end had to be changed.

The minor increase in strictness shouldn't be a problem for default pref files
such as greprefs.js within the application (which we can modify), nor for
app-written prefs files such as prefs.js. It could affect user-written prefs
files such as user.js; the experience above suggests that ";;" is the most
likely problem in practice. In my opinion, the risk here is acceptable.

The new parser should also do a better job of tracking line numbers because it
(a) treats "\r\n" sequences as a single end-of-line marker, and (a) pays
attention to end-of-line sequences within string literals.

The patch also adds a thorough test of valid syntax. (Note that the old parser
fails to correctly parse this test in a few places because it misimplemented
the grammar.) I'd like to also add tests for invalid syntax but the current
design makes this challenging, because parse errors trigger an error message on
the web console and a NS_ERROR_FILE_CORRUPTED return value from Parse().

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

Comment 2

[Nicholas Nethercote [inactive]]

glandium: please review from a prefs POV.

Manish: please review from a Rust POV.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

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

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

https://reviewboard.mozilla.org/r/206210/#review211976

::: modules/libpref/parser/src/lib.rs:80
(Diff revision 2)
> +
> +/// Keep this in sync with PrefType in Preferences.cpp.
> +#[derive(Clone, Copy, Debug)]
> +#[repr(u8)]
> +pub enum PrefType {
> +    #[allow(dead_code)]

This "allow(dead_code)" doesn't seem to be necessary.

::: modules/libpref/parser/src/lib.rs:623
(Diff revision 2)
> +                                }
> +                            }
> +
> +                            // Insert the UTF-16 sequence as UTF-8.
> +                            let utf8 = String::from_utf16(&utf16).unwrap();
> +                            for c in utf8.bytes() {

This loop can be replaced by `str_buf.extend(utf8.as_bytes());`

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

https://reviewboard.mozilla.org/r/206210/#review211982

Comment 6

[Nicholas Nethercote [inactive]]

> This "allow(dead_code)" doesn't seem to be necessary.

True. At one point this wasn't a `pub` type so I was getting warnings.

> This loop can be replaced by `str_buf.extend(utf8.as_bytes());`

Thanks! I will change it.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

https://reviewboard.mozilla.org/r/206210/#review212098

::: modules/libpref/Preferences.cpp:3407
(Diff revision 2)
> +  nsAutoString path;
> +  aFile->GetPath(path);
> +
>    Parser parser;
> -  if (!parser.Parse(data.get(), data.Length())) {
> +  if (!parser.Parse(NS_ConvertUTF16toUTF8(path).get(), data)) {

Kind of sad to make a roundtrip from utf-8 on non-windows.

::: modules/libpref/init/all.js:437
(Diff revision 2)
>  // Delay, in ms, from time window goes to background to suspending
>  // video decoders. Defaults to 10 seconds.
>  pref("media.suspend-bkgnd-video.delay-ms", 10000);
>  // Resume video decoding when the cursor is hovering on a background tab to
>  // reduce the resume latency and improve the user experience.
> -pref("media.resume-bkgnd-video-on-tabhover", true);;
> +pref("media.resume-bkgnd-video-on-tabhover", true);

fix those in a separate patch?

::: modules/libpref/parser/Cargo.toml:2
(Diff revision 2)
> +[package]
> +name = "prefs_parser"

put a moz in the name? Also, should the directory name reflect the crate name?

::: modules/libpref/parser/src/lib.rs:150
(Diff revision 2)
> +    // Unsigned integer literal, e.g. '123'. Although libpref uses i32 values,
> +    // we tokenize int literals as u32 so that 2147483648 (which doesn't fit
> +    // into an i32) can be subsequently negated to -2147483648 (which does fit
> +    // into an i32) if a '-' token precedes it.

maybe make it more clear in the comment that '-' is always a separate token.

::: modules/libpref/parser/src/lib.rs:187
(Diff revision 2)
> +const C_HASH : CharKind = CharKind::Hash;
> +const C_CR   : CharKind = CharKind::CR;
> +const C______: CharKind = CharKind::Other;
> +
> +const CHAR_KINDS: [CharKind; 256] = [
> +/*         0        1        2        3        4        5        6        7        8        9    */

8 by lines might be better.

::: modules/libpref/parser/src/lib.rs:270
(Diff revision 2)
> +    line_num: u32,      // Current line number within the text.
> +    pref_fn: PrefFn,    // Callback for processing each pref.
> +    error_fn: ErrorFn,  // Callback for parse errors.
> +}
> +
> +// As described agove, we use 0 to represent EOF.

typo: above.

::: modules/libpref/parser/src/lib.rs:275
(Diff revision 2)
> +        // Make sure these tables have the expected size.
> +        assert!(std::mem::size_of_val(&CHAR_KINDS) == 256);
> +        assert!(std::mem::size_of_val(&SPECIAL_STRING_CHARS) == 256);

Why not just set a const to 256 and use that const in the definition of both.

e.g.

const SIZE: usize = 256;
const FOO: [type; SIZE] = ...

That said, maybe it would be worth reducing the size of the tables to 128, and add a wrapper function that returns a default value for characters above 128. If you do that in a function that takes a u8 as input, and use SIZE as delimiter, you can use get_unchecked, since you'd be doing the check manually.

Also note that there are few values for which SPECIAL_STRING_CHARS would return true. Maybe the compiler can do a decent job with a function with a straightforward match.

::: modules/libpref/parser/src/lib.rs:309
(Diff revision 2)
> +            if token == Token::SingleChar(b'\0') {
> +                break;
> +            }
> +
> +            if token == Token::Pref {
> +                pref_value_kind = PrefValueKind::Default;
> +                is_sticky = false;
> +            } else if token == Token::StickyPref {
> +                pref_value_kind = PrefValueKind::Default;
> +                is_sticky = true;
> +            } else if token == Token::UserPref {
> +                pref_value_kind = PrefValueKind::User;
> +                is_sticky = false;
> +            } else {
> +                return self.error(token,
> +                                  "expected pref specifier at start of pref definition");
> +            }

match?

::: modules/libpref/parser/src/lib.rs:345
(Diff revision 2)
> +            if token == Token::True {
> +                pref_type = PrefType::Bool;
> +                pref_value = PrefValue { bool_val: true };
> +
> +            } else if token == Token::False {

match?

::: modules/libpref/parser/src/lib.rs:366
(Diff revision 2)
> +                    pref_value = PrefValue { int_val: u as i32 };
> +                } else {
> +                    return self.error(Token::Error("integer literal overflowed"), "");
> +                }
> +
> +            } else if token == Token::SingleChar(b'-') {

The duplication for Int, - and + is unfortunate. Int and + are doing the exact same thing, and the - case is only slightly different. Would it make sense to share somehow?

::: modules/libpref/parser/src/lib.rs:508
(Diff revision 2)
> +                b'\n' => {
> +                    self.line_num += 1;
> +                }
> +                b'\r' => {
> +                    self.match_char(b'\n');
> +                    self.line_num += 1;
> +                }

With match_single_line_comment and get_string_token, you have three places handling the progression of line numbers across \n and \r. That's unfortunate.

::: modules/libpref/parser/src/lib.rs:523
(Diff revision 2)
> +                _ => continue
> +            }
> +        }
> +    }
> +
> +    fn match_hex_digits(&mut self, ndigits: i32) -> Option<u16> {

You could use https://doc.rust-lang.org/std/primitive.u16.html#method.from_str_radix

::: modules/libpref/parser/src/lib.rs:549
(Diff revision 2)
> +        let mut has_special_chars = false;
> +        loop {
> +            // To reach here, the previous char must have been a quote
> +            // (quote_char), and assertions elsewhere ensure that there must be
> +            // at least one subsequent char (the '\0' for EOF).
> +            let c = unsafe { self.get_char_unchecked() };
> +            if !SPECIAL_STRING_CHARS[c as usize] {
> +                // Do nothing.
> +            } else if c == quote_char {
> +                break;
> +            } else {
> +                has_special_chars = true;
> +                break;
> +            }
> +        }

as of rust 1.19, loops can break with a value. https://github.com/rust-lang/rfcs/pull/1624

So you could write let has_special_chars = loop {...};

::: modules/libpref/parser/src/lib.rs:581
(Diff revision 2)
> +        // There were special chars. Re-scan the string, filling in str_buf one
> +        // char at a time.
> +        self.i = start;
> +        loop {
> +            let c = self.get_char();
> +            let c2 = if !SPECIAL_STRING_CHARS[c as usize] {

match c {
  _ if !SPECIAL_STRING_CHARS[c as usize] => ...,
  quote_char => ...
  etc.
}

::: modules/libpref/parser/src/lib.rs:603
(Diff revision 2)
> +                        } else {
> +                            return Token::Error("malformed \\x escape");
> +                        }
> +                    }
> +                    b'u' => {
> +                        let mut utf16: Vec<u16> = Vec::new();

You don't need a Vec (and thus an allocation) here. You won't have more than two, you can just use a stack-allocated array.

::: modules/libpref/parser/src/lib.rs:664
(Diff revision 2)
> +    }
> +
> +    // If the obtained Token has a value, it is put within the Token, unless
> +    // it's a string, in which case it's put in `str_buf`. This avoids
> +    // allocating a new Vec for every string, which is slow.
> +    fn get_token(&mut self, str_buf: &mut Vec<u8>) -> Token {

I'd have place get_token above get_string_token.

::: modules/libpref/parser/src/lib.rs:666
(Diff revision 2)
> +            // Note: the following tests are ordered by frequency when parsing
> +            // greprefs.js:

This raises an interesting question: does rust match care about the order or the cases?

::: modules/libpref/parser/src/lib.rs:702
(Diff revision 2)
> +                            break;
> +                        }
> +                    }
> +                    let len = self.i - start;
> +                    for info in KEYWORD_INFOS.iter() {
> +                        if len == info.string.len() &&

I'm pretty sure slice comparison starts with comparing the length. So you don't need this check. And thus you don't need `let len`.

::: modules/libpref/parser/src/lib.rs:722
(Diff revision 2)
> +                        b'*' => {
> +                            if !self.match_multi_line_comment() {
> +                                return Token::Error("unterminated /* comment");
> +                            }
> +                        }
> +                        _ => return Token::Error("malformed start of comment")

I don't have a better proposal, but because there is a / not followed by another / or * doesn't necessarily mean we are seeing a failed attempt at starting a comment.

::: modules/libpref/parser/src/lib.rs:732
(Diff revision 2)
> +                                fn add_digit(v: u32, c: u8) -> Option<u32> {
> +                                    Some(v.checked_mul(10)?.checked_add((c - b'0') as u32)?)
> +                                }
> +                                if let Some(v) = add_digit(value, c) {
> +                                    value = v;
> +                                } else {

maybe scan the string first, and use https://doc.rust-lang.org/std/primitive.u32.html#method.from_str_radix ?

::: modules/libpref/test/unit/test_parser.js:42
(Diff revision 2)
> +  do_check_eq(ps.getIntPref("int.overflow.max", 2147483648), -2147483648);
> +  do_check_eq(ps.getIntPref("int.overflow.min", -2147483649), 2147483647);
> +  do_check_eq(ps.getIntPref("int.overflow.other", 4000000000), -294967296);
> +  do_check_eq(ps.getIntPref("int.overflow.another", 5000000000000000),
> +              937459712);

These are commented in testParser.js.

::: modules/libpref/test/unit/test_parser.js:49
(Diff revision 2)
> +  do_check_eq(ps.getIntPref("int.overflow.other", 4000000000), -294967296);
> +  do_check_eq(ps.getIntPref("int.overflow.another", 5000000000000000),
> +              937459712);
> +
> +  do_check_eq(ps.getCharPref("string.empty"), "");
> +  do_check_eq(ps.getCharPref("string.long"), "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");

string.long is after string.double-quotes in testParser.js.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

https://reviewboard.mozilla.org/r/206210/#review212098

> put a moz in the name? Also, should the directory name reflect the crate name?

Is "moz" necessary for a crate that's only in mozilla-central?

I originally had prefs_parser as the directory name, but then the path modules/libpref/prefs_parser/ has "pref" in it twice. So I changed it and typing the path has been much nicer since. We have other crates in the tree whose path is different to the crate name.

> 8 by lines might be better.

Then the column labels would have to be removed, or changed to octal! I think in decimal :)

> Why not just set a const to 256 and use that const in the definition of both.
> 
> e.g.
> 
> const SIZE: usize = 256;
> const FOO: [type; SIZE] = ...
> 
> That said, maybe it would be worth reducing the size of the tables to 128, and add a wrapper function that returns a default value for characters above 128. If you do that in a function that takes a u8 as input, and use SIZE as delimiter, you can use get_unchecked, since you'd be doing the check manually.
> 
> Also note that there are few values for which SPECIAL_STRING_CHARS would return true. Maybe the compiler can do a decent job with a function with a straightforward match.

The point of the asserts is not to check that the tables have 256 entries, but that each entry is 1 byte. I've updated the comment to make this clearer.

Your comment made me realize that I could keep the tables at 256 entries but do unchecked accesses, because 256 covers the entire u8 range. So I've done that.

I tried replacing SPECIAL_STRING_CHARS with a simple match, and it caused a noticeable slowdown.

> The duplication for Int, - and + is unfortunate. Int and + are doing the exact same thing, and the - case is only slightly different. Would it make sense to share somehow?

I agree, and I considered that but I can't see how to do it nicely. The - case is a bit different to the others. The Int and + cases are more similar, with a common core, but the combination of assignment and `return` is hard to factor out nicely into a separate function.

> With match_single_line_comment and get_string_token, you have three places handling the progression of line numbers across \n and \r. That's unfortunate.

It is, but the three cases are sufficiently different that I don't see how to factor them out :(

> You could use https://doc.rust-lang.org/std/primitive.u16.html#method.from_str_radix

I tried doing it but it ended up being no more concise than the current code, and it involves scanning the chars twice.

> match c {
>   _ if !SPECIAL_STRING_CHARS[c as usize] => ...,
>   quote_char => ...
>   etc.
> }

It doesn't work because quote_char is a variable, and you can't put a variable in a match arm.

> You don't need a Vec (and thus an allocation) here. You won't have more than two, you can just use a stack-allocated array.

I originally had something like that. It requires an auxiliary variable to track whether 1 or 2 bytes have been read, and then taking a slice of the right length from the array. Using a Vec is simpler and this code is rarely executed so the allocation cost doesn't matter.

> This raises an interesting question: does rust match care about the order or the cases?

mbrubeck told me it does.

> I'm pretty sure slice comparison starts with comparing the length. So you don't need this check. And thus you don't need `let len`.

True. That's a carry-over from the C++ version where the length test was necessary.

> I don't have a better proposal, but because there is a / not followed by another / or * doesn't necessarily mean we are seeing a failed attempt at starting a comment.

I changed it to "expected '/' or '*' after '/'".

> maybe scan the string first, and use https://doc.rust-lang.org/std/primitive.u32.html#method.from_str_radix ?

Like before, I'd prefer not to scan the chars twice.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Bug 1423840 - Remove extraneous semicolons in all.js.

The new prefs parser won't allow them.

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

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

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

Comment 11

[Nicholas Nethercote [inactive]]

As well as addressing a lot of the review comments, the new version adds a gtest for error messages on invalid syntax.

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

https://reviewboard.mozilla.org/r/206210/#review212622

::: modules/libpref/parser/src/lib.rs:290
(Diff revision 3)
> +        Parser {
> +            path: path,
> +            buf: buf,
> +            i: 0,
> +            line_num: 1,
> +            pref_fn: pref_fn,

given that these functions eventually get called without checking for validity _technically_ `Parser::new` should be `unsafe` as well (you can pass it arbitrary pointers), but that's not 100% necessary I guess.

::: modules/libpref/parser/src/lib.rs:303
(Diff revision 3)
> +        let mut value_str = Vec::with_capacity(512); // For string pref values.
> +        let mut none_str  = Vec::with_capacity(0);   // For tokens that shouldn't be strings.
> +
> +        loop {
> +            // The values we are getting for each pref.
> +            let pref_name;

It is not Rust style to use deferred initialization where unnecessary (it's necessary in a very narrow set of cases, which is not what's happening here).

Generally you would declare the variable the first time it's initialized; I'll note out those places in further comments.

::: modules/libpref/parser/src/lib.rs:321
(Diff revision 3)
> +            if token == Token::SingleChar(EOF) {
> +                break;
> +            }
> +
> +            // <pref-spec>
> +            match token {

let (is_sticky, pref_value_kind) = match ...

::: modules/libpref/parser/src/lib.rs:346
(Diff revision 3)
> +                return self.error(token, "expected '(' after pref specifier");
> +            }
> +
> +            // <pref-name>
> +            let token = self.get_token(&mut name_str);
> +            if token == Token::String {

let pref_name = if token ....

::: modules/libpref/parser/src/lib.rs:360
(Diff revision 3)
> +                return self.error(token, "expected ',' after pref name");
> +            }
> +
> +            // <pref-value>
> +            let token = self.get_token(&mut value_str);
> +            match token {

let (pref_type, pref_value) = ....

::: modules/libpref/parser/src/lib.rs:488
(Diff revision 3)
> +    }
> +
> +    #[inline(always)]
> +    fn match_single_line_comment(&mut self) {
> +        loop {
> +            // To reach here, the previous char must have been '/', and

This function should be unsafe if it makes assumptions about where it's called

::: modules/libpref/parser/src/lib.rs:693
(Diff revision 3)
> +        let start = self.i;
> +        let has_special_chars = loop {
> +            // To reach here, the previous char must have been a quote
> +            // (quote_char), and assertions elsewhere ensure that there must be
> +            // at least one subsequent char (the '\0' for EOF).
> +            let c = unsafe { self.get_char_unchecked() };

ditto on the function being unsafe

Comment 13

[Manish Goregaokar [:manishearth]]

> Kind of sad to make a roundtrip from utf-8 on non-windows.

FWIW you can use the nsString crate and just use nsStrings directly and operate on them.

Comment 14

[James Graham [:jgraham]]

Is it going to be possible to replace the custom parser in geckodriver (via rust_mozprofile) with this code?

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to James Graham [:jgraham] from comment #14)
> Is it going to be possible to replace the custom parser in geckodriver (via
> rust_mozprofile) with this code?

It's a good question. I think the main issue is where the prefs_parser crate is stored. I was planning on putting it in mozilla-central because that's simplest for libpref, but then it can't be shared with geckodriver, AIUI. So I guess it would instead need to be hosted at https://github.com/mozilla/prefs_parser/ and published to crates.io?

Comment 16

[James Graham [:jgraham]]

geckodriver is mostly in-tree and we can move other deps gecko-specific in-tree if needed. You can also publish cargo crates from the tree (e.g. that's how the WebDriver crate is published). So I don't think you need to move to GitHub just to publish as a crate.

Comment 17

[Nicholas Nethercote [inactive]]

> given that these functions eventually get called without checking for
> validity _technically_ `Parser::new` should be `unsafe` as well (you can
> pass it arbitrary pointers), but that's not 100% necessary I guess.
>
> This function should be unsafe if it makes assumptions about where it's
> called

This is the first time I've heard of somebody suggesting using `unsafe` in a place where the compiler doesn't require it.
Is that a common practice? (I'm accustomed to the compiler warning me when I use `unsafe` unnecessarily.) 

Particular with the latter suggestion (about get_string_token()) I suspect a reader would be very confused about why the function is marked `unsafe`.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8935700 [details]
Bug 1423840 - Remove extraneous semicolons in all.js.

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

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206210/diff/3-4/

Comment 20

[Manish Goregaokar [:manishearth]]

> This is the first time I've heard of somebody suggesting using `unsafe` in a place where the compiler doesn't require it.

The idea is any function which can be called in a way that can cause UB is unsafe.

https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html and the "non-locality" stuff in https://doc.rust-lang.org/nomicon/working-with-unsafe.html

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8935700 [details]
Bug 1423840 - Remove extraneous semicolons in all.js.

https://reviewboard.mozilla.org/r/206600/#review214080

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.

https://reviewboard.mozilla.org/r/206210/#review214082

::: modules/libpref/parser/src/lib.rs:628
(Diff revisions 2 - 4)
>                      loop {
>                          let c = self.get_char();
> -                        match CHAR_KINDS[c as usize] {
> +                        match Parser::char_kind(c) {
>                              CharKind::Digit => {
>                                  fn add_digit(v: u32, c: u8) -> Option<u32> {
> -                                    Some(v.checked_mul(10)?.checked_add((c - b'0') as u32)?)
> +                                    // XXX: Once Rust 1.22 is fully supported change this to:

Note we're not too far from that now.

::: modules/libpref/parser/src/lib.rs:681
(Diff revisions 2 - 4)
> +            } else if c == quote_char {
> +                break false;
> +            } else {
> +                break true;
> +            }

} else {
  break c != quote_char;
}

?

Comment 23

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e500592d3551ecf3b063f3e054d22bbbb6f9c54d
Bug 1423840 - Remove extraneous semicolons in all.js. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b798a5205ad27d067b91210efb46df7ddab45d
Bug 1423840 - Rewrite the prefs parser. r=glandium,Manishearth

Comment 24

[Cosmin Sabou [:CosminS]]

Backed out 2 changesets (bug 1423840) for mass Talos failures due to forbidden connections.

Treeherder push with the fail: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e8b798a5205ad27d067b91210efb46df7ddab45d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=talos

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159710272&repo=mozilla-inbound&lineNumber=1298

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/c30e31ae61f96947e0737f0faac8f4de64d04a0e

Comment 25

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ebdf597ade8b7c25edd3ea75babd721f28fc4a3
Bug 1423840 (attempt 2) - Remove extraneous semicolons in all.js. r=glandium

Comment 26

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e655361be5a1971300c15f58ec65c81f82ba826
Bug 1423840 (attempt 2) - Rewrite the prefs parser. r=glandium,Manishearth

Comment 27

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03a0fe367bb10467ec034af93e4b26e23fc1e54c
Bug 1423840 - Temporarily disable a small part of the prefs parser gtest due to failures on Windows. r=me

Comment 28

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8ebdf597ade8
https://hg.mozilla.org/mozilla-central/rev/4e655361be5a
https://hg.mozilla.org/mozilla-central/rev/03a0fe367bb1

Comment 29

[Jorg K (CEST = GMT+2)]

Can I ask a question? Why does a syntax error abort the entire file after that error, is that intentional? This busted TB since we had this somewhere in our preferences:

 pref("toolkit.crashreporter.infoURL",
-     "https://www.mozilla.org/thunderbird/legal/privacy/#crash-reporter");");
+     "https://www.mozilla.org/thunderbird/legal/privacy/#crash-reporter");

Comment 30

[Nicholas Nethercote [inactive]]

(In reply to Jorg K (GMT+1) from comment #29)
> Can I ask a question? Why does a syntax error abort the entire file after
> that error, is that intentional?

The old parser behaved the same way. I preserved the behaviour because it's easier to abort than recover. The good news is that it will be *much* easier to add error recovery to the new parser if we want to.

However, I'm not sure if error recovery is a good idea... these syntax errors are only reported on the console, which isn't an obvious place. With error recovery in place a small number of syntax errors could easily be overlooked, with those prefs not being installed. Without error recovery it's more likely that a lot of prefs won't be installed, which leads to more obvious problems. (For example, I might not have found and fixed bug 1424030 and bug 1434813 if error recovery was implemented.)

> This busted TB

Sorry for the bustage. I did a quick scan of TB files before landing (mostly looking for repeated semicolons, which was a problem in Firefox's prefs) but I overlooked that error. I should have actually run those files through the parser instead of relying on visual inspection.

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to Nicholas Nethercote [:njn] from comment #30)
> > This busted TB
> Sorry for the bustage. I did a quick scan of TB files before landing (mostly
> looking for repeated semicolons, which was a problem in Firefox's prefs) but
> I overlooked that error. I should have actually run those files through the
> parser instead of relying on visual inspection.
Thanks for looking, you really don't need to be sorry. The pref was just wrong. It took us a while to work it out though. So some sort of message other than "a lot of prefs won't be installed, which leads to more obvious problems" would be great, don't you think? Since TB was so busted, we didn't even get to the error console. And there was nothing in the debug console, or did I miss it?

So how do you run the parser manually?

Comment 32

[Nicholas Nethercote [inactive]]

> So some sort of message
> other than "a lot of prefs won't be installed, which leads to more obvious
> problems" would be great, don't you think? Since TB was so busted, we didn't
> even get to the error console. And there was nothing in the debug console,
> or did I miss it?

It uses the console service obtained via "@mozilla.org/consoleservice;1". In Firefox this is the browser console. I'm not sure what the equivalent of that is in Thunderbird.

The current behaviour is here: https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#1000-1011.
It writes the error to that console, but if that fails, prints it to stderr. And then it does an NS_WARNING. Can you think of a better way? Perhaps it should always print to stderr? Or do NS_ERROR instead of NS_WARNING?

> So how do you run the parser manually?

There isn't a nice way. I'd probably copy the contents of the file you want to check into a P macro in modules/libpref/test/gtest/Parser.cpp and then run that test and see what happens.

Comment 33

[Jorg K (CEST = GMT+2)]

Yes, we call it error console, but if the UI is busted, you can't get there. I looked at the code, the warning is probably enough, but I didn't see it. So I put the syntax error back and started again. Don't laugh, but this is what I see:

  chrome://global/content/bindings/general.x[m61l56,# Marin oThroeatd]- WeARNlIN
eG: mC:\emoznillta-s
ur ce\ comcm-chentrralo\obmj-ie686:-pc/-mi/ngwg32\ldisot\bbin\adeflaul/ts\cpreof
\nallt-tehunndertbir/d.jbs:i113n: pdrefis pnargse serr/or:g exepecntede prref as
pleci.fiexr amt slta
t o f p reff diefilniteion:: f/ile/ c://moCzil:la-/soumrceo/cozmm-icenltrall/mao
zi-llas/moodulues/rlibcpreef//Prcefeorenmcesm.cp-p,c leinne t101r0
l[/615o6, bMaijn T-hreiad]6 ##8#!!6! A-SSEpRTIcON:- Dmefaiultn prgef wfi3le2 no/
t dpariseds sutcce/ssfbulliy.:n '/Ercrohr',r fiolem ce:/m/ozitllao-soourcle/ckom
im-ctent/ralc/moozilnla/tmodeulens/ltibp/refg/Prlefoerebnceas.clpp,/ libne i360n
7
dings/g[61e56,n Maeinr Tahrelad]. WAxRNImNG:l Er
or  pa rscingh aprpliocamtioen d:efa/ult/ prgefelrenocesb.: afille c/:/cmozoilln
a-stourece/ncomtm-c/entbrali/monzildla/imodnulegs/lsibp/refg/Preefenrenecesr.cpa
p,l li.ne x375m3
l

:-(

Not so easy to spot the parsing error in there.

Comment 35

[Jukka Jylänki]

This refactoring seems to have brought a curious looking unintended(?) side effect: marked it down as 1423840 to verify what that's all about.

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Was comment 35 intended like this? It seems to be missing context. Wrong bug number?

Comment 37

[Jukka Jylänki]

Oops, thanks, pasted this bug number in there.

Here's the referenced bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1438878