107264 - bad syntax in prefs.js causes file to be ignored.

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect, P3)

Description

[Basil Fritts]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.5) Gecko/20011020
BuildID:    2001102000

If a line with bad syntax is added to prefs.js by the user, then the entire file
is ignored.

Reproducible: Always
Steps to Reproduce:
1.Add a line with bad syntax to prefs.js.
2.Start the browser.

Actual Results:  All information in prefs.js is invisible. No mail/news account
settings, no browsing preferences, nothing.

Expected Results:  Either ignore the one line with bad syntax or pop an error
message.

I know that one really should not edit the generated files, but people do
anyway, right? Although failing gracefully often means failing silently and
moving on, in this case Mozilla's silence was frightening. ("What? prefs.js is
right there! Why isn't it loading? What do I do now?!") I understand that user
prefs should be put in user.js. (Or I should say, //now// I know that!:-D)
However, an error message in this case would have been far more useful than
failing silently. Or perhaps, ignoring only the line in question with an error
message about the bad line. (On the plus side, several times during this episode
I tried resetting my prefs in the UI -- which could have led to serious
dataloss. Kudos to the code that kept the original prefs.js file from being
overwritten!)

Comment 1

[Bradley Baetz (:bbaetz)]

-> prefs backend

We really should print a warning if loading the js files failed. I'm not sure
why it wasn't clobbered on shutdown.

Comment 2

[Brian Nesse (gone)]

It used to bring up an error dialog when this happened. I was told to remove
this as the preferences library should have no dependencies on the UI. I can
certainly add an NS_WARNING that it failed.

As the parsing is happening in the JS engine right now, I'm not sure how
feasible it is to ignore the bad line. That might have to wait until we do
something drastic like remove the JS dependence from prefs. See bug 98533.

Comment 3

[sairuh (rarely reading bugmail)]

confirm, adding dep.

Comment 4

[R.K.Aa.]

*** Bug 111532 has been marked as a duplicate of this bug. ***

Comment 5

[Robert Spotswood]

Actually, not all bad syntax causes the file to be ignored.  For 
instance, adding the (bad syntax) line

user.pref("network.cookie.rememberAboutCookies", true);

(which is taken directly from the release notes; I need to figure out 
who to email about changing them) does not cause the prefs.js file to be 
ignored, although I believe it does cause user.js to be ignored.  Notice 
the line has a period between the user and pref rather than an 
underscore.

However, it will prevent the prefs.js file from being overwritten, 
which means any changes you make in the GUI are lost.  Very subtle and 
frustrating.

Comment 6

[Brian Nesse (gone)]

Bad syntax will cause whatever file the bad syntax is in to be ignored. If
prefs.js is bad, it will be ignored. If user.js is bad, it will be ignored. This
is because the JS parser will kick out when it hits a line it doesn't
understand. I believe that any prefs sucessfully processed before the bad syntax
will be retained, but I'm not 100% certain.

Comment 7

[Robert Spotswood]

> ...believe that any prefs sucessfully processed before the bad
> syntax will be retained, but I'm not 100% certain.

My tests confirm this.  Good prefs that occur before the bad one are 
retained.

Comment 8

[R.K.Aa.]

*** Bug 115093 has been marked as a duplicate of this bug. ***

Comment 9

[Brian Nesse (gone)]

*** Bug 107273 has been marked as a duplicate of this bug. ***

Comment 10

[R.K.Aa.]

*** Bug 142670 has been marked as a duplicate of this bug. ***

Comment 11

[R.K.Aa.]

*** Bug 146988 has been marked as a duplicate of this bug. ***

Comment 12

[Conrad Carlen (not reading bugmail)]

Taking some of Brian's bugs.

Comment 13

[Conrad Carlen (not reading bugmail)]

Setting to milestone that's not passed.

Comment 14

[Conrad Carlen (not reading bugmail)]

-> Future. See 2nd para of comment 2.

Comment 15

[Mantas Kriaučiūnas]

My profile is partially corrupted - last line is:

user_

When mozilla starts it reads all correct lines from my profile, but when I
change something in preferences the changes are not saved :(

I think mozilla should simply ignore corrupted lines or at least inform user:
"Your profile ~/.mozilla/path_to _profile/pregs.js is corrupted - please correct
the profile manually"

Because of this bug many users are getting nervous - they can't save passwords,
proxy, etc. and didn't know why these things appear :(

Comment 16

[Mantas Kriaučiūnas]

Attachment: partially corrupted prefs.js (last line is bad)

Comment 17

[cmbirt]

When starting up Mozilla, 'Configuration Warning' reads: 

An error occurred reading the startup configuration file. Please contact your 
administrator. prejs.js, line 62: SyntaxError: unterminated string literal. 
user_pref("intl.ch

I think that this refers to this bug, is there a fix coming or can a non-coder 
fix this easily (and how)? I tried going back to version 1.4.1, but the same 
error message pops up, and I don't have any of my preferences. 
thanks for your hard work, I love mozilla. It is just difficult to see how a 
non-coder could easily contribute, for instance it is hard to understand how 
this bugzilla thing is supposed to work

Comment 18

[R.K.Aa.]

*** Bug 233622 has been marked as a duplicate of this bug. ***

Comment 19

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

This patch requires some careful reviewing.  I can't imagine it would ever
actually do anything bad, but I'm not familiar enough with the prefs system to
know what side effects it might have.

Comment 20

[Benjamin Smedberg]

I don't think we ought to attempt recovery, and this bug should be WONTFIXed.

Comment 21

[Brendan Eich [:brendan]]

At least cc: darin, if not reassign to him -- it's obvious from cvs logs that he
wrote this code, so he is in the best position to judge whether this patch is
one that should go in.

/be

Comment 22

[Darin Fisher]

well, chris and i discussed this on IRC.  i helped him a bit with the patch...
i'm not sure if it is wise or not to go with it.  recovering from a malformed
pref file is risky business... it is useful to know that the pref file is
corrupt.  maybe it would be better to backup the corrupt file, and then continue
on as we do today.  that might be better than trying to workaround the problem.
 the fear i have (and this is why i didn't add this patch originally) is that it
could lead to strange pref settings that result in really bogus behavior.  what
if a security pref got switched inadvertantly (in a silent fashion no less)?!? 
sure, that's unlikely, but i think we should be very careful with this.

and, we now have about:config which if used to edit prefs.js makes sure that it
does not get corrupted.  maybe we should "chmod a-w prefs.js" to make people
think twice before editing it.  heck, we could even put a reference about:config
in the comment at the top of that file!

Comment 23

[Darin Fisher]

-> me

Comment 24

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

>> maybe it would be better to backup the corrupt file, and then
>> continue on as we do today.  that might be better than trying
>> to workaround the problem.
Ideally the user should be notified that some of their preferences may have been
lost, so they are given the opportunity to look at prefs.bak before it gets
overwritten at some later date.

>> the fear i have (and this is why i didn't add this patch originally)
>> is that it could lead to strange pref settings that result in really
>> bogus behavior.  what if a security pref got switched inadvertantly
>> (in a silent fashion no less)?!? 
>> sure, that's unlikely, but i think we should be very careful with this.
First, it shouldn't be silent - even novice users would probably like to know
that mozilla just blew away half of their mail settings so they can go fix them
(and so they don't wonder why moz started warning them about secure connections,
etc).  Maybe that deserves a separate bug.

Second, the present behavior allows security prefs to be silently reset to the 
defaults, since that's what ends up happening to every pref after a broken line.  

I guess the defaults are reasonably secure - the only question is whether or not
it's possible to get a different value for a pref after a broken pref.

As brendan noted in the original pref parser rewrite bug, a combination multiple
holes might allow this with the new behavior, creating a pref such as
user_pref("browser.some_int_pref", "-" user_pref("security.turn_into_MSIE", 1);");

After the parser breaks on the minus sign without a number and restarts parsing,
we will get a vulnerable setup.  I'd have to check this possibility tonight.

Comment 25

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

on IRC:

<bz> the right thing to do is to lock prefs down on any error during startup,
disallow changing prefs, report an error to the user, and NOT write out prefs on
shutdown, imo..
<bz> the equivalent of Windows' "safe" mode, for prefs

Comment 26

[Darin Fisher]

Comment on attachment 141016 [details] [diff] [review]
Patch

minusing...

bz's idea sounds pretty good to me.  i'm all for finding a way to inform the
user and at the same time avoid stomping over an existing prefs file when
errors have been detected.

Comment 27

[Asa Dotzler [:asa]]

*** Bug 200297 has been marked as a duplicate of this bug. ***

Comment 28

[Serge Gautherie (:sgautherie)]

(Filter "spam" on 'prefs-nobody-20080612'.)

Comment 29

[Benjamin Smedberg]

I really want telemetry for this. UI sounds hard, but since we're not using a real JS parser now we should just try to recover...

Comment 32

[Nicholas Nethercote [inactive]]

Attachment: Bug 107264 - Make prefs parser errors louder.

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

Comment 33

[Nicholas Nethercote [inactive]]

Attachment: Bug 107264 - Add error recovery to the prefs parser.

This was first suggested 17 years ago!

The error recovery works by just scanning forward for the next ';' token.

This change allows a lot of the gtest tests to be combined.

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

Comment 34

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7a6b135e31411835b79b869fabf284c772fa2c7

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8952061 [details]
Bug 107264 - Make prefs parser errors louder.

https://reviewboard.mozilla.org/r/221282/#review227292

::: modules/libpref/Preferences.cpp:1110
(Diff revision 1)
>      if (NS_SUCCEEDED(rv)) {
>        console->LogStringMessage(NS_ConvertUTF8toUTF16(aMsg).get());
> -    } else {
> -      printf_stderr("%s\n", aMsg);
>      }
> -    NS_WARNING(aMsg);
> +#ifdef DEBUG

MOZ_DEBUG

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 8952061 [details]
Bug 107264 - Make prefs parser errors louder.

https://reviewboard.mozilla.org/r/221282/#review227294

::: modules/libpref/Preferences.cpp:1113
(Diff revision 1)
> -      printf_stderr("%s\n", aMsg);
>      }
> -    NS_WARNING(aMsg);
> +#ifdef DEBUG
> +    NS_ERROR(aMsg);
> +#else
> +    printf_stderr("%s\n", aMsg);

Note, iirc, there's a pref to make the console service send messages to stderr too. I'm not entirely sure code should randomly do that on its own.

Comment 37

[Mike Hommey [:glandium]]

Comment on attachment 8952062 [details]
Bug 107264 - Add error recovery to the prefs parser.

https://reviewboard.mozilla.org/r/221284/#review227296

::: modules/libpref/parser/src/lib.rs:474
(Diff revision 1)
> +                Token::SingleChar(b';') => return self.get_token(&mut dummy_str),
> +                Token::SingleChar(EOF) => return token,

Until I got here, I was confused by the `token = self.error(); continue` patterns. And then it clicked: error returns the next token. Which is really not obvious when reading `token = self.error()` and when Error is a form of token. Please change the method name.

::: modules/libpref/parser/src/lib.rs:493
(Diff revision 1)
>  
> -    // This function skips the bounds check. Using it at the hottest two call
> -    // sites gives a ~15% parsing speed boost.
> +    // This function skips the bounds check in non-optimized builds. Using it
> +    // at the hottest two call sites gives a ~15% parsing speed boost.
>      #[inline(always)]
>      unsafe fn get_char_unchecked(&mut self) -> u8 {
> +        debug_assert!(self.i < self.buf.len());

alternatively, you could circle back to get_char with the condition if cfg!(debug_assertions).

::: modules/libpref/parser/src/lib.rs:712
(Diff revision 1)
> +    fn string_error_token(&self, token: &mut Token, msg: &'static str) {
> +        // We only want to capture the first tokenization error within a string.
> +        if *token == Token::String {
> +            *token = Token::ErrorAtLine(msg, self.line_num);
> +        }
> +    }

I /think/ it would be more idiomatic if it were something like:

fn string_error_token(&self, token: Token, msg: &'static str) -> Token {
  if token == Token::String {
    Token::ErrorAtLine(msg, self.line_num)
  } else {
    token
  }
}

At least it feels like it would make things clearer at the call sites, although the mutable references are already a hint.

::: modules/libpref/parser/src/lib.rs:821
(Diff revision 1)
> +                            continue;
>                          }
>                          continue; // We don't want to str_buf.push(c2) below.
>                      }
> -                    _ => return Token::Error("unexpected escape sequence character after '\\'")
> +                    _ => {
> +                        // Unget in case the char an EOF.

missing "is" ?

::: modules/libpref/test/gtest/Parser.cpp:63
(Diff revision 1)
>    // in the one spot.)
>    //-------------------------------------------------------------------------
>  
>    // Integer overflow errors.
> -
>    P(R"(

fwiw, I think it wouldn't hurt if you just had one big test case with all the errors caught at once.

Comment 38

[Nicholas Nethercote [inactive]]

> > +#ifdef DEBUG
> 
> MOZ_DEBUG

I don't think so? There are only 10 occurrences of MOZ_DEBUG in .h and .cpp files in the codebase. 9 of them are in memory/build/, and 1 is in xpcom/. MOZ_DEBUG does show up in moz.build files quite a bit, though.

Comment 39

[Nicholas Nethercote [inactive]]

> Note, iirc, there's a pref to make the console service send messages to
> stderr too. I'm not entirely sure code should randomly do that on its own.

I think it's really useful for Firefox devs to be guaranteed to see an error on a prefs parse error. This was the best way I could think of doing that.

Comment 40

[Nicholas Nethercote [inactive]]

> Please change the method name.

I'll call it error_and_recover().


> I /think/ it would be more idiomatic if it were something like:
> 
> fn string_error_token(&self, token: Token, msg: &'static str) -> Token {

I went to do that, and then I nearly forgot to put the "token = " in front of the function calls. AFAICT Rust only supports `must_use` on types, not functions, and it feels strange to put `must_use` on the Token type. So I kept this unchanged.


> fwiw, I think it wouldn't hurt if you just had one big test case with all
> the errors caught at once.

It can't be a single big test case because a number of them need EOF in a particular place. I grouped the non-EOF ones where it  made sense in terms of the comments, but I'm happy with the current level of separation.

Comment 41

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a837bfff20dd30cf1dfee28abf9de7379b0b064a
Bug 107264 - Make prefs parser errors louder. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/689995322a89a666a7c62cbad1e31787a75562b6
Bug 107264 - Add error recovery to the prefs parser. r=glandium

Comment 42

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #38)
> > > +#ifdef DEBUG
> > 
> > MOZ_DEBUG
> 
> I don't think so? There are only 10 occurrences of MOZ_DEBUG in .h and .cpp
> files in the codebase. 9 of them are in memory/build/, and 1 is in xpcom/.
> MOZ_DEBUG does show up in moz.build files quite a bit, though.

It's rather new as a #define.

Comment 43

[Nicholas Nethercote [inactive]]

> It's rather new as a #define.

Is it identical in meaning to DEBUG? Having two identical constants doesn't seem useful...

Comment 44

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #43)
> > It's rather new as a #define.
> 
> Is it identical in meaning to DEBUG? Having two identical constants doesn't
> seem useful...

See bug 1261161.

Comment 46

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a837bfff20dd
https://hg.mozilla.org/mozilla-central/rev/689995322a89