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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Alice0775 White]

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

Comment 1

[Alice0775 White]

Attachment: 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

Comment 2

[Makoto Kato [:m_kato]]

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

Comment 3

[Makoto Kato [:m_kato]]

humm, mIMC is valid...

Comment 4

[Makoto Kato [:m_kato]]

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...

Comment 5

[Makoto Kato [:m_kato]]

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?

Comment 6

[Makoto Kato [:m_kato]]

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...

Comment 7

[Masatoshi Kimura [:emk]]

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?

Comment 8

[Makoto Kato [:m_kato]]

(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.

Comment 9

[Makoto Kato [:m_kato]]

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 10

[Makoto Kato [:m_kato]]

Attachment: Don't use GetModuleHandleA on RtlImageNtHeader

Comment 11

[Makoto Kato [:m_kato]]

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.

Comment 12

[Masatoshi Kimura [:emk]]

Are widget/windows changes needed?

Comment 13

[Makoto Kato [:m_kato]]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> Are widget/windows changes needed?

I think it is unnecessary.  This is API hook issue.

Comment 14

[Masatoshi Kimura [:emk]]

It would be better to remove unnecessary changes from the patch before requesting a review.

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8571893 [details] [diff] [review]
Don't use GetModuleHandleA on RtlImageNtHeader

oops, this patch has garbage.

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Don't use GetModuleHandleA on RtlImageNtHeader v1.1

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.

Comment 17

[(Away)]

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?

Comment 18

[(Away)]

Also could you test that the IME still works with bug 1137609? (I hope GetProcAddress isn't breaking anything)

Comment 19

[(Away)]

[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

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, can you also request uplift once we have a fix? Thanks David!

Comment 21

[(Away)]

> (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)

Comment 22

[(Away)]

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.

Comment 23

[(Away)]

Got OK from KWierso to land this for you on the closed tree.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1134d7e0dab

Comment 24

[(Away)]

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

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Noting for Sylvestre that this needs to get uplifted along with bug 1137609 (once it looks ok on mozilla-central)

Comment 26

[Makoto Kato [:m_kato]]

(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.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1134d7e0dab

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/611f37fccce0

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75b472026467

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84a2cfba8deb

Comment 32

[(Away)]

Thank you :emk and :m_kato for the debugging and patch. Sorry for the trouble.

Comment 33

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 34

[Frederic GRAVIER]

(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