Integer pref dialog silently accepts non-numbers

RESOLVED FIXED in mozilla1.9alpha6

Status

()

--
minor
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({polish, regression})

unspecified
mozilla1.9alpha6
polish, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
Integer pref dialog silently accepts non-numbers

STEPS TO REPRODUCE
1. Start Firefox/Suiterunner
2. about:config
3. double-click an integer pref
4. delete the current value then type 333hello in the text input box
5. ENTER or click OK

ACTUAL RESULT
value changes to 333

EXPECTED RESULT
Only base-10 numbers should be accepted as input.
This would help avoiding mistyping values as in bug 53098.

PLATFORMS AND BUILDS TESTED
Bug occurs in trunk.
Bug does not occur in xpfe SeaMonkey (displays a warning alert and does
not change the value, so it's a regression for SeaMonkey)
(Assignee)

Comment 1

12 years ago
Created attachment 267035 [details] [diff] [review]
Patch rev. 1

Disable the OK button until there is an acceptable number.
Attachment #267035 - Flags: review?(neil)
(Assignee)

Updated

12 years ago
Blocks: 53098

Comment 2

12 years ago
Comment on attachment 267035 [details] [diff] [review]
Patch rev. 1

>+<dialog id="intValueDialog"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" 
>+	ondialogaccept="return onIntValueDialogOK();"
>+	ondialogcancel="return onIntValueDialogCancel();"
>+        onload="onIntValueDialogLoad();">
Some tabs crept in here. You also forgot to specify the dimensions.

>+    var gIntValueDialogParams;
Are you planning on making this dialog accessible to C++ callers? If not, you should just pass a JS object instead of a dialog param block.

>+      textbox.value = gIntValueDialogParams.GetInt(2);
>+      validateIntValue();
I would be very surprised if GetInt(2) returned an invalid int value...

>+    function onIntValueDialogOK()
>+    {
>+      if (!validateIntValue())
>+        return false;
I would be almost as surprised if you could click OK when it was disabled. (The Enter key also checks that the default button is not disabled.)

>+    function validateIntValue()
>+    {
>+      // We intentionally don't want to support octal/hex numbers here (bug 63117).
>+      var value = document.getElementById("textbox").value;
>+      var intValue = parseInt(value, 10);
>+      var valid = !isNaN(value) && value == intValue.toString();
Hmm... I guess (value | 0) == (value - 0) access hex numbers. But at least it only accepts integers, whereas this code accepts numbers up to 100000000000000000 (which appears to be the limit of double precision). Maybe (value | 0) == parseFloat(value) would be a suitable compromise?

>+      <textbox id="textbox" flex="1" oninput="validateIntValue()"/>
flex="1" is wrong here.

>+  else if (entry.typeCol == nsIPrefBranch.PREF_INT) {
>+    if (!openIntValueDialog(entry, title))
>+      return false;
>+  }
I don't like the way this conceals the pref setting code.

>-      var supportsString = Components.classes[nsSupportsString_CONTRACTID].createInstance(nsISupportsString);
>-      supportsString.data = result.value;
>-      gPrefBranch.setComplexValue(entry.prefCol, nsISupportsString, supportsString);
>-    }
>+    var supportsString = Components.classes[nsSupportsString_CONTRACTID].createInstance(nsISupportsString);
>+    supportsString.data = result.value;
>+    gPrefBranch.setComplexValue(entry.prefCol, nsISupportsString, supportsString);
I assume this is just a whitespace change?
Attachment #267035 - Flags: review?(neil) → review-
(Assignee)

Comment 3

12 years ago
Created attachment 267099 [details] [diff] [review]
Patch rev. 2
Attachment #267035 - Attachment is obsolete: true
(Assignee)

Comment 4

12 years ago
Created attachment 267100 [details] [diff] [review]
Patch rev. 2 (diff -w)

(In reply to comment #2)
> Some tabs crept in here. You also forgot to specify the dimensions.

Fixed.

> >+    var gIntValueDialogParams;
> Are you planning on making this dialog accessible to C++ callers? If not, you
> should just pass a JS object instead of a dialog param block.

Fixed.

> >+    function onIntValueDialogOK()
> >+    {
> >+      if (!validateIntValue())
> >+        return false;
> I would be almost as surprised if you could click OK when it was disabled. (The
> Enter key also checks that the default button is not disabled.)

I'd like to keep it, because we have bugs that would make you surprised ;-)
I have added the following comment:

      // XXX Validate one more time because there are ways to change
      // the text that does not trigger 'oninput' such as drag-n-drop
      // (bug 128066) and autocomplete (bug 320462).

> >+      var value = document.getElementById("textbox").value;
> >+      var intValue = parseInt(value, 10);
> >+      var valid = !isNaN(value) && value == intValue.toString();
> Hmm... I guess (value | 0) == (value - 0) access hex numbers. But at least it
> only accepts integers, whereas this code accepts numbers up to
> 100000000000000000 (which appears to be the limit of double precision). Maybe
> (value | 0) == parseFloat(value) would be a suitable compromise?

I kept the current code and added "(value | 0) == intValue" to limit
the number to the integer range without overflow, [-2147483648 .. 2147483647]

> >+      <textbox id="textbox" flex="1" oninput="validateIntValue()"/>
> flex="1" is wrong here.

Removed.

> I don't like the way this conceals the pref setting code.

Fixed.

> I assume this is just a whitespace change?

Well, yes for the non-integer branch, but for integers the old
code actually sets the value twice - first with setIntPref()
and then again with setComplexValue(), I guessed this
was just a bug and that setIntPref() is enough.
Attachment #267100 - Flags: review?(neil)
(Assignee)

Comment 5

12 years ago
(In reply to comment #4)
> > I assume this is just a whitespace change?

Please disregard my last answer to this question, I misread the code.
Yes, this is just a whitespace change.

Comment 6

12 years ago
Comment on attachment 267100 [details] [diff] [review]
Patch rev. 2 (diff -w)

>+<dialog id="intValueDialog"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" 
>+        ondialogaccept="return onIntValueDialogOK();"
>+        ondialogcancel="return onIntValueDialogCancel();"
>+        onload="onIntValueDialogLoad();"
>+        style="min-width: 29em; min-height: 10em;">
I just noticed you're missing the buttonpack... can you add that too please?

>+      textbox.value = gIntValueDialogParams.value;
>+      validateIntValue();
I worked out why you need to validate this - it's a string, not an int, so the old code was wrong to pass it as such ;-)

>+                   cancelled: false };
I think this should default to true, just in case. r=me with these fixed.
Attachment #267100 - Flags: review?(neil) → review+
(Assignee)

Comment 7

12 years ago
Created attachment 267695 [details] [diff] [review]
nits fixed
(Assignee)

Comment 8

12 years ago
Checked in to trunk at 2007-06-08 02:52 PDT.

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha6

Updated

11 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.