Parser problem with elements like r="0.2" (locale related)

RESOLVED FIXED

Status

()

Core
SVG
--
critical
RESOLVED FIXED
17 years ago
4 years ago

People

(Reporter: Daniele Nicolodi, Assigned: Daniele Nicolodi)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

17 years ago
I noticed xbl parser problems whit tag that contain attributes like r="0.2".

In a more general form problems are related to attributes that have a value that
start whith "0."
You can verify this problem (if you have a build whith new SVG code included)
seeing the differences beetwen

http://www.croczilla.com/svg/xbl-shapes.xml

and

http://www.grinta.net/svg/examples/shapes.xml

the only relevant difference is in line 142 of the xbl bindings file: in
the first there is 

<svg:circle r="0.2cm" style="fill:red" />

in the second 

<svg:circle r="5" style="fill:red" />

The second work fine, the first don't.
(Assignee)

Comment 1

17 years ago
Strange: this problem is present only in one of two environments where i tested
it. The two computer run the same linux distribution (debian 2.2r3 + ximian
gnome) and the mozilla build is the same but in one the problem is present and
in one isn't. I checked all the library  used by mozilla and all is exatly the
same in two environments. I don't know.
(Assignee)

Updated

17 years ago
Summary: XBL parser problem whith element like r="0.2" → XBL parser problem whith elements like r="0.2"
What's the result of running 'locale' on each machine?
/me clutches at straws.
(Assignee)

Comment 3

17 years ago
You are right !!

On one maschine locale is "C" and all works fine on the other locale is set to
"it_IT" and there is the problem. If i set locale to C the problem go away.

Immeditly workaround for this bug is add 'export LANG=C' to mozilla/run-mozilla.sh
... and I'll guess that in italy, 0.2 is written as 0,2?

What about if you set LC_NUMERIC, rather than LANG? Can you add some printfs to 
the code which does the parsing, and see what the value is just before its 
parsed?

Can you try a non-svg test case (eg XUL, or preferably html)? I can't remember 
if the SVG code does its own parsing (no tree here), but this may be a generic 
style system thing.

Confirming based on screenshots in npm.svg.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: XBL parser problem whith elements like r="0.2" → Parser problem with elements like r="0.2"
(Assignee)

Comment 5

17 years ago
Yes, in italian the decimal separator is ","
I tried whith only LC_NUMERIC set to C and all work fine.
The parsing for SVG is maded by XML parser so i think this is not only a SVG bug.
I don't know well XBL and also Mozilla parser so for other testcases plase wait.
I thought we did something special to cope with the fact that svg can have 
unitless numbers?

Anyway, -> style system. I wonder if we're using *scanf, which is taking the 
locale into account.
Assignee: hyatt → dbaron
Component: XBL → Style System
QA Contact: jrgm → ian
Summary: Parser problem with elements like r="0.2" → Parser problem with elements like r="0.2" (locale related)
(Assignee)

Comment 7

17 years ago
In plain SVG decimal numbers work fine see
http://www.grinta.net/svg/examples/02.xml

The strange thing is that problems whith XBL + SVG is only for "0." strings and
not for other decimal numbers. I'm making some more testcases.
QA Contact: ian → jrgm
Summary: Parser problem with elements like r="0.2" (locale related) → Parser problem with elements like r="0.2"
QA Contact: jrgm → ian
Summary: Parser problem with elements like r="0.2" → Parser problem with elements like r="0.2" (locale related)
bbaetz:  What in the SVG code parses that?  Can you give a testcase that doesn't
involve SVG?
dbaron: Not without net access at home, unfortunately, and I can't point to a
code bit (on the branch) without that, either. At least not til the middle of
next week.
Oh, didn't know that. lxr won't search on it, though, and I don't have a tree
here. So unless someone can come up with a test case (and I can verify this at
home), then this will have to wait til later next week.
(Assignee)

Comment 12

17 years ago
I'm trying to make a testcase.
Someone know an element (XHTML, XUL, or other) that accept decimal argument ??
The only parsing code that I can find is nsSVGLength::SetValueAsString.  The SVG
code seems like a maze of indirection to me, so I can't really tell what's being
used.  So back to SVG, at least until someone can point to style system code
that's the problem.
Assignee: dbaron → dean.jackson
Component: Style System → SVG
QA Contact: ian → dean.jackson
It is a maze of indirection :) I'll try to come up with a test case later next
week.

Comment 15

17 years ago
This looks indeed like an SVG bug.
In content/svg/content/src/nsSVGLength.cpp, nsSVGLength::SetValueAsString(), we 
parse numbers using PR_strtod(), which seems to be locale-sensitive.
We also use PR_strtod() in a few other places in the SVG code, and I guess 
they'll all need to be replaced by something suitable.
(Assignee)

Comment 16

17 years ago
Documentationd and source code of PR_strtod() say that this function is _not_
locale sensitive. The problem is not here. 

Comment 17

17 years ago
I think I've got it now:
The problem is not PR_strtod(), but the sprintf() in 
nsSVGLength::GetValueAsString(). When XBL clones the anonymous content (in this 
case an nsSVGCircleElement), nsSVGAttributes::CopyAttributes() is called in the 
process. This function calls GetValueString() on all source attribs and then 
sets the destination attribs by calling SetAttr() and passing the string value. 
(Admittedly a stupid implementation, but it should work of course.)
So on your Italian machine, sprintf() outputs a comma as separator, which 
PR_strtod doesn't recognise because it has '.' hardcoded.
(Assignee)

Comment 18

17 years ago
OK i have add a printf and i have found that the problem is here.
I have try whith PR_snprintf() but it's the same.
One solution could be:

#define SVGsprintf(b,x...)                \
{                                         \
  char * oldlocale;                       \
  oldlocale = setlocale(LC_NUMERIC, "C"); \
  sprintf(b,x);                           \
  setlocale(LC_NUMERIC, oldlocale);       \
}

but is only a hack.
(Assignee)

Comment 19

17 years ago
Created attachment 49500 [details] [diff] [review]
Proposed patch whith use of setlocale. I think we can have better solution.
(Assignee)

Comment 20

17 years ago
This patch use setlocale() to change LC_NUMERIC to C before use of NS_smprintf()
to print the string and to revert LC_NUMERIC to original value after the print.
I haven't made a function because this is the only occurence of *printf().

I don't know if this code is portable on non *NIX platforms.
No, its not portable. We could use the mozilla collation stuff, but that would
be a lot of overhead.

We really should write our own itoa routine, or grab one of the ones floating in
the tree somewhere and place it in nsCRT, or something.
(Assignee)

Comment 22

17 years ago
I'm grepping all the tree locking for something like an ltoa (long to string)
function if i'll not found it i'll write a simple ltoa function to place into
nsCRT:: (i think to take it from K&R).

Comment 23

17 years ago
setlocale() is ANSI, so it should be ok. Also it's been used before in Mozilla 
for the same purpose (in 
http://lxr.mozilla.org/seamonkey/source/gfx/src/ps/nsPostScriptObj.cpp#60).

We've also got 3 more faulty instances of sprintf() that need fixing
(content/svg/content/src/nsSVGPathSeg.cpp, nsSVGTransform.cpp and 
nsSVGPointList.cpp).
The postscript stuff is only used on unix, though...

Are there any non-svg bits which fail a test:

foo.setAttribute('bar',foo.getAttribute('bar')); ?

Comment 25

17 years ago
I think all the rest of Mozilla just stores attributes as strings, so that test 
probably wouldn't fail anywhere elese. 
In the SVG code we store attributes' values in their appropriate types and only 
in the generic case as strings. 
(Assignee)

Comment 26

17 years ago
Created attachment 49510 [details] [diff] [review]
New path that use macros and ifdefs.
(Assignee)

Comment 27

17 years ago
This problem there is also under other platforms than *NIX ??
If no this could be the solution.

How work localization (and *printf) under Win, Mac and OS/2 ??
(Assignee)

Comment 28

17 years ago
Hack propose:

char buf[80];
char * i;
sprintf(buf, ...);
i = strchr(buf, ',');
if (i) *i = '.';

is not the best but should work.

Comment 29

17 years ago
I'd be inclined to go with the first patch. VC++ implements the same 
localisation mechanism as Unix. Earlier versions of the VC++ lib only 
implemented the 'C'-locale. Daniele's proposed patch would even be compatible 
with that. I'm pretty sure it'll work on all platforms. I'll check the Mac just 
to be sure. Searching and replacing ',' is nasty (e.g. what if there's some 
locale where the separator is something other than ','?). As for coding our own 
locale-insensitive sprintf clone, I think this is an issue that should be 
addressed in a wider context than just SVG. As Bradley says - we should really 
put something into nsCRT or NSPR. But something that's not #ifdef MOZ_SVG'ed. 
Maybe PR_smprintf() should be locale insensitive?
For the short term I think we should just use the locale-setting hack. Any 
objections/thoughts/better ideas?
(Assignee)

Comment 30

17 years ago
OK form me. Attached a better patch.

If someone is interested in i have found "dietlibc" (on freshmeat) that contain
a great dtostr() that can be a good starting point for an our implementation.
(Assignee)

Comment 31

17 years ago
Created attachment 49525 [details] [diff] [review]
Improved path. Tomorrow patch for other occurrences of sprintf in SVG code.

Comment 32

17 years ago
I just checked the standard C library impl on Mac, and setlocale() is basically
a  no-op (not quite, but for most purposes). Suffice to say that the C-library
is allways in the C locale under MacOS as far as I can tell. So any locale based
fix should have zero impact from the Mac side.
(Assignee)

Comment 33

17 years ago
I'm paching other files. Only a question: do you prefer to use statically
allocated buffers whith PR_snprintf() or dinamically allocated buffers whith
PR_smprintf() and PR_smprintf_free() ??
I'm using PR_smprintf() but if you don't like that i use someother function.
Use sprintf is bad for the possibility af buffers oferflow.
(Assignee)

Comment 34

17 years ago
Created attachment 49575 [details] [diff] [review]
Comprensive patch for all occurrences of sprinf.
I get net access this week; I'll come up with an itoa.

Note that nsString::AppendFloat uses printf, so that won't help.

Comment 36

17 years ago
The setlocale() way of ensuring the correct locale seems to be standard usage.
Even glib uses setlocale() in the way Daniele suggests.
(http://lxr.mozilla.org/seamonkey/source/xpcom/typelib/xpidl/glib/glib-﷒0﷓)
I'm quite convinced now that sprintf in conjunction with setlocale() is safe & 
sound on all platforms. The overhead should be negligible.
I'd prefer to use sprintf() over itoa()/ftoa(), especially in places like 
nsSVGPathSegArcRel::GetValueString(), where our format string looks like this:
 sprintf(buf, "a%g,%g %g %d,%d %g,%g", mR1, mR2,
          mAngle, mLargeArcFlag, mSweepFlag, mX, mY);

Do you really think ftoa() would be better? Instead, how about changing 
PR_smprintf() to  C-locale to make it consistent with other PR functions like 
PR_strtod()?
(Assignee)

Comment 37

17 years ago
Now that we know that setlocale() solution to *printf() problem is in good,
there is someone that can checkin my patch ?? There is someone that think that
there is a better solution ??
Keywords: patch
(Assignee)

Comment 38

17 years ago
Created attachment 51455 [details] [diff] [review]
New patch that uses nsTextFormatter::smprintf
(Assignee)

Comment 39

17 years ago
My last patch is not valid. nsTextFormatter is locale sensitive :(
(Assignee)

Comment 40

17 years ago
Last patch works whith patch for #102690.
Add dependency on #102690 (that is waiting only review)
Depends on: 102690

Updated

17 years ago
No longer depends on: 102690
(Assignee)

Comment 41

16 years ago
Patch for bug #102690 is nopw checked in so my last patch (attachment id=51455)
is now OK. Someone can review and checkin ?? 

The approach of setting svg values by string -> value (DOM) string -> value
conversion is still bad but now it works well on any locale.

I was going to check this in last night, but the tree was in flames, and since
i'm packing up my computer, I'm not checking in today.

r=bbaetz on the patch, but I still want to know why we're hitting this
conversion in this case.
Assignee: dean.jackson → daniele
QA Contact: dean.jackson → bbaetz

Comment 43

16 years ago
My guess what happens here is that XBL makes a copy of the SVG node to build
anonymous content:

817 content->GetAttr(namespaceID, name, value2);
http://lxr.mozilla.org/mozilla/source/content/xbl/src/nsXBLBinding.cpp#817

just guessing only though, I'm no XBL expert.
how about using PR_dtoa, we use that in transformiix and it seems to work fine.
(Assignee)

Comment 45

16 years ago
Created attachment 63844 [details] [diff] [review]
New patch that use snprintf() instead of smprintf()
Attachment #49500 - Attachment is obsolete: true
Attachment #49510 - Attachment is obsolete: true
Attachment #49525 - Attachment is obsolete: true
Attachment #49575 - Attachment is obsolete: true
Attachment #51455 - Attachment is obsolete: true

Comment 46

16 years ago
You'll have to change all the occurences of 'sizeof(buf)' to 
'sizeof(buf)/sizeof(PRUnichar)' to get the length of the buffer.
r=afri with that modification.
(Assignee)

Comment 47

16 years ago
Created attachment 63996 [details] [diff] [review]
sizeof(buf) -> sizeof(buf)/sizeof(PRUnichar)
Attachment #63844 - Attachment is obsolete: true
(Assignee)

Comment 48

16 years ago
Created attachment 64167 [details] [diff] [review]
the right one
Attachment #63996 - Attachment is obsolete: true

Comment 49

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.