Window emulation needs to SetProp inside WM_CREATE

RESOLVED FIXED in Firefox 56

Status

()

Core
Disability Access APIs
RESOLVED FIXED
14 days ago
8 days ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: aes+)

Attachments

(1 attachment)

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.
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
Created attachment 8885962 [details] [diff] [review]
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.
Attachment #8885962 - Flags: review?(yzenevich)
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
Attachment #8885962 - Flags: review?(yzenevich) → review+

Comment 4

13 days ago
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?
(In reply to alexander :surkov from comment #4)
> a newbie's question about lambdas: is 'this' guaranteed to over live the
> closure?

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

13 days ago
(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.
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
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
Flags: needinfo?(aklotz)
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
Flags: needinfo?(aklotz)

Comment 11

8 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c135a38a76a8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.