63117 - Numbers in prefs accepted in octal when started with 0 and hexadecimal when started with 0x

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect, P3)

Description

[matxdr]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; m18) Gecko/20001216
BuildID:    2000121620

If the number you enter for the cache size begins with zero, it will change
itself to another value.  010 gives 8, 020 give 16, etc..

Reproducible: Always
Steps to Reproduce:
Go to pref, advanced, cache
Set either the disk or memory cache to 010
Click ok


Actual Results:  Go back to the prefs, the cache size is now 8


Expected Results:  Go back to the prefs, the cache size is still 10

Comment 1

[Doron Rosenberg (IBM)]

seeing on a cvs linux build from today, over to preferences

Comment 2

[Boris Zbarsky [:bzbarsky]]

Numbers entered with a leading 0 are intepreted as octal numbers.
Furthermore, numbers entered with a leading 0x are interpeted as hexadecimal
numbers (0x50 becomes 80).

updating summary.

Comment 3

[Daniel Bratell]

That is more of a feature than a bug, even though octal numbers can be 
confusing. I guess that it is the same for every text field interpreted as a 
number from JavaScript.

Comment 4

[sairuh (rarely reading bugmail)]

should this go to networking: cache...?

Comment 5

[Gagan]

cache bugs to gordon

Comment 6

[gordon]

This works as designed.  Marking bug INVALID.

Comment 7

[Mike Shaver (:shaver emeritus)]

Gordon: you really _designed_ the cache pref to be interpreted as octal when it
begins with a leading 0?  If so, I think your design is flawed.

I'm reopening this, because I think it's confusing to users without any real
value added in return for that confusion, and I can't find another browser that
behaves this way.  If you don't want to fix it, please reassign it to me.

Comment 8

[Matthew T (active 1999-2002)]

The fact that this field lets you enter the `x' character at all is bug 66163.

Comment 9

[gordon]

I didn't design it.  It seems an intentional decision in the way js prefs are 
read.  However, if it is desired to have additional verification of user input, 
that's fine.

Matt, I imaging we're already doing these kind of checks elsewhere.  Could point 
me at a good example, or would you like to take this bug?  It probably should be 
a pref panel bug; it doesn't have much to do with the networking/cache code.  
What do you think?

Comment 10

[gordon]

Chris, this seems to be a general phenomenon with preferences.

Comment 11

[Chris McAfee]

over to bnesse for a look.

Comment 12

[benc]

is gordon going to qa this?

Comment 13

[gordon]

I don't feel a strong need to.  Feel free to take the QA Contact.

Comment 14

[benc]

qa to me. 

Comment 15

[gordon]

An example of this bug can be found at:

http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/
content/pref-cache.xul

Comment 16

[Brian Nesse (gone)]

The backend is storing the value it receives (in this case 8) which is why it 
shows up as 8 when it gets reloaded from the preference. The value has already 
been converted to octal before being passed to the backend preferences.

Passing the buck to frontend preferences for further triage :)

Comment 17

[eddyk]

I've found a call in nsPrefWindow.js:225 where we call parseInt(value) where 
value is the content of the textbox.  If I change the value to 
parseInt(value,10) we'll always interpret in base ten.  Is this what we really 
want in *all* cases?

If so, I'll need r and sr to check this in.

Comment 18

[eddyk]

Attachment: fix

Comment 19

[Alec Flett]

I think this is what we want in all cases... if someone at some point needs a
different base, we can extend the prefwindow code to support that.

in any case, what happens if I enter "alskdjf" instead of a number? does the
pref get reset to 0? does an exception get thrown? We should try to catch the
error and not reset the pref, if at all possible.

Comment 20

[eddyk]

I typed some random ascii garbage into the cache textbox and saved it.  Upon 
re-opening, it was set to 0.  I'll see what can be done to ignore garbage and 
reload the previous value.

Comment 21

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 22

[rcummins]

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

Comment 23

[rcummins]

I feel it important to emphasize, that this bug affects not only cache sizes,
but all preference fields that expect a numeric value; perhaps the summary of
this bug should be changed to reflect that.  According to comment #18 there was
a fix in May 2001 but it never got reviewed and/or checked in(?)

Comment 24

[Peter Trudelle]

->ben/moz1.0/critical, this is data loss folks, but an unlikely scenario so P3.

Comment 25

[Peter Trudelle]

nsbeta1- per Nav triage team

Comment 26

[benc]

Changed summary to reflect real problem.

This has been reported to affect cache and proxy users.

In fact, one user reported they got the wrong proxy because of this.

+nsbeta1 again.

Comment 27

[benc]

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

Comment 28

[rcummins]

Re: comment 19 - why even let the user type a non-numeric key in a field where 
only a numeric value is expected; NC 4.8 doesn't allow it, neither should 
Mozilla.  It would seem to me that the fix in comment 18 in conjunction with 
keystroke-checking in the appropriate preference fields would allow this bug to 
be resolved...

Comment 29

[Peter Trudelle]

I agree.  Support for octal and hex is absurd here, and only makes support for
decimal seem broken.

Comment 30

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 31

[Andreas Kunz]

Attachment: patch with validity check

This patch is the one by eddyk extended by a check if the value string contains
any non-digit/non-whitespace chars. In this case it re-sets the value string to
the old/current pref value to avoid changing the pref.

This is pretty restrictive: any letter in the field will prevent changing the
pref. I included whitespace in the valid chars because a leading/trailing space
should not make changing the pref fail. Whitespace inmidst the string will end
parsing of the int (as it always did), saving " 6 7 " as "6".

Comment 32

[Andreas Kunz]

Attachment: first fix updated

However, the simplest solutions mostly are the best ones: this is the first
patch by eddyk, updated to current trunk.
Comment #19 and #20 suggest a problem when entering nonsensical values (i.e.:
no numbers). However, this is identical to the current behaviour:
"3e4" -> "3"
"xyz" -> "0"
etc. So I think this patch does clearly improve the situation without making
anything worse.

Comment 33

[Andreas Kunz]

Comment on attachment 140438 [details] [diff] [review]
first fix updated

...therefore requesting r= from Neil for the updated version of eddyk's patch.

Comment 34

[Andreas Kunz]

Another error that's a previously not found part of this bug: 08 and 09 are
parsed as octal numbers - where 8 and 9 are invalid digits. So entering those
will result in setting the pref to 0 (as will entering 08346 etc.).

Comment 35

[Andreas Kunz]

Comment on attachment 140438 [details] [diff] [review]
first fix updated

Requesting sr= from alecf.

Comment 36

[Alec Flett]

Comment on attachment 140438 [details] [diff] [review]
first fix updated

sr=alecf

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

last attachment checked in.
Checking in xpfe/components/prefwindow/resources/content/nsPrefWindow.js;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/nsPrefWindow.js,v
 <--  nsPrefWindow.js
new revision: 1.46; previous revision: 1.45
done

Comment 38

[Boris Zbarsky [:bzbarsky]]

This checkin may have caused bug 233167 

Comment 39

[Andreas Kunz]

Second patch was checked in instead of the third one. Biesi will correct this
later today. Reopening to not forget it.

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

wrong patch backed out, right patch checked in. marking fixed again.

sorry for the screwup :(