Closed Bug 1091594 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Win32, defect)

31 Branch
x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: gk, Assigned: jacek)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch patch.diff (obsolete) — Splinter Review
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.
Assignee: nobody → jacek
Attached patch patch.diffSplinter Review
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.
Attachment #8514256 - Attachment is obsolete: true
Attachment #8514919 - Flags: review?(jmathies)
Attachment #8514919 - Flags: review?(jmathies) → review+
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/9c16882c6c05
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.