1091594 - Changing the download folder crashes Firefox on Windows XP when compiled with mingw-w64

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Georg Koppen]

On Windows XP changing the download folder crashes Firefox when it is compiled with mingw-w64. According to a cypherpunk the problematic code is https://mxr.mozilla.org/mozilla-esr31/source/widget/windows/nsFilePicker.cpp#545

What happens is this (according to him/her):

[quote]
Problem with ​code:

browserInfo.lParam = (LPARAM) aInitialDir.get();

But compiler generates instructions:

.6A76D41C: 833F00                 cmp         d,[edi],0
.6A76D41F: 0F95C0                 setnz       al
.6A76D422: 0FB6C0                 movzx       eax,al
.6A76D425: 89442438               mov         [esp][038],eax

Which equal to:
browserInfo.lParam = *(DWORD *)aInitialDir != 0;

So browserInfo.lParam can be 0 or 1 only, while internally it used as pointer to unicode string.

Correct instructions from vanilla Firefox 31ESR generated by MSVC for the same code:

.10C34DE6: 8B450C                   mov         eax,[ebp][00C]
.10C34DE9: 8B00                     mov         eax,[eax]
.10C34DEB: 8945F0                   mov         [ebp][-010],eax
[/quote]

The original bug report is https://bugs.torproject.org/13558.

Comment 1

[Jacek Caban]

Attachment: patch.diff

It seems to be the problem with char16ptr_t (get() doesn't really return a pointer, but char16ptr_t instead on mingw to work around char16_t != wchar_t problem). Please try the attached patch.

Comment 2

[Jacek Caban]

Attachment: patch.diff

This vesion of the patch adds more explicit casts to different integer types. While I was at this, it also adds more operator+ variants to fix some warnings about ambiguous operators.

Comment 3

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c16882c6c05

Comment 4

[Georg Koppen]

(In reply to Jacek Caban from comment #1)
> Created attachment 8514256 [details] [diff] [review]
> patch.diff
> 
> It seems to be the problem with char16ptr_t (get() doesn't really return a
> pointer, but char16ptr_t instead on mingw to work around char16_t != wchar_t
> problem). Please try the attached patch.

Looking at https://trac.torproject.org/projects/tor/ticket/13558#comment:11 it seems the bad code is still generated. I used the attached patch for ESR31.

Comment 5

[Georg Koppen]

Attachment: mingwcrash.patch -- ESR 31

Comment 6

[Georg Koppen]

(In reply to Georg Koppen from comment #4)
> (In reply to Jacek Caban from comment #1)
> > Created attachment 8514256 [details] [diff] [review]
> > patch.diff
> > 
> > It seems to be the problem with char16ptr_t (get() doesn't really return a
> > pointer, but char16ptr_t instead on mingw to work around char16_t != wchar_t
> > problem). Please try the attached patch.
> 
> Looking at https://trac.torproject.org/projects/tor/ticket/13558#comment:11
> it seems the bad code is still generated. I used the attached patch for
> ESR31.

Okay, we have a user that finally tested the patch I attached and it is still crashing: https://lists.torproject.org/pipermail/tor-qa/2014-November/000505.html

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c16882c6c05