1380471 - Window emulation needs to SetProp inside WM_CREATE

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Looking into our latest problem reported by the JAWS developer:

JAWS' initial WM_GETOBJECT is failing because neither kPropNameDocAcc nor kPropNameDocAccParent are set on the emulated window.

I have seen this before in other contexts (Window neutering), and the reason that this happens is because we don't SetProp() until after CreateWindowEx() has returned.

We end up losing a race with JAWS hooks because those hook procedures are being invoked *before* we ever return from CreateWindowEx().

The solution is pretty straightforward: Instead of calling SetProp() after CreateWindowEx() returns, we need to handle WM_CREATE in the window procedure and do it from there.

We can pass any context that we need via the |lpParam| parameter to CreateWindowEx(), which is then passed to WM_CREATE via CREATESTRUCT::lpCreateParams.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Here is the call stack for the initial WM_GETOBJECT from JAWS. Notice that we are still executing inside CreateWindowEx when this hook fires:

00 xul!WindowProc+0x44
01 USER32!_InternalCallWinProc+0x2b
02 USER32!UserCallWinProcCheckWow+0x2d3
03 USER32!DispatchClientMessage+0xea
04 USER32!__fnDWORD+0x49
05 ntdll!KiUserCallbackDispatcher+0x4d
06 win32u!NtUserMessageCall+0xc
07 USER32!SendMessageTimeoutWorker+0x98
08 USER32!SendMessageTimeoutW+0x22
09 xul!mozilla::TIPMessageHandler::SendMessageTimeoutWHook+0x7d
0a OLEACC!NativeIAccessibleFromWindow+0x70
0b OLEACC!ORIGINAL_AccessibleObjectFromWindow+0x2b
0c OLEACC!AccessibleObjectFromWindow+0x2f
0d OLEACC!AccessibleObjectFromEvent+0x65
0e OLEACC!EXTERNAL_AccessibleObjectFromEvent+0x22
0f jhook+0x29599
10 jhook+0x2c6df
11 jhook+0x2f555
12 USER32!__ClientCallWinEventProc+0x43
13 ntdll!KiUserCallbackDispatcher+0x4d
14 win32u!NtUserCreateWindowEx+0xc
15 USER32!VerNtUserCreateWindowEx+0x237
16 USER32!CreateWindowInternal+0x152
17 USER32!CreateWindowExW+0x38
18 xul!mozilla::a11y::nsWinUtils::CreateNativeWindow+0x42
19 xul!mozilla::a11y::DocAccessibleParent::MaybeInitWindowEmulation+0xd3
1a xul!mozilla::dom::TabParent::RecvPDocAccessibleConstructor+0x146
1b xul!mozilla::dom::PBrowserParent::OnMessageReceived+0x754
1c xul!mozilla::dom::PContentParent::OnMessageReceived+0x5b
1d xul!mozilla::ipc::MessageChannel::DispatchAsyncMessage+0x4d
1e xul!mozilla::ipc::MessageChannel::DispatchMessageW+0x183
1f xul!mozilla::ipc::MessageChannel::RunMessage+0xec
20 xul!mozilla::ipc::MessageChannel::MessageTask::Run+0x3b
21 xul!nsThread::ProcessNextEvent+0x4a0
22 xul!NS_ProcessNextEvent+0x2d
23 xul!mozilla::ipc::MessagePump::Run+0xc3
24 xul!MessageLoop::RunInternal+0x8
25 xul!MessageLoop::RunHandler+0xa4
26 xul!MessageLoop::Run+0x3e
27 xul!nsBaseAppShell::Run+0x2f
28 xul!nsAppShell::Run+0x1f
29 xul!nsAppStartup::Run+0x20
2a xul!XREMain::XRE_mainRun+0xaf3
2b xul!XREMain::XRE_main+0x48b
2c xul!XRE_main+0x34
2d xul!mozilla::BootstrapImpl::XRE_main+0x11
2e firefox!do_main+0x25a
2f firefox!NS_internal_main+0x118
30 firefox!wmain+0x15f
31 firefox!invoke_main+0x1d
32 firefox!__scrt_common_main_seh+0xf9
33 KERNEL32!BaseThreadInitThunk+0x24
34 ntdll!__RtlUserThreadStart+0x2f
35 ntdll!_RtlUserThreadStart+0x1b

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Move follow-up initialization of emulated window to WM_CREATE handler

I modified the emulated window to accept a std::function that is invoked during WM_CREATE, and then moved the initialization code into lambdas that are then passed into nsWinUtils::CreateNativeWindow.

These callbacks are invoked *before* JAWS attempts its first WM_GETOBJECT, so we're able to get our initialization done ahead of time.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8885962 [details] [diff] [review]
Move follow-up initialization of emulated window to WM_CREATE handler

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

this is great, thanks!

::: accessible/windows/msaa/DocAccessibleWrap.cpp
@@ +162,5 @@
>          nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(container);
>          docShell->GetIsActive(&isActive);
>        }
>  
> +      nsWinUtils::NativeWindowCreateProc onCreate([this](HWND aHwnd) -> void {

Since this code only runs in parent process, I guess this issue happens for docs that are in parent process and if e10s is off.

::: accessible/windows/msaa/nsWinUtils.cpp
@@ +151,5 @@
> +      ::SetPropW(hWnd, kPropNameTabContent, reinterpret_cast<HANDLE>(1));
> +
> +      auto createStruct = reinterpret_cast<CREATESTRUCT*>(lParam);
> +      auto createProc = reinterpret_cast<nsWinUtils::NativeWindowCreateProc*>(
> +          createStruct->lpCreateParams);

nit: indentation

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8885962 [details] [diff] [review]
Move follow-up initialization of emulated window to WM_CREATE handler

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

::: accessible/ipc/DocAccessibleParent.cpp
@@ +622,5 @@
>      auto tab = static_cast<dom::TabParent*>(Manager());
>      tab->GetDocShellIsActive(&isActive);
>    }
>  
> +  nsWinUtils::NativeWindowCreateProc onCreate([this](HWND aHwnd) -> void {

a newbie's question about lambdas: is 'this' guaranteed to over live the closure?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to alexander :surkov from comment #4)
> a newbie's question about lambdas: is 'this' guaranteed to over live the
> closure?

No guarantees.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

I should clarify: The reason that it is safe to capture |this| in this case is because the lambda is only ever invoked during WM_CREATE, which always happens before CreateWindowEx returns.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #6)
> I should clarify: The reason that it is safe to capture |this| in this case
> is because the lambda is only ever invoked during WM_CREATE, which always
> happens before CreateWindowEx returns.

now it's clear, thanks :) I wasn't sure about CreateWindowEx internals, what exactly they do with the function.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec72cbf67b6e612ef7eff1a542ea99ac2539b5d
Bug 1380471: Move follow-up initialization for emulated windows into a callback invoked by the emulated window's WM_CREATE handler; r=yzen

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Windows 2012 static bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4effa27df4c80076fac61fca0438af0a841097

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7ec72cbf67b6e612ef7eff1a542ea99ac2539b5d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114186591&repo=mozilla-inbound

01:59:50     INFO -  In file included from z:/task_1499994090/build/src/obj-firefox/accessible/windows/msaa/Unified_cpp_windows_msaa0.cpp:38:
01:59:50     INFO -  z:/task_1499994090/build/src/accessible/windows/msaa/DocAccessibleWrap.cpp(167,69):  error: Refcounted variable 'this' of type 'mozilla::a11y::DocAccessibleWrap' cannot be captured by a lambda
01:59:50     INFO -          ::SetPropW(aHwnd, kPropNameDocAcc, reinterpret_cast<HANDLE>(this));
01:59:50     INFO -                                                                      ^
01:59:50     INFO -  z:/task_1499994090/build/src/accessible/windows/msaa/DocAccessibleWrap.cpp(167,69):  note: Please consider using a smart pointer
01:59:50     INFO -  20 warnings and 1 error generated.
01:59:50     INFO -  z:/task_1499994090/build/src/config/rules.mk:1048: recipe for target 'Unified_cpp_windows_msaa0.obj' failed
01:59:50     INFO -  mozmake.EXE[5]: *** [Unified_cpp_windows_msaa0.obj] Error 1
01:59:50     INFO -  mozmake.EXE[5]: Leaving directory 'z:/task_1499994090/build/src/obj-firefox/accessible/windows/msaa'
01:59:50     INFO -  z:/task_1499994090/build/src/config/recurse.mk:73: recipe for target 'accessible/windows/msaa/target' failed
01:59:50     INFO -  mozmake.EXE[4]: *** [accessible/windows/msaa/target] Error 2

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c135a38a76a806508fe5d12493441e2fea082b3c
Bug 1380471: Move follow-up initialization for emulated windows into a callback invoked by the emulated window's WM_CREATE handler; r=yzen

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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