Crash on launch if %LANG% not C or POSIX (Windows, trunk)

Bug #1666314 reported by su_v
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Fix Released
Undecided
Patrick Storz

Bug Description

Inkscape trunk crashes on launch on Windows if the current environment defines LANG to any (valid) locale but C or POSIX.

Steps to reproduce:
In Command Prompt window, change to inkscape's install folder:

C:\path\to\inkscape>set LANG=C
C:\path\to\inkscape>inkscape

--> inkscape launches as expected

C:\path\to\inkscape>set LANG=POSIX
C:\path\to\inkscape>inkscape

--> inkscape launches as expected

C:\path\to\inkscape>set LANG=en_US
C:\path\to\inkscape>inkscape

--> inkscape crashes on launch

C:\path\to\inkscape>set LANG=de_CH
C:\path\to\inkscape>inkscape

--> inkscape crashes on launch

Console message (without gdb):
terminate called after throwing an instance of 'std::runtime_error'
  what(): locale::facet::_S_create_c_locale name not valid

Notes:
* The crash is for example exposed if inkscape is launched with GUI (e.g. for verbs) via python-based extensions which use a subclass of inkex.Effect() (bug #1662531 , bug #1663697, bug #1663585), because inkex.localize() sets LANG in os.environ on Windows (bug #1666108).
* Not reproduced with Inkscape 0.91, 0.92.0, 0.92.1 (32bit, 64bit, installed from 7z to custom location): no crash on launch, inkscape sets the GUI language according to LANG as defined in the environment.
* Based on earlier bisecting by Alvin Penner, this regression seems to have been exposed with the merge of the doc-rotate branch (lp:inkscape r15444):
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15444

Reproduced with Inkscape 0.92+devel 15534 on Windows 10 (local build).

Tags: crash ui win64
Revision history for this message
su_v (suv-lp) wrote :
su_v (suv-lp)
description: updated
Revision history for this message
su_v (suv-lp) wrote :

Just came across this recent message on one of GCC's mailing lists (no replies yet):
https://gcc.gnu.org/ml/gcc-help/2017-02/msg00080.html

Similarly, there is a known issue with libstdc++ on (older) versions of Mac OS X (before the switch to clang and libc++ as default): C++ locale only supports C, POSIX
https://mail.gnome.org/archives/gtkmm-list/2010-April/msg00030.html

Revision history for this message
Alvin Penner (apenner) wrote :

crashes confirmed on Windows 10, Inkscape 0.92+devel 15524 custom

just for curiosity do you get a crash if you set LANG = null
using a command like SET LANG =
?

Changed in inkscape:
status: New → Confirmed
Revision history for this message
Patrick Storz (ede123) wrote :

Confirmed with Inkscape 0.92+devel 15530 (devlibs 64) and 0.92+devel 15513 (MSYS2 build)

Revision history for this message
su_v (suv-lp) wrote : Re: [Bug 1666314] Re: Crash on launch if %LANG% not C or POSIX (Windows, trunk)

On 2017-02-20 22:09 (+0100), Alvin Penner wrote:
> just for curiosity do you get a crash if you set LANG = null
> using a command like SET LANG =

No crash with 'set LANG=' (in bash, the same command would be 'unset
LANG'). If LANG is unset, then there is no crash (presumably, that's the
default situation on Windows).

Revision history for this message
Patrick Storz (ede123) wrote :

Crash is in function "sp_dtw_rotation_output()" in "src/widgets/desktop-widget.cpp".

Alvin's bisecting was correct, specifically it was introduced in
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15142.1.24

Subscribing Jabier, as he probably knows what he wanted to achieve with this specific commit...

I hope we can do without locale conversions at this point. Shouldn't be necessary thanks to the UTF8-nature of GTK?

Revision history for this message
Patrick Storz (ede123) wrote :

Reverting the commit works just fine for me (German locale).

<off-bug-topic>
To be honest I also don't understand the conditional introduced in the linked commit:
If (new_scrolled == new_typed) then we can just as well set "*new_val = new_typed;" instead of "*new_val = new_scrolled;". No conditional needed there...
</off-bug-topic>

Revision history for this message
Patrick Storz (ede123) wrote :

Ah, OK, problem is that without the code I can't set a value of e.g. "10.5" as my system expects a comma as decimal separator (i.e. "10,5").

@Jabier: Couldn't we implement the current GtkSpinButton as "Inkscape::UI::Widget::SpinButton()"? It would take care of all these issues (and even adding functionality like allowing to enter simple arithmetic expressions).

Revision history for this message
Patrick Storz (ede123) wrote :

It seems that in fact the "std::locale()" is simply not supported by gcc, see [1]. It might work on *nix [2] but other OSs are out of luck.

As you can see in [1] the function "locale::facet::_S_create_c_locale()" throws for all values except "C".

[1] https://github.com/gcc-mirror/gcc/blob/b7937c548fe1cbe1ea562a2ba6f8c11d20691317/libstdc%2B%2B-v3/config/locale/generic/c_locale.cc#L220
[2] https://github.com/gcc-mirror/gcc/blob/HEAD/libstdc%2B%2B-v3/config/locale/gnu/c_locale.cc#L132

Revision history for this message
Patrick Storz (ede123) wrote :

Suggested fix for this issue (not pretty but it works).

@su_v: Did I use the correct degree symbol?

Revision history for this message
Jabiertxof (jabiertxof) wrote :

Thanks Eduard. For what is this part of the patch?

gchar *comma = g_strstr_len (b, -1, ",");
    if (comma) {
        *comma = '.';
     }

Revision history for this message
Patrick Storz (ede123) wrote :

It replaces comma with dot (e.g. if you enter "1,5" it's converted to "1.5").

Then the locale is set to "C" (which is using dots as decimal separators) and the value converted to a double which is then used to update the value of the SpinButton.

This way it does not matter what locale Inkscape is using, one can always use both - commas and dots - as decimal separator when typing a value.

Revision history for this message
Jabiertxof (jabiertxof) wrote :

I dont know I cann substitute in this way. Cool. comma coulden't be free?

Revision history for this message
Patrick Storz (ede123) wrote :

It should be freed from what I understand: "comma" points to a single gchar in the gchar-array "b". When b is freed, "comma" should be freed, too.

Revision history for this message
Jabiertxof (jabiertxof) wrote :

Thanks Edward. If finaly apply the patch to trunk ping me to apply also in measure-line-lpe

Revision history for this message
Patrick Storz (ede123) wrote :
Changed in inkscape:
assignee: nobody → Eduard Braun (eduard-braun2)
status: Confirmed → Fix Committed
Revision history for this message
Patrick Storz (ede123) wrote :

@Jabiertxof: Here's a patch we could apply against "lpe-measure-line.cpp" to avoid the occurrences of "std::locale("")".

It'd be nicer to be able to use std:: functions and std::strings, but I wasn't able to find a way to achieve locale-dependent string formatting with the incomplete state libstdc++ is in.

jazzynico (jazzynico)
Changed in inkscape:
status: Fix Committed → Fix Released
Revision history for this message
Jabiertxof (jabiertxof) wrote :

Thanks for fixing it!

Revision history for this message
Jabiertxof (jabiertxof) wrote :

And thanks for the measure line patch! I try tonight and commit

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.