Last Comment Bug 107264 - bad syntax in prefs.js causes file to be ignored.
: bad syntax in prefs.js causes file to be ignored.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: x86 All
P3 major with 3 votes (vote)
: mozilla60
Assigned To: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
:
: Nicholas Nethercote [:njn]
Mentors:
: 107273 111532 115093 142670 146988 191101 200297 233622 1399444 1438878 (view as bug list)
Depends on: 98533
Blocks: profile-corrupt
  Show dependency treegraph
 
Reported: 2001-10-28 14:32 PST by Basil Fritts
Modified: 2018-09-22 07:31 PDT (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
partially corrupted prefs.js (last line is bad) (4.00 KB, application/x-javascript)
2003-05-16 05:42 PDT, Mantas Kriaučiūnas
no flags Details
Patch (8.45 KB, patch)
2004-02-09 21:07 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
darin.moz: superreview-
Details | Diff | Splinter Review
Bug 107264 - Make prefs parser errors louder. (59 bytes, text/x-review-board-request)
2018-02-18 21:58 PST, Nicholas Nethercote [:njn]
glandium: review+
Details
Bug 107264 - Add error recovery to the prefs parser. (59 bytes, text/x-review-board-request)
2018-02-18 21:58 PST, Nicholas Nethercote [:njn]
glandium: review+
Details

Description User image Basil Fritts 2001-10-28 14:32:11 PST
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 User image Bradley Baetz (:bbaetz) 2001-10-28 14:52:20 PST
-> 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 User image Brian Nesse (gone) 2001-10-29 09:36:45 PST
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 User image sairuh (rarely reading bugmail) 2001-10-29 13:15:44 PST
confirm, adding dep.
Comment 4 User image R.K.Aa. 2001-11-25 19:04:22 PST
*** Bug 111532 has been marked as a duplicate of this bug. ***
Comment 5 User image Robert Spotswood 2001-11-25 22:09:26 PST
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 User image Brian Nesse (gone) 2001-11-27 09:38:09 PST
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 User image Robert Spotswood 2001-11-27 09:59:17 PST
> ...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 User image R.K.Aa. 2001-12-13 11:41:35 PST
*** Bug 115093 has been marked as a duplicate of this bug. ***
Comment 9 User image Brian Nesse (gone) 2002-01-30 08:57:47 PST
*** Bug 107273 has been marked as a duplicate of this bug. ***
Comment 10 User image R.K.Aa. 2002-05-06 18:07:40 PDT
*** Bug 142670 has been marked as a duplicate of this bug. ***
Comment 11 User image R.K.Aa. 2002-05-25 04:26:04 PDT
*** Bug 146988 has been marked as a duplicate of this bug. ***
Comment 12 User image Conrad Carlen (not reading bugmail) 2003-02-12 10:48:28 PST
Taking some of Brian's bugs.
Comment 13 User image Conrad Carlen (not reading bugmail) 2003-02-12 13:17:35 PST
Setting to milestone that's not passed.
Comment 14 User image Conrad Carlen (not reading bugmail) 2003-02-24 09:23:57 PST
-> Future. See 2nd para of comment 2.
Comment 15 User image Mantas Kriaučiūnas 2003-05-16 05:40:21 PDT
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 User image Mantas Kriaučiūnas 2003-05-16 05:42:44 PDT
Created attachment 123498 [details]
partially corrupted prefs.js (last line is bad)
Comment 17 User image cmbirt 2003-10-25 10:14:27 PDT
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 User image R.K.Aa. 2004-02-09 20:57:50 PST
*** Bug 233622 has been marked as a duplicate of this bug. ***
Comment 19 User image Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2004-02-09 21:07:14 PST
Created attachment 141016 [details] [diff] [review]
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 User image Benjamin Smedberg 2004-02-09 22:00:09 PST
I don't think we ought to attempt recovery, and this bug should be WONTFIXed.
Comment 21 User image Brendan Eich [:brendan] 2004-02-09 22:05:06 PST
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 User image Darin Fisher 2004-02-09 23:33:48 PST
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 User image Darin Fisher 2004-02-09 23:34:47 PST
-> me
Comment 24 User image Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2004-02-10 09:11:25 PST
>> 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 User image Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2004-02-10 20:04:49 PST
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 User image Darin Fisher 2004-03-02 15:26:38 PST
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 User image Asa Dotzler [:asa] 2004-03-25 15:24:39 PST
*** Bug 200297 has been marked as a duplicate of this bug. ***
Comment 28 User image Serge Gautherie (:sgautherie) 2008-06-11 15:02:17 PDT
(Filter "spam" on 'prefs-nobody-20080612'.)
Comment 29 User image Benjamin Smedberg 2013-02-15 14:00:04 PST
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 30 User image Wayne Mery (:wsmwk) 2013-02-19 04:12:35 PST
*** Bug 191101 has been marked as a duplicate of this bug. ***
Comment 31 User image Nicholas Nethercote [:njn] 2017-11-28 21:23:14 PST
*** Bug 1399444 has been marked as a duplicate of this bug. ***
Comment 32 User image Nicholas Nethercote [:njn] 2018-02-18 21:58:51 PST Comment hidden (mozreview-request)
Comment 33 User image Nicholas Nethercote [:njn] 2018-02-18 21:58:51 PST Comment hidden (mozreview-request)
Comment 35 User image Mike Hommey [:glandium] 2018-02-19 16:19:25 PST
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 User image Mike Hommey [:glandium] 2018-02-19 16:21:05 PST
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 User image Mike Hommey [:glandium] 2018-02-19 16:52:39 PST
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 User image Nicholas Nethercote [:njn] 2018-02-20 11:56:17 PST
> > +#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 User image Nicholas Nethercote [:njn] 2018-02-20 11:58:04 PST
> 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 User image Nicholas Nethercote [:njn] 2018-02-20 13:31:44 PST
> 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 User image Nicholas Nethercote [:njn] 2018-02-20 13:49:52 PST
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 User image Mike Hommey [:glandium] 2018-02-20 14:05:32 PST
(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 User image Nicholas Nethercote [:njn] 2018-02-20 15:32:18 PST
> It's rather new as a #define.

Is it identical in meaning to DEBUG? Having two identical constants doesn't seem useful...
Comment 44 User image Mike Hommey [:glandium] 2018-02-20 15:40:19 PST
(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 45 User image Nicholas Nethercote [:njn] 2018-02-20 18:11:00 PST
*** Bug 1438878 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.