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

RESOLVED FIXED in Firefox 60

Status

()

P3
major
RESOLVED FIXED
18 years ago
4 months ago

People

(Reporter: basil, Assigned: csthomas)

Tracking

(Blocks: 1 bug)

Trunk
mozilla60
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
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!)
-> 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.
Assignee: rogerl → bnesse
Component: Javascript Engine → Preferences: Backend
QA Contact: pschwartau → sairuh

Comment 2

18 years ago
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.
confirm, adding dep.
Status: UNCONFIRMED → NEW
Depends on: 98533
Ever confirmed: true

Comment 4

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

Comment 5

17 years ago
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

17 years ago
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

17 years ago
> ...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

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

Updated

17 years ago
Target Milestone: --- → mozilla1.0

Comment 9

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

Updated

17 years ago
Blocks: 123929

Updated

17 years ago
Target Milestone: mozilla1.0 → mozilla1.1

Comment 10

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

Comment 11

17 years ago
*** Bug 146988 has been marked as a duplicate of this bug. ***
Taking some of Brian's bugs.
Assignee: bnesse → ccarlen
Setting to milestone that's not passed.
Target Milestone: mozilla1.1alpha → mozilla1.4alpha
-> Future. See 2nd para of comment 2.
Target Milestone: mozilla1.4alpha → Future
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 :(
Created attachment 123498 [details]
partially corrupted prefs.js (last line is bad)

Comment 17

15 years ago
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

15 years ago
*** Bug 233622 has been marked as a duplicate of this bug. ***
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.
Attachment #141016 - Flags: superreview?(darin)
Status: NEW → ASSIGNED

Comment 20

15 years ago
I don't think we ought to attempt recovery, and this bug should be WONTFIXed.
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

15 years ago
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

15 years ago
-> me
Assignee: ccarlen → darin
Status: ASSIGNED → NEW
>> 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.
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

15 years ago
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.
Attachment #141016 - Flags: superreview?(darin) → superreview-
Attachment #141016 - Flags: review?(brendan)
*** Bug 200297 has been marked as a duplicate of this bug. ***
OS: Linux → All

Updated

13 years ago
Assignee: darin → prefs
QA Contact: bugzilla
Target Milestone: Future → ---
(Filter "spam" on 'prefs-nobody-20080612'.)
Assignee: prefs → nobody
QA Contact: prefs
QA Contact: preferences → preferences-backend

Updated

6 years ago
Severity: normal → major
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...
Priority: -- → P3

Updated

6 years ago
Duplicate of this bug: 191101
Duplicate of this bug: 1399444
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

11 months ago
mozreview-review
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
Attachment #8952061 - Flags: review?(mh+mozilla) → review+

Comment 36

11 months ago
mozreview-review
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

11 months ago
mozreview-review
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.
Attachment #8952062 - Flags: review?(mh+mozilla) → review+
> > +#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.
> 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.
> 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.
(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.
> It's rather new as a #define.

Is it identical in meaning to DEBUG? Having two identical constants doesn't seem useful...
(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.
Duplicate of this bug: 1438878

Comment 46

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a837bfff20dd
https://hg.mozilla.org/mozilla-central/rev/689995322a89
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee: nobody → csthomas
You need to log in before you can comment on or make changes to this bug.