The Chrome behaviour here doesn't seem wrong, just different.

FWIW there are Windows keyboard layouts which assign meanings to physical keys that don't actually exist on physical keyboards for that layout - they tend to behave as duplicates of other keys on that same layout.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bdcfb5a1816f1bad537d7611c7ccb972b550339c

commit bdcfb5a1816f1bad537d7611c7ccb972b550339c
Author: chongz <chongz@chromium.org>
Date: Tue Jun 14 21:41:33 2016

[DomKey] Expose Korean special keys on Korean keyboard layout only

Korean keyboard has special keys "HangulMode" and "HanjaMode", we should
only expose them when the system layout is Korean. Otherwise we should
return "Unidentified".

This matches Firefox and windows virtual key.

Note: This also works with JIS keyboard + Korean layout, because we are
mapping DomKey based on KeyboardCode instead of DomCode.

BUG= 612736 

Review-Url: https://codereview.chromium.org/2046193002
Cr-Commit-Position: refs/heads/master@{#399783}

[modify] https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c/ui/events/keycodes/platform_key_map_win.cc
[modify] https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c/ui/events/keycodes/platform_key_map_win_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bdcfb5a1816f1bad537d7611c7ccb972b550339c

commit bdcfb5a1816f1bad537d7611c7ccb972b550339c
Author: chongz <chongz@chromium.org>
Date: Tue Jun 14 21:41:33 2016

[DomKey] Expose Korean special keys on Korean keyboard layout only

Korean keyboard has special keys "HangulMode" and "HanjaMode", we should
only expose them when the system layout is Korean. Otherwise we should
return "Unidentified".

This matches Firefox and windows virtual key.

Note: This also works with JIS keyboard + Korean layout, because we are
mapping DomKey based on KeyboardCode instead of DomCode.

BUG= 612736 

Review-Url: https://codereview.chromium.org/2046193002
Cr-Commit-Position: refs/heads/master@{#399783}

[modify] https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c/ui/events/keycodes/platform_key_map_win.cc
[modify] https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c/ui/events/keycodes/platform_key_map_win_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9248145f2dca37ab3492980ce884787818223ab

commit a9248145f2dca37ab3492980ce884787818223ab
Author: tommycli <tommycli@chromium.org>
Date: Wed Jun 15 15:56:31 2016

Revert of [DomKey] Expose Korean special keys on Korean keyboard layout only (patchset #2 id:20001 of https://codereview.chromium.org/2046193002/ )

Reason for revert:
Breaks Windows Unit DrMemory tests. Sorry.

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(DrMemory%20full)%20(5)

First breaking build:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(DrMemory%20full)%20(5)

Error:

UNADDRESSABLE ACCESS: reading 0x00001d2c-0x00001d30 4 byte(s) within 0x00001d2c-0x00001d30
# 0 system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Buffer
# 1 USER32.dll!SetSystemCursor                                                +0x6c     (0x75780272 <USER32.dll+0x70272>)
# 2 USER32.dll!OpenWindowStationW                                             +0xa7b    (0x7576babf <USER32.dll+0x5babf>)
# 3 USER32.dll!LoadKeyboardLayoutW                                            +0x14     (0x7576bb2c <USER32.dll+0x5bb2c>)
# 4 ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody                   [ui\events\keycodes\platform_key_map_win_unittest.cc:309]
# 5 testing::internal::HandleExceptionsInMethodIfSupported<>                   [testing\gtest\src\gtest.cc:2458]
Note: @0:00:18.236 in thread 3168
Suppression (error hash=#54506E6D47A9FB57#):
For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-error-reports-from-the-
{
UNADDRESSABLE ACCESS
name=<insert_a_suppression_name_here>
system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Buffer
USER32.dll!SetSystemCursor
USER32.dll!OpenWindowStationW
USER32.dll!LoadKeyboardLayoutW
*!ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody
*!testing::internal::HandleExceptionsInMethodIfSupported<>
}

UNADDRESSABLE ACCESS: reading 0x00001d28-0x00001d2a 2 byte(s) within 0x00001d28-0x00001d2a
# 0 system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Length
# 1 USER32.dll!SetSystemCursor                                                +0x6c     (0x75780272 <USER32.dll+0x70272>)
# 2 USER32.dll!OpenWindowStationW                                             +0xa7b    (0x7576babf <USER32.dll+0x5babf>)
# 3 USER32.dll!LoadKeyboardLayoutW                                            +0x14     (0x7576bb2c <USER32.dll+0x5bb2c>)
# 4 ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody                   [ui\events\keycodes\platform_key_map_win_unittest.cc:309]
# 5 testing::internal::HandleExceptionsInMethodIfSupported<>                   [testing\gtest\src\gtest.cc:2458]
Note: @0:00:18.236 in thread 3168
Suppression (error hash=#EBB09B57A8C7AF19#):
For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-error-reports-from-the-
{
UNADDRESSABLE ACCESS
name=<insert_a_suppression_name_here>
system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Length
USER32.dll!SetSystemCursor
USER32.dll!OpenWindowStationW
USER32.dll!LoadKeyboardLayoutW
*!ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody
*!testing::internal::HandleExceptionsInMethodIfSupported<>
}

Original issue's description:
> [DomKey] Expose Korean special keys on Korean keyboard layout only
>
> Korean keyboard has special keys "HangulMode" and "HanjaMode", we should
> only expose them when the system layout is Korean. Otherwise we should
> return "Unidentified".
>
> This matches Firefox and windows virtual key.
>
> Note: This also works with JIS keyboard + Korean layout, because we are
> mapping DomKey based on KeyboardCode instead of DomCode.
>
> BUG= 612736 
>
> Committed: https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c
> Cr-Commit-Position: refs/heads/master@{#399783}

TBR=wez@chromium.org,chongz@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 612736 

Review-Url: https://codereview.chromium.org/2061123005
Cr-Commit-Position: refs/heads/master@{#399907}

[modify] https://crrev.com/a9248145f2dca37ab3492980ce884787818223ab/ui/events/keycodes/platform_key_map_win.cc
[modify] https://crrev.com/a9248145f2dca37ab3492980ce884787818223ab/ui/events/keycodes/platform_key_map_win_unittest.cc

Bruce, do you have any idea why Win32 LoadKeyboardLayout() would trigger DrMemory failures with specific layout Ids..?

FWIW it looks to me like both Korean and Japanese locales are special in that their only valid InputLayout Ids start with E0 (which I think indicates an "extended" layout?) - e.g. E0010412 for Korean.  By contrast, there are languages like Chinese_PRC which have both "extended" and a non-extended InputLayout code, which may explain why we don't see those fail.

Could you try with GetKeyboardLayout('E0010412'), if you haven't already?

Yes I've tried "GetKeyboardLayout('E0010412')", "GetKeyboardLayout('e0010412')", "GetKeyboardLayout('0412:E0010412')" and they all return en-US (HKL)0x04090409, however DrMemory shall pass.

"GetKeyboardLayout('00000412')" returns ko-KR (HKL)0x04120412 but fails DrMemory test.

Other languages (except Korean and Japanese) all has at least one locale start with 0x0000, I guess that's why they don't fail...

Correction: It should be "LoadKeyboardLayout(L'E0010412', KLF_ACTIVATE)", etc... Replacing KLF_ACTIVATE with 0 won't help.

brucedawson@ can you take a look at the LoadKeyboardLayout() issue as in #7 please? Thanks!

FWIW, the "First breaking build:" link in comment #6 is just a link to the builder. The actual first breaking build is this one:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28DrMemory%20full%29%20%285%29/builds/7511

But, not more information available from there.


The GetKeyboardLayout function has a pretty simple API and it should behave correctly (without reading inaccessible memory) when passed any null-terminated string. That is, it should not crash or read inaccessible memory even if passed an invalid keyboard layout identifier. Therefore, the DrMemory failure seems like it must necessarily either be a bug in DrMemory or a bug in Windows.

It could, perhaps, be interesting to investigate the behavior under the debugger in order to verify that it is a Windows bug in Windows 2008 R2 (and possibly other versions) but I wouldn't spend too much time on it. I think that we can justify adding a DrMemory suppression and re-landing the change.

Thanks for the comment! I will re-land the change with a revised unittest to skip the DrMemory issue, and update it after we got the DrMemory suppression.

sgtm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f4cf0ae18a6792114b1358f7bf56333b041ba27

commit 3f4cf0ae18a6792114b1358f7bf56333b041ba27
Author: chongz <chongz@chromium.org>
Date: Wed Jun 29 00:21:40 2016

Original issue's description:
> [DomKey] Expose Korean special keys on Korean keyboard layout only
>
> Korean keyboard has special keys "HangulMode" and "HanjaMode", we should
> only expose them when the system layout is Korean. Otherwise we should
> return "Unidentified".
>
> This matches Firefox and windows virtual key.
>
> Note: This also works with JIS keyboard + Korean layout, because we are
> mapping DomKey based on KeyboardCode instead of DomCode.
>
> BUG= 612736 
>
> Committed: https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c
> Cr-Commit-Position: refs/heads/master@{#399783}
> Revert: https://codereview.chromium.org/2061123005

Original issue:
Windows API |LoadKeyboardLayout()| won't pass DrMemory
tests for Korean and Japanese locale.
This should be an issue of DrMemory test or Windows itself.
See  https://crbug.com/612736#c11 

Solution:
Just skip |LoadKeyboardLayout()| and hardcode locale for Korean,
it works because we are only testing non-printable keys, which
doesn't care about |ToUnicodeEx()|.

Note:
|ToUnicodeEx()| returns empty string if the specified locale is invalid. (e.g. Korean HKL without calling 'LoadKeyboardLayout()')

Also Note:
"mock_keyboard.h" has the similar issue, e.g.
Call "MockKeyboard::GetCharacters(MockKeyboard::LAYOUT_KOREAN, ..)" somewhere and run DrMemory full test, which will fail as well.

BUG= 612736 

Review-Url: https://codereview.chromium.org/2097643002
Cr-Commit-Position: refs/heads/master@{#402611}

[modify] https://crrev.com/3f4cf0ae18a6792114b1358f7bf56333b041ba27/ui/events/keycodes/platform_key_map_win.cc
[modify] https://crrev.com/3f4cf0ae18a6792114b1358f7bf56333b041ba27/ui/events/keycodes/platform_key_map_win_unittest.cc

Created  issue 624527  for DrMemory tests.