Closed Bug 1330460 Opened 7 years ago Closed 7 years ago

Prevent touchscreens on Windows 8.x from instantiating accessibility (64-bit builds)

Categories

(Core :: Widget: Win32, defect, P1)

x86_64
Windows 8
defect

Tracking

()

RESOLVED FIXED
mozilla54
Performance Impact high
Tracking Status
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: bugzilla, Assigned: handyman)

References

Details

(Whiteboard: ,aes+, tpi:+)

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1325676 +++

We need to enable this on x64 once bug 1328996 lands.
Priority: -- → P1
Whiteboard: aes+ → aes+, tpi:+
Hi Aaron,

My Windows 8.1 Pro development desktop machine (no touch screen) is hitting the assertion mention in bug 1325676 comment #6 in a debug build:
> Assertion failure: ok, at c:/…/widget/windows/nsWindow.cpp:464 [1]
> mozilla::TIPMessageHandler::Initialize() (c:\…\widget\windows\nsWindow.cpp:425)

Is there a workaround for this? I tried various prefs to disable e10s and accessibility and also --safe-mode but none seemed to work.

Thanks

[1] https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/widget/windows/nsWindow.cpp#464
Flags: needinfo?(aklotz)
Thanks Matt. I have filed bug 1336515 for this.
Flags: needinfo?(aklotz)
Assignee: nobody → davidp99
Summary: Prevent touchscreens on Windows 8.x from instantiating accessibility → Prevent touchscreens on Windows 8.x from instantiating accessibility (64-bit builds)
So, this patch should do what we discussed yesterday.  Let me know if its not what you had in mind.

However, this doesn't resolve the issue on my new Surface book (Win 10).  I've traced the behavior and it hooks the expected methods but the WH_GETMESSAGE/OBJID_CLIENT message that it wants never arrives.  I figure this means that the internals have changed and that this was not guaranteed behavior to begin with (again, I'm guessing).  I _do_ see OBJID_WINDOW messages (no big surprise) as well as a bunch of undefined messages coming in (id is -25 -- not a declared OBJID value).  I figured these undefined messages could be the replacement but treating them the same way as your OBJID_CLIENT handler didn't work.

I need to dig deeper into the roots of what this is supposed to do to determine why its failing.  In the mean time, is there any reason to go forward with what we have?  Right now, I have no evidence it will help anyone.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c7cec7687c9ee7333b1bf292fce53a63dcf3045
Attachment #8840475 - Flags: review?(aklotz)
(In reply to David Parks (dparks) [:handyman] from comment #3)
> However, this doesn't resolve the issue on my new Surface book (Win 10). 
> I've traced the behavior and it hooks the expected methods but the
> WH_GETMESSAGE/OBJID_CLIENT message that it wants never arrives.  I figure
> this means that the internals have changed and that this was not guaranteed
> behavior to begin with (again, I'm guessing).  I _do_ see OBJID_WINDOW
> messages (no big surprise) as well as a bunch of undefined messages coming
> in (id is -25 -- not a declared OBJID value).  I figured these undefined
> messages could be the replacement but treating them the same way as your
> OBJID_CLIENT handler didn't work.

We are specifically interested in preventing the WM_GETOBJECT message with object id OBJID_CLIENT from reaching our window, as that is the "magic doorknock," if you will, that triggers a11y instantiation. The other values we don't care about and are just handled by Windows.

Looking at the docs for WM_GETOBJECT
https://msdn.microsoft.com/en-us/library/windows/desktop/dd373892(v=vs.85).aspx

I see:

A server application must check this value to identify the type of information being requested. Before comparing this value against the OBJID_ values, the server must cast it to DWORD; otherwise, on 64-bit Windows, the sign extension of the lParam can interfere with the comparison.


Maybe I'm missing some casts, so we don't get the comparison right?
> Maybe I'm missing some casts, so we don't get the comparison right?

You called it.  This new patch now works for me.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6201aaf0efa123835df497162f32c74e2ad7622b
Attachment #8840475 - Attachment is obsolete: true
Attachment #8840475 - Flags: review?(aklotz)
Attachment #8840531 - Flags: review?(aklotz)
Comment on attachment 8840531 [details] [diff] [review]
Do not block TIPMessageHandler on Win8/Win10

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

::: widget/windows/nsWindow.cpp
@@ +455,5 @@
>      MOZ_ASSERT(mHook);
>  
>      // On touchscreen devices, tiptsf.dll will have been loaded when STA COM was
>      // first initialized.
> +    if (GetModuleHandle(L"tiptsf.dll") && !sProcessCaretEventsStub) {

This hook should only be necessary on Windows 8, hence the !IsWin10OrLater() check that was in here.

Before landing, can you please verify whether or not this change is necessary and remove it if it is not?
Attachment #8840531 - Flags: review?(aklotz) → review+
> Before landing, can you please verify whether or not this change is
> necessary and remove it if it is not?

Indeed, the change wasn't needed.  I've taken it out.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f22758a633b26e3b420aafa73facd30afee2bc8e
Attachment #8840531 - Attachment is obsolete: true
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b2b65a1b476
Unblock parts of TIPMessageHandler in order to block a11y on Win 8+ 64-bit touchscreens. a=aklotz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2b2b65a1b476
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I'm guessing this isn't uplift material considering the dependency on bug 1328996; updating status flags.
Whiteboard: aes+, tpi:+ → [qf:p1],aes+, tpi:+
Performance Impact: --- → P1
Whiteboard: [qf:p1],aes+, tpi:+ → ,aes+, tpi:+
You need to log in before you can comment on or make changes to this bug.