Closed Bug 107264 Opened 23 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Preferences: Backend
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: basil, Assigned: csthomas)

References

Details

Attachments

(4 files)

partially corrupted prefs.js (last line is bad)
4.00 KB, application/x-javascript
Details
Patch
8.45 KB, patch
darin.moz
: superreview-
Details | Diff | Splinter Review
Bug 107264 - Make prefs parser errors louder.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 107264 - Add error recovery to the prefs parser.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
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
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
*** Bug 111532 has been marked as a duplicate of this bug. ***
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.

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.
> ...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.

*** Bug 115093 has been marked as a duplicate of this bug. ***
Target Milestone: --- → mozilla1.0
*** Bug 107273 has been marked as a duplicate of this bug. ***
Blocks: profile-corrupt
Target Milestone: mozilla1.0 → mozilla1.1
*** Bug 142670 has been marked as a duplicate of this bug. ***
*** 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 :(
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
*** Bug 233622 has been marked as a duplicate of this bug. ***
Attached patch Patch — — Splinter Review 
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.
Status: NEW → ASSIGNED
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
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!
-> 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 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. ***
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
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
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 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 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.
https://hg.mozilla.org/mozilla-central/rev/a837bfff20dd
https://hg.mozilla.org/mozilla-central/rev/689995322a89
Status: NEW → RESOLVED
Closed: 6 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: