Trying to backport OSX-Cocoa ticket #13504 fix to 2.9.3

[ardi]

Hi,

I'm still at 2.9.3 because I still need to support OSX 10.4 Tiger, and
during these years I applied to 2.9.3 all the fixes to bugs that affected
me. It's been completely rock-solid and stable for me during these years.

However, a couple of years ago, I fixed one of its bugs (failure in
conversion between wxString and ISO 8859-1) by applying the normalize.patch
attachment that can be found at http://trac.wxwidgets.org/ticket/13504 and
unfortunately this patch produces a crash on Mavericks.

So, last month I went back to the ticket and realized this normalize.patch
was actually discarded in the real fix of #13504.

Then I unapplied normalize.patch on my 2.9.3 source, and applied (manually
revised) changesets 71918, 72894, and 72895, which seem to be the real fix
for ticket #13504.

After doing that (and also applying changesets 73414 and 74076 for fixing
potential Mavericks crashes), I completely fixed my Mavericks crashes
(however, 73414 and 74076 weren't enough for fixing the crashes, I also had
to unapply normalize.patch and apply 71918, 72894, and 72895).

The bad news is that now the result of (const
char*)dialog.GetPath().mb_str(wxConvISO8859_1) is only a valid C string if
the path has ASCII chars only. If it has international chars (accents,
etc), the resulting string seems to be wrong, as it crashes when passed to
strlen().

So... what has normalize.patch that fixes this, while changesets 71918,
72894, and 72895 don't fully fix it?

I just checked the latest 3.0.1 release and it works fine, so I'm really
missing some changeset here :-\

Thanks a lot for any clue!!

ardi
PS: I know I could use my tuned 2.9.3 with normalize.patch for Tiger, and
the latest 3.0.1 for newer OSX releases. However, my tuned 2.9.3 also has
custom features I added to it (such as multitouch gestures in OpenGL
views), so I'd like to keep using my 2.9.3 source.

[Vadim Zeitlin]

On Sat, 12 Jul 2014 08:48:36 -0700 (PDT) ardi wrote:

a> I'm still at 2.9.3 because I still need to support OSX 10.4 Tiger,

FWIW I don't think Stefan would actually object if you submitted patches
making 3.0 10.4-compatible, so this could be a more reasonable solution
than staying with this old release.

a> However, a couple of years ago, I fixed one of its bugs (failure in
a> conversion between wxString and ISO 8859-1) by applying the normalize.patch
a> attachment that can be found at http://trac.wxwidgets.org/ticket/13504 and
a> unfortunately this patch produces a crash on Mavericks.
a>
a> So, last month I went back to the ticket and realized this normalize.patch
a> was actually discarded in the real fix of #13504.
a>
a> Then I unapplied normalize.patch on my 2.9.3 source, and applied (manually
a> revised) changesets 71918, 72894, and 72895, which seem to be the real fix
a> for ticket #13504.

AFAIR the idea of our changes was exactly the same, we just did it without
code duplication. So I'm quite surprised that your original code resulted
in crashes. Why do you think they were due to these changes?

a> The bad news is that now the result of (const
a> char*)dialog.GetPath().mb_str(wxConvISO8859_1) is only a valid C string if
a> the path has ASCII chars only. If it has international chars (accents,
a> etc), the resulting string seems to be wrong, as it crashes when passed to
a> strlen().

Looks like conversion fails (but you should really check what does
mb_str() actually return, it's a bit strange to have to guess when you can
just verify it). Are you sure the "international chars" you use are
actually in Latin-1?

Regards,
VZ

[ardi]

Post by Vadim Zeitlin
a> I'm still at 2.9.3 because I still need to support OSX 10.4 Tiger,
[
]

Looks like conversion fails (but you should really check what does

Post by Vadim Zeitlin
mb_str() actually return, it's a bit strange to have to guess when you can
just verify it). Are you sure the "international chars" you use are
actually in Latin-1?

Yes, they're in Latin-1 (just accents, for example).

I tested it, and mb_str(wxConvISO8859_1) returns an empty string (first
char nil) for this path: /Users/ardi/Desktop/víctor
However, it works fine for this path: /Users/ardi/Desktop/victor
Note that the only difference is the accent in victor

Do you've any idea on what source file(s) should I look for changesets
since 2.9.3 for this?

Regarding submitting patches for making wx Tiger-compatible, I'm with such
limited time that I'm considering myself dropping Tiger as well. I don't
wish to do so, so I'm trying to figure out why normalize.patch worked, but
the changesets I applied do not. But if I cannot find some way for fixing
it, I'll drop Tiger and go for 3.0.1 (well, patching it with some
modifications I did over the years and that I need for my apps).

On the other hand, I never got the app resources dir to work with
normalize.patch, I didn't test if it's fixed on 3.0.1 or not.

Thanks!

ardi

[Vadim Zeitlin]

On Wed, 16 Jul 2014 05:51:28 -0700 (PDT) ardi wrote:

a> Yes, they're in Latin-1 (just accents, for example).

This doesn't mean it's in Latin-1.

a> I tested it, and mb_str(wxConvISO8859_1) returns an empty string (first
a> char nil) for this path: /Users/ardi/Desktop/víctor

U00ED (LATIN SMALL LETTER I WITH ACUTE) is in Latin-1, but the paths under
OS X use NFKD, so I strongly suspect you actually don't have it in the
input but have "i" followed by "'" instead. What are the actual bytes of
the string being converted?

a> Do you've any idea on what source file(s) should I look for changesets
a> since 2.9.3 for this?

They are the patches you're looking at, maybe they got misapplied somehow.
The simplest would be to debug the conversion code and check that the
conversion to the NFC is actually performed.

Regards,
VZ

[ardi]

Post by Vadim Zeitlin
a> Yes, they're in Latin-1 (just accents, for example).
This doesn't mean it's in Latin-1.
a> I tested it, and mb_str(wxConvISO8859_1) returns an empty string (first
a> char nil) for this path: /Users/ardi/Desktop/víctor
U00ED (LATIN SMALL LETTER I WITH ACUTE) is in Latin-1, but the paths under
OS X use NFKD, so I strongly suspect you actually don't have it in the
input but have "i" followed by "'" instead. What are the actual bytes of
the string being converted?
a> Do you've any idea on what source file(s) should I look for changesets
a> since 2.9.3 for this?
They are the patches you're looking at, maybe they got misapplied somehow.
The simplest would be to debug the conversion code and check that the
conversion to the NFC is actually performed.

Given that the string seems to be empty, and that
wxCFStringRef::AsStringWithNormalizationFormC() returns wxEmptyString when
ref is NULL, I'm beginning to suspect about the cast between CFStringRef
and NSString* via the wxCFRef template class, done with some magic in
cfstring.cpp

What does the wxOSX_USE_COCOA_OR_IPHONE definition mean in that file?
Shouldn't it be always defined if cfstring.cpp is compiled?

Anyway, I guess if the problem is the cast, it should always fail, but it's
correctly working if the string is already normalized before
calling wxCFStringRef::AsStringWithNormalizationFormC()...

ardi

[Stefan Csomor]

Hi

Given that the string seems to be empty, and that wxCFStringRef::AsStringWithNormalizationFormC() returns wxEmptyString when ref is NULL, I'm beginning to suspect about the cast between CFStringRef and NSString* via the wxCFRef template class, done with some magic in cfstring.cpp

What does the wxOSX_USE_COCOA_OR_IPHONE definition mean in that file? Shouldn't it be always defined if cfstring.cpp is compiled?

CFStringRef and NSString* are toll free bridged types, can be used interchangeably, cfstring.cpp is used also for Carbon, NSString* was used in cocoa or cocoa touch builds only that's why we had the wxOSX_USE_COCOA_OR_IPHONE, now I'm also using it in Carbon / Webkit, therefore in current trunk this #if has gone.

Sooner or later I'll take out the carbon build from trunk anyway.

Best,

Stefan

[ardi]

Found it!! I was missing changeset #75698. Without that bug-fix, paths
won't be normalized at all. That's what was happening to me.
http://trac.wxwidgets.org/changeset/75698

It's working now :-)

Thanks,

ardi