131867 - Our use of setlocale() is not threadsafe

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Roland Mainz]

[per jkeisers request in bug 131831]
We have some code in the tree (like in the PostScript module, see
http://lxr.mozilla.org/seamonkey/source/gfx/src/ps/nsPostScriptObj.cpp#76) which
calls setlocale(LC_NUMERIC, "C") to force POSIX-behaviour of sscanf() and other
functions and then immediately restore the locale to the previous value.

Problem is that this use of setlocale() to set and restore the locale settings
effects all threads and not only the calling one...

Comment 1

[Boris Zbarsky [:bzbarsky]]

To printing.  This should not be languishing in browser-general.

Perhaps the right solution is to use the locale service or the NSPR equivalents
of sscanf and the like?

Comment 2

[rods (gone)]

over top don

Comment 3

[rods (gone)]

Don, this is from the printing code, here is how you get the locale:
    nsresult rv;
    nsCOMPtr<nsILocale> locale; 
    nsCOMPtr<nsILocaleService> localeSvc = do_GetService(kLocaleServiceCID, &rv);
    if (NS_SUCCEEDED(rv)) {
      rv = localeSvc->GetApplicationLocale(getter_AddRefs(locale));
      if (NS_SUCCEEDED(rv) && locale) {

Comment 4

[Kenneth Herron]

Taking. I have a fix for the postscript printing module, which I can upload once
bug 168614 is checked in.

From reading bug 131831 it looks like this bug report is also intended to apply
to use of setlocale() within the xprint module, which is apparently still an
issue; see
<http://lxr.mozilla.org/seamonkey/source/gfx/src/xprintutil/xprintutil.c#718>.
Does somebody want to open a separate bug for that, or should I assign this one
to Roland after the PS fix is checked in?

Comment 5

[Kenneth Herron]

.

Comment 6

[Kenneth Herron]

Attachment: Use a Private string class for floating point numbers

I looked at nsPrintfCString and the NSPR PR_*printf functions, but it appeared
that all of them ultimately used *printf() to format floating point. NSPR also
provides a couple of conversion functions, PR_dtoa() and PR_cnvtf(), but those
just produce the raw digits; additional code is required to format them for
printing.

This patch uses a private subclass of nsAutoCString for floating point numbers.
nsCString::AppendFloat() is locale-insensitive; see
<http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsStringObsolete.cpp#1276>.


I've taken the opportunity to add a function for the postscript "scale"
operator. nsRenderingContextPS was directly printing a "scale" command with
floating-point arguments, and adding a scale() method was cleaner than giving
nsRenderingContextPS access to the private string class.

I've also refactored the curve-drawing routines. nsPostScriptObj contained two
methods for drawing curved lines, ellipse() and arc(), which were almost
identical; ellipse() just hardcoded two numbers that were variables in arc().
I've removed ellipse() and modified its callers to use arc() instead.

Comment 7

[Kenneth Herron]

Comment on attachment 141891 [details] [diff] [review]
Use a Private string class for floating point numbers

Hmm, in the previous comment I should have mentioned this patch only addresses
the postscript printing module.

Biesi, you were involved with bug 131831. Could you review this?

Comment 8

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

Comment on attachment 141891 [details] [diff] [review]
Use a Private string class for floating point numbers

please have someone familiar with strings look at your impl of fpCString, like
dbaron or darin.

For arc and scale: You need to put an @param on each line of parameter
descriptions so that it works correctly

-    fprintf(mPrintContext->prSetup->tmpBody,"%3.2f setgray\n",
greyBrightness);
+    fprintf(mPrintContext->prSetup->tmpBody, "%s setgray\n",

you're losing the 3.2 here... is it not needed?

also in the next fprintf

r=biesi with that

Comment 9

[Kenneth Herron]

Comment on attachment 141891 [details] [diff] [review]
Use a Private string class for floating point numbers

Darin, could you sr this? This patch involves a private subclass of
nsAutoCString.

Regarding the %3.2f question, AppendFloat() always produces up to about six
digits, so exactly reproducing the 3.2 format would be tricky. The arguments to
"setgray" and "setrgbcolor" are just floating-point numbers between 0 and 1,
representing color intensities. Six digits of accuracy may be unnecessarily
precise, but they're valid postscript.

I nobody cares, I'll fix the @param thing after super-review.

Comment 10

[Darin Fisher]

hmm.. is there a module owner for gfx/src/ps that should review this patch?  the
patch looks reasonable to me.  the nsCAutoString subclass is simple enough.  i
don't see any issues with that.

despot (http://www.mozilla.org/owners.html) says that tor and bz are co-owners
of this module.  that means that either one of them needs to review this patch
before it can go into the tree.

i'll defer sr= to one of them.

Comment 11

[Kenneth Herron]

Comment on attachment 141891 [details] [diff] [review]
Use a Private string class for floating point numbers

Okay, tor, could you sr this?

Comment 12

[Kenneth Herron]

Checked in.

Checking in nsPostScriptObj.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.109; previous revision: 1.108
done
Checking in nsPostScriptObj.h;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.h,v  <--  nsPostScriptObj.h
new revision: 1.41; previous revision: 1.40
done
Checking in nsRenderingContextPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsRenderingContextPS.cpp,v  <-- 
nsRenderingContextPS.cpp
new revision: 1.71; previous revision: 1.70
done

I never got a responseto comment 4. I'm going to leave this open for the xprint
issue.

Comment 13

[Roland Mainz]

Kenneth Herron (wrote:
> I never got a responseto comment 4. I'm going to leave this open for the 
> xprint issue.

Uhm... I never got a bugzilla email for comment #4 . ;-(
I guess we can ignore that problem for now, I am currently rewriting the
XprintUtils library from scratch (that's
http://xprint.mozdev.org/bugs/show_bug.cgi?id=4870).

I'll mark this bug as FIXED for now.