Closed Bug 1138070 Opened 9 years ago Closed 9 years ago

WindowsCrtPatch breaks LoadLibrary(Ex)A (Unable to enable Japanese IME or doesn't load pages) on Windows XP SP2

Categories

(Firefox Build System :: General, defect)

36 Branch
x86
Windows XP
defect
Not set
normal

Tracking

(firefox36+ fixed, firefox37 fixed, firefox38 fixed, firefox39 fixed, firefox-esr31 unaffected, firefox-esr38 ?)

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 + fixed
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- ?

People

(Reporter: alice0775, Assigned: m_kato)

References

Details

(Keywords: inputmethod, jp-critical, regression)

Attachments

(2 files, 1 obsolete file)

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/b94bcbc389e8
Mozilla/5.0 (Windows NT 5.1; rv:39.0) Gecko/20100101 Firefox/39.0 ID:20150228030231

The problem happens since Firefox36.0.
The problem does not happen on Firefox35.0.1.

Firefox36 should be compatible with Windows XP SP2.
See https://www.mozilla.org/en-US/firefox/36.0/system-requirements/

Steps To Reproduce:
1. Setup Windows XP SP2 Professional (Japanese Edition)
2. Launch Firefox36
4. Focus input/textarea of web page or LocationBar/SearchBar of browser UI
5. Attempt to turn on IME (Press 半角/全角)

Actual Results:
Indicator of IME toolbar retain "A".
And cannot enter the Japanese text.

Expected Results:
Indicator of IME toolbar should become "あ".
And should be able to enter the Japanese text

Regression window
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e0c4c804b279&tochange=ea2ea74d0e33
Regressed by:
ea2ea74d0e33	David Major — Bug 1061335 - Part 3: Switch Win32 builders to VS2013. r=glandium sr=bsmedberg
OS: Windows 7 → Windows XP
Attached file fx_imm32.log
IME log

Steps
set NSPR_LOG_MODULES=nsIMM32HandlerWidgets:1
set NSPR_LOG_FILE=c:\fx_imm32.log
firefox.exe
Press 半角/全角 key on input field of about:home
exit
0:000> u
xul!nsIMEContext::AssociateDefaultContext:
010b2d67 56              push    esi
010b2d68 8bf1            mov     esi,ecx
010b2d6a 837e0400        cmp     dword ptr [esi+4],0
010b2d6e 7404            je      xul!nsIMEContext::AssociateDefaultContext+0xd (010b2d74)
010b2d70 32c0            xor     al,al
010b2d72 5e              pop     esi
010b2d73 c3              ret
010b2d74 6a10            push    10h
0:000> r
eax=00d00119 ebx=00000001 ecx=0012e740 edx=7c94eb94 esi=0cb99c00 edi=0cb99c00
eip=010b2d67 esp=0012e734 ebp=0012e748 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000202
xul!nsIMEContext::AssociateDefaultContext:
010b2d67 56              push    esi
0:000> dt xul!nsIMEContext @ecx
   +0x000 mWnd             : 0x000400f4 HWND__
   +0x004 mIMC             : 0x00d00119 HIMC__
...
0:000> p
eax=00d00119 ebx=00000001 ecx=0012e740 edx=7c94eb94 esi=0cb99c00 edi=0cb99c00
eip=010b2d68 esp=0012e730 ebp=0012e748 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000202
xul!nsIMEContext::AssociateDefaultContext+0x1:
010b2d68 8bf1            mov     esi,ecx
0:000> p
eax=00d00119 ebx=00000001 ecx=0012e740 edx=7c94eb94 esi=0012e740 edi=0cb99c00
eip=010b2d6a esp=0012e730 ebp=0012e748 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000202
xul!nsIMEContext::AssociateDefaultContext+0x3:
010b2d6a 837e0400        cmp     dword ptr [esi+4],0  ds:0023:0012e744=00d00119
0:000> p
eax=00d00119 ebx=00000001 ecx=0012e740 edx=7c94eb94 esi=0012e740 edi=0cb99c00
eip=010b2d6e esp=0012e730 ebp=0012e748 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000202
xul!nsIMEContext::AssociateDefaultContext+0x7:
010b2d6e 7404            je      xul!nsIMEContext::AssociateDefaultContext+0xd (010b2d74) [br=0]
0:000> p
eax=00d00119 ebx=00000001 ecx=0012e740 edx=7c94eb94 esi=0012e740 edi=0cb99c00
eip=010b2d70 esp=0012e730 ebp=0012e748 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000202
xul!nsIMEContext::AssociateDefaultContext+0x9:
010b2d70 32c0            xor     al,al
Assignee: nobody → m_kato
humm, mIMC is valid...
Hardware: x86_64 → x86
It seems to be that imjp81k!CIImeGrammar::GetGainenInfo returns error.  Then IME2002 will call ImmSetOpenStatus(FALSE).  So IME statue doesn't change to open.

I continue to inverstigate IME2002...
It is fail to load C:\WINDOWS\IME\IMJP8_1\Dicts\imjpgn.grm in imjp81k!CIImeGrammar::Init at start up (by set input context).  So it cannot find DICSTREAM in resource.  So IME cannot open dictionary correctly, then user cannot input character via IME.

LoadLibraryExA is successful, but image is invalid... why?
I found workaround...

Root cause is that LoadLibraryExA("C:\WINDOWS\IME\IMJP8_1\Dicts\imjpgn.grm", NULL, LOAD_LIBRARY_AS_DATAFILE). If we change 3rd parameter to 0, it works...

This is too strange...
My speculation is that the RtlImageNtHeader patch breaks things.
Can you reproduce the bug on WinXP SP3 when attachment 8571748 [details] [diff] [review] is applied?
(In reply to Masatoshi Kimura [:emk] from comment #7)
> My speculation is that the RtlImageNtHeader patch breaks things.
> Can you reproduce the bug on WinXP SP3 when attachment 8571748 [details] [diff] [review]
> [diff] [review] is applied?

Thanks, emk-san.  I will try this.
On XPSP2, hooked RtlImageNtHeader is always called.  Even if applying bug 1137609, this still occurs.  MapViewOfFileEx maps invalid data...

If we remove this hack, it works.  this is a regression of hooking of RtlImageNtHeader.  GetModuleHandleA override filename of LoadLibraryExW.  I think we should use GetModuleHandleW.

0:000> du 7ffdec00
7ffdec00  "C:\WINDOWS\IME\IMJP8_1\Dicts\imj"
7ffdec40  "pgn.grm"
0:000> ba w1 7ffdec00
0:000> g
Breakpoint 2 hit
eax=0000006d ebx=7ffb0220 ecx=7ffdec00 edx=00419bd8 esi=7ffb001c edi=0000000b
eip=7c94f122 esp=0012d9dc ebp=0012d9e8 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000246
ntdll!RtlMultiByteToUnicodeN+0x245:
7c94f122 41              inc     ecx
0:000> k
ChildEBP RetAddr
0012d9e8 7c94f0aa ntdll!RtlMultiByteToUnicodeN+0x245
0012da10 7c80e2df ntdll!RtlAnsiStringToUnicodeString+0x7d
0012da30 7c80b53c kernel32!Basep8BitStringToStaticUnicodeString+0x3f
0012da3c 00414b00 kernel32!GetModuleHandleA+0x21
0012da50 7c95708f firefox!WindowsCrtPatch::patched_RtlImageNtHeader+0x1c
0012dd04 7c95652e ntdll!LdrpCheckForLoadedDll+0x4cd
0012dd80 7c95659e ntdll!LdrGetDllHandleEx+0x258
0012dd9c 7c801d1f ntdll!LdrGetDllHandle+0x18
0012de04 7c801d6e kernel32!LoadLibraryExW+0x161
0012de18 6495fc83 kernel32!LoadLibraryExA+0x1f
0012df3c 649448f8 imjp81k!CIImeGrammar::Init+0x63
0012df50 64944be1 imjp81k!FLoadGrammarModule+0x38
0012df60 649066e8 imjp81k!KnlInitialize+0xb1
0012df74 4ede9974 imjp81k!CreateIImeKbdInstance+0x8
Comment on attachment 8571893 [details] [diff] [review]
Don't use GetModuleHandleA on RtlImageNtHeader

This is a regression after upgrading VS2013.

When loading IME dictionary by Microsoft IME 2002 on XP SP2, it cannot load well.  Because our hook of RtlImageNtHeader overrides filename of discretionary.

LoadLibraryExA uses common buffer by kernel32's internal API to convert to unicode.  So we should not use ANSI API on RtlImageNtHeader's hook.
Attachment #8571893 - Flags: review?(dmajor)
Are widget/windows changes needed?
(In reply to Masatoshi Kimura [:emk] from comment #12)
> Are widget/windows changes needed?

I think it is unnecessary.  This is API hook issue.
It would be better to remove unnecessary changes from the patch before requesting a review.
Comment on attachment 8571893 [details] [diff] [review]
Don't use GetModuleHandleA on RtlImageNtHeader

oops, this patch has garbage.
Attachment #8571893 - Flags: review?(dmajor)
This is a regression after upgrading VS2013.

When loading IME dictionary by Microsoft IME 2002 on XP SP2, it cannot load well.  Because our hook of RtlImageNtHeader overrides filename of IME 2002's dictionary file.

LoadLibraryExA(filename, NULL, LOAD_LIBRARY_AS_DATAFILE) uses common buffer by kernel32's internal API to convert ANSI filename to unicode.  So GetModuleFilenameA causes filename is overridden.

So we should not use ANSI API on RtlImageNtHeader's hook.
Attachment #8571893 - Attachment is obsolete: true
Attachment #8571959 - Flags: review?(dmajor)
Comment on attachment 8571959 [details] [diff] [review]
Don't use GetModuleHandleA on RtlImageNtHeader v1.1

Review of attachment 8571959 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this! Could you please pick up bug 1137609 from inbound and make the same change to the other GetModuleHandleA?
Attachment #8571959 - Flags: review?(dmajor) → review+
Also could you test that the IME still works with bug 1137609? (I hope GetProcAddress isn't breaking anything)
Flags: needinfo?(m_kato)
[Tracking Requested - why for this release]: 
Requesting 36 tracking because:
* It's a trivial fix for an important regression in 36.0
* It's partly a follow-up to bug 1137609 which will likely land for 36.0.1
OK, can you also request uplift once we have a fix? Thanks David!
Flags: needinfo?(dmajor)
> (I hope GetProcAddress isn't breaking anything)
Actually, this shouldn't be an issue, because we only do the Init stuff once at startup (and we were already using GetProcAddress as part of AddHook)
Ok, nevermind my request. From the stack in comment 9 it's clear that the issue is due to nested conversions, so we don't need to change any of the other GetModuleHandleA's. This patch is good as-is.
Flags: needinfo?(dmajor)
Keywords: checkin-needed
Flags: needinfo?(dmajor)
Got OK from KWierso to land this for you on the closed tree.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1134d7e0dab
Flags: needinfo?(dmajor)
Keywords: checkin-needed
Comment on attachment 8571959 [details] [diff] [review]
Don't use GetModuleHandleA on RtlImageNtHeader v1.1

Approval Request Comment
[Feature/regressing bug #]: bug 1023941
[User impact if declined]: can't use Japanese IME on XPSP2
[Describe test coverage new/current, TreeHerder]: local testing
[Risks and why]: This should be very low risk; GetModuleHandleA was already calling GetModuleHandleW under the hood
[String/UUID change made/needed]: none
Attachment #8571959 - Flags: approval-mozilla-release?
Attachment #8571959 - Flags: approval-mozilla-beta?
Attachment #8571959 - Flags: approval-mozilla-aurora?
Noting for Sylvestre that this needs to get uplifted along with bug 1137609 (once it looks ok on mozilla-central)
(In reply to David Major [:dmajor] (UTC+13) from comment #18)
> Also could you test that the IME still works with bug 1137609? (I hope
> GetProcAddress isn't breaking anything)

When I test using http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1425420644/ (this has all fixes), IME works fine.
Flags: needinfo?(m_kato)
Attachment #8571959 - Flags: approval-mozilla-release?
Attachment #8571959 - Flags: approval-mozilla-release+
Attachment #8571959 - Flags: approval-mozilla-beta?
Attachment #8571959 - Flags: approval-mozilla-beta+
Attachment #8571959 - Flags: approval-mozilla-aurora?
Attachment #8571959 - Flags: approval-mozilla-aurora+
Summary: Unable to enable Japanese IME on Windows XP SP2 → WindowsCrtPatch breaks LoadLibrary(Ex)A (Unable to enable Japanese IME or doesn't load pages) on Windows XP SP2
https://hg.mozilla.org/mozilla-central/rev/f1134d7e0dab
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Thank you :emk and :m_kato for the debugging and patch. Sorry for the trouble.
This seems to work fine now on Windows XP x86 SP2 with Firefox 36.0.1 (build2). I will leave it to people who are more experienced in this area to fully confirm the fix and close this.
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #33)
> This seems to work fine now on Windows XP x86 SP2 with Firefox 36.0.1
> (build2). I will leave it to people who are more experienced in this area to
> fully confirm the fix and close this.

Im confirm its ok for me.
French version  Firefox 36.0.1  md5  6f82cc9c196fff758a37115bd64aef91 Windows XP SP2   OK
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: