Closed Bug 99771 Opened 23 years ago Closed 23 years ago

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

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: daniele, Assigned: daniele)

References

(
URL
)

Details

Attachments

(1 file, 7 obsolete files)

the right one
12.16 KB, patch
Details | Diff | Splinter Review 
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.
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.
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.
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"
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)
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.
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.
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.

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


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.

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.
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.
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).
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')); ?
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. 


    
    

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

    
    

    
  
Hack propose:

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

is not the best but should work.

    
    

    
  
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?


    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
I get net access this week; I'll come up with an itoa.

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

    
    

    
  
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()?


    
    

    
  
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

    
    

    
  


  

    
    

    
  
My last patch is not valid. nsTextFormatter is locale sensitive :(

    
    

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

    
  
No longer depends on: 102690

    
    

    
  
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

    
    

    
  
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.

    
    

    
  


  

        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

    
    

    
  
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.

    
    

    
  


  

        Attachment #63844 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        the right one
        — 
          — Splinter Review
      

    


  

        Attachment #63996 -
        Attachment is obsolete: true

    
    

    
  
checked in
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: