Closed
Bug 1423840
Opened 7 years ago
Closed 7 years ago
Rewrite the prefs parser
Categories
(Core :: Preferences: Backend, enhancement)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
The prefs parser isn't good.
| Comment hidden (mozreview-request) |
|
Assignee | |
Comment 2•7 years ago
|
||
glandium: please review from a prefs POV.
Manish: please review from a Rust POV.
| Comment hidden (mozreview-request) |
|
||
Comment 4•7 years ago
|
||
| mozreview-review | ||
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•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935310 [details]
Bug 1423840 - Rewrite the prefs parser.
https://reviewboard.mozilla.org/r/206210/#review211982
|
|
Assignee | |
Comment 6•7 years ago
|
||
> 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•7 years ago
|
||
| mozreview-review | ||
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.
Attachment #8935310 -
Flags: review?(mh+mozilla)
|
|
||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
|
|
Assignee | |
Comment 8•7 years ago
|
||
| mozreview-review-reply | ||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 11•7 years ago
|
||
As well as addressing a lot of the review comments, the new version adds a gtest for error messages on invalid syntax.
|
||
Comment 12•7 years ago
|
||
| mozreview-review | ||
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
Attachment #8935310 -
Flags: review?(manishearth) → review+
|
|
||
Comment 13•7 years ago
|
||
> 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•7 years ago
|
||
Is it going to be possible to replace the custom parser in geckodriver (via rust_mozprofile) with this code?
|
|
Assignee | |
Comment 15•7 years ago
|
||
(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•7 years ago
|
||
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.
|
|
Assignee | |
Comment 17•7 years ago
|
||
> 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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 20•7 years ago
|
||
> 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•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935700 [details]
Bug 1423840 - Remove extraneous semicolons in all.js.
https://reviewboard.mozilla.org/r/206600/#review214080
Attachment #8935700 -
Flags: review?(mh+mozilla) → review+
|
|
||
Comment 22•7 years ago
|
||
| mozreview-review | ||
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;
}
?
Attachment #8935310 -
Flags: review?(mh+mozilla) → review+
|
|
Assignee | |
Comment 23•7 years ago
|
||
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•7 years ago
|
||
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
Flags: needinfo?(n.nethercote)
|
|
Assignee | |
Comment 25•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ebdf597ade8b7c25edd3ea75babd721f28fc4a3
Bug 1423840 (attempt 2) - Remove extraneous semicolons in all.js. r=glandium
|
|
Assignee | |
Comment 26•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e655361be5a1971300c15f58ec65c81f82ba826
Bug 1423840 (attempt 2) - Rewrite the prefs parser. r=glandium,Manishearth
|
|
Assignee | |
Comment 27•7 years ago
|
||
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
|
|
Assignee | |
Updated•7 years ago
|
Flags: needinfo?(n.nethercote)
|
||
Comment 28•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/8ebdf597ade8
https://hg.mozilla.org/mozilla-central/rev/4e655361be5a
https://hg.mozilla.org/mozilla-central/rev/03a0fe367bb1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
|
||
Comment 29•7 years ago
|
||
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");
|
|
Assignee | |
Comment 30•7 years ago
|
||
(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•7 years ago
|
||
(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?
|
|
Assignee | |
Comment 32•7 years ago
|
||
> 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•7 years ago
|
||
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.
|
||
Updated•7 years ago
|
|
||
Comment 35•7 years ago
|
||
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•7 years ago
|
||
Was comment 35 intended like this? It seems to be missing context. Wrong bug number?
Flags: needinfo?(jujjyl)
|
|
||
Comment 37•7 years ago
|
||
Oops, thanks, pasted this bug number in there.
Here's the referenced bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1438878
Flags: needinfo?(jujjyl)
You need to log in
before you can comment on or make changes to this bug.
Description
•