1297549 - Arrow keys will not read the page with a11y and e10s

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[tapper82]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160823072522

Steps to reproduce:

go to about:config and create a new boolean pref named browser.tabs.remote.force-enable and set it to true. Restart Nightly, and tried to enjoy a11y+e10s. 


Actual results:

I can tab through the page and my screen reader will read out the links and headings. when using the arrow keys to read up and down the screen i get no feedback.


Expected results:

my screen reader should read out the webpage line by line

Comment 1

[tapper82]

screenreader used is NVDA next 13513,9e4ea1d3
Fri Aug 19 04:49:42 2016
http://www.nvaccess.org/files/nvda/snapshots/

Comment 2

[tapper82]

OS is windows 10 X64 build 14905

Comment 3

[tapper82]

Application Basics
------------------

Name: Firefox
Version: 51.0a1
Build ID: 20160823072522
Update Channel: nightly
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
OS: Windows_NT 10.0
Multiprocess Windows: 0/3 (Disabled)
Safe Mode: false

Crash Reports for the Last 3 Days
---------------------------------

All Crash Reports

Extensions
----------

Name: FlyWeb
Version: 1.0.0
Enabled: true
ID: [e-mail]

Name: Multi-process staged rollout
Version: 1.0
Enabled: true
ID: [e-mail]

Name: NoScript
Version: 2.9.0.14rc1
Enabled: true
ID: {73a6fe31-595d-460b-a920-fcc0f8843232}

Name: uBlock Origin
Version: 1.8.5rc1
Enabled: true
ID: [e-mail]

Name: Web Compat
Version: 1.0
Enabled: true
ID: [e-mail]

Name: Google™ Hangouts
Version: 0.1.6
Enabled: false
ID: jid1-uqbSKwXpf2K6yl@jetpack

Name: HTTPS-Everywhere
Version: 5.2.1
Enabled: false
ID: [e-mail]

Name: Pocket
Version: 1.0.4
Enabled: false
ID: [e-mail]

Graphics
--------

Features
Compositing: Direct3D 11
Asynchronous Pan/Zoom: none
WebGL Renderer: Google Inc. -- ANGLE (NVIDIA GeForce GTX 260 Direct3D11 vs_4_0 ps_4_0)
WebGL2 Renderer: WebGL creation failed: * Refused to create native OpenGL context because of blacklist entry: WEBGL_NATIVE_GL_OLD_NVIDIA * Exhausted GL driver options.
Hardware H264 Decoding: Yes; Using D3D11 API
Audio Backend: wasapi
Direct2D: true
DirectWrite: true (10.0.14905.1000)
GPU #1
Active: Yes
Description: NVIDIA GeForce GTX 260
Vendor ID: 0x10de
Device ID: 0x05e2
Driver Version: 9.18.13.4195
Driver Date: 1-29-2016
Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Subsys ID: 2ae2107d
RAM: 896

Diagnostics
AzureCanvasAccelerated: 0
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo





Important Modified Preferences
------------------------------

accessibility.lastLoadDate: 1471995104
accessibility.loadedInLastSession: true
accessibility.typeaheadfind.flashBar: 0
browser.cache.disk.capacity: 358400
browser.cache.disk.filesystem_reported: 1
browser.cache.disk.smart_size.first_run: false
browser.cache.disk.smart_size.use_old_max: false
browser.cache.frecency_experiment: 4
browser.download.folderList: 2
browser.download.importedFromSqlite: true
browser.places.smartBookmarksVersion: 8
browser.search.useDBForOrder: true
browser.sessionstore.upgradeBackup.latestBuildID: 20160823072522
browser.startup.homepage: https://startpage.com/do/mypage.pl?prf=3b36323faf5a729d448138aed4d27bcb
browser.startup.homepage_override.buildID: 20160823072522
browser.startup.homepage_override.mstone: 51.0a1
browser.tabs.animate: false
browser.tabs.remote.autostart.2: false
browser.tabs.remote.disabled-for-a11y: false
browser.tabs.remote.force-enable: false
browser.tabs.warnOnClose: false
browser.tabs.warnOnOpen: false
browser.urlbar.trimURLs: false
browser.urlbar.userMadeSearchSuggestionsChoice: true
dom.apps.lastUpdate.buildID: 20160725030248
dom.apps.lastUpdate.mstone: 50.0a1
dom.apps.reset-permissions: true
dom.max_chrome_script_run_time: 0
dom.max_script_run_time: 0
dom.mozApps.used: true
dom.push.userAgentID: 8a3cf3ac2f714cf095a397a273c3f27d
extensions.lastAppVersion: 51.0a1
font.internaluseonly.changed: false
gfx.crash-guard.d3d11layers.appVersion: 47.0
gfx.crash-guard.d3d11layers.deviceID: 0x06cd
gfx.crash-guard.d3d11layers.driverVersion: 10.18.13.6472
gfx.crash-guard.d3d11layers.feature-d2d: true
gfx.crash-guard.d3d11layers.feature-d3d11: true
gfx.crash-guard.status.d3d11layers: 2
gfx.crash-guard.status.d3d9video: 2
gfx.direct3d.last_used_feature_level_idx: 0
gfx.offscreencanvas.enabled: true
media.benchmark.vp9.fps: 209
media.benchmark.vp9.versioncheck: 1
media.gmp-eme-adobe.abi: x86_64-msvc-x64
media.gmp-eme-adobe.lastUpdate: 1459522893
media.gmp-eme-adobe.version: 17
media.gmp-gmpopenh264.abi: x86_64-msvc-x64
media.gmp-gmpopenh264.lastUpdate: 1470777225
media.gmp-gmpopenh264.version: 1.6
media.gmp-manager.buildID: 20160823072522
media.gmp-manager.lastCheck: 1471984300
media.gmp-widevinecdm.abi: x86_64-msvc-x64
media.gmp-widevinecdm.lastUpdate: 1470777228
media.gmp-widevinecdm.version: 1.4.8.903
media.gmp.storage.version.observed: 1
media.hardware-video-decoding.failed: false
media.peerconnection.ice.default_address_only: true
media.webrtc.debug.aec_log_dir: C:\Users\jlanc\AppData\Local\Temp
media.webrtc.debug.log_file: C:\Users\jlanc\AppData\Local\Temp\WebRTC.log
network.cookie.prefsMigrated: true
network.dns.disablePrefetch: true
network.http.speculative-parallel-limit: 0
network.predictor.cleaned-up: true
network.prefetch-next: false
places.database.lastMaintenance: 1471958188
places.history.expiration.transient_current_max_pages: 51186
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_bgcolor: false
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_bgimages: false
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_duplex: -437918235
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_edge_bottom: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_edge_left: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_edge_right: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_edge_top: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_evenpages: true
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_footercenter:
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_footerleft: &PT
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_footerright: &D
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_headercenter:
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_headerleft: &T
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_headerright: &U
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_in_color: true
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_margin_bottom: 0.5
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_margin_left: 0.5
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_margin_right: 0.5
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_margin_top: 0.5
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_oddpages: true
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_orientation: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_page_delay: 50
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_paper_data: 9
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_paper_height: -1.00
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_paper_name:
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_paper_size_unit: 1
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_paper_width: -1.00
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_resolution: 600
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_reversed: false
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_scaling: 1.00
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_shrink_to_fit: true
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_to_file: false
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_unwriteable_margin_bottom: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_unwriteable_margin_left: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_unwriteable_margin_right: 0
print.printer_HP-Printer_(HP_Photosmart_B110_series).print_unwriteable_margin_top: 0
privacy.donottrackheader.enabled: true
privacy.sanitize.migrateClearSavedPwdsOnExit: true
privacy.sanitize.migrateFx3Prefs: true
privacy.trackingprotection.enabled: true
privacy.trackingprotection.introCount: 20
security.sandbox.content.tempDirSuffix: {8ddc25b2-a177-4af1-acc2-28039cc147f7}
security.ssl.errorReporting.automatic: true
services.sync.declinedEngines:
services.sync.engine.prefs.modified: false
services.sync.lastPing: 1471943188
services.sync.lastSync: Wed Aug 24 2016 00:23:39 GMT+0100 (GMT Standard Time)
services.sync.numClients: 3
storage.vacuum.last.index: 1
storage.vacuum.last.places.sqlite: 1471869959
ui.osk.debug.keyboardDisplayReason: IKPOS: Touch screen not found.
ui.osk.enabled: false

Important Locked Preferences
----------------------------

Places Database
---------------

JavaScript
----------

Incremental GC: true

Accessibility
-------------

Activated: false
Prevent Accessibility: 0

Library Versions
----------------

NSPR
Expected minimum version: 4.12
Version in use: 4.12

NSS
Expected minimum version: 3.27 Beta
Version in use: 3.27 Beta

NSSSMIME
Expected minimum version: 3.27 Beta
Version in use: 3.27 Beta

NSSSSL
Expected minimum version: 3.27 Beta
Version in use: 3.27 Beta

NSSUTIL
Expected minimum version: 3.27 Beta
Version in use: 3.27 Beta

Experimental Features
---------------------

Name: DisplayPort Size Tuning
ID: [e-mail]
Description: Measuring the effect of different displayport sizes on checkerboarding.
Active: false
End Date: 1457186284462
Homepage:
Branch: group3

Sandbox
-------

Content Process Sandbox Level: 2

Comment 4

[Marco Zehe (:MarcoZ)]

This is definitely confirmed. While focus tracking works, NVDA's virtual buffers remain completely empty in all tabs. They are initialized, they just don't get any content. I am getting many NVDA errors which I'll attach below. I didn't bother logging the COM traffic, since this is very straight-forward to reproduce. Any web page comes up with an empty virtual buffer, so something elementary is still missing.

Comment 5

[Marco Zehe (:MarcoZ)]

Attachment: nvda E10S Errors log file

Comment 6

[James Teh [:Jamie]]

Just took a quick look at this. When we try to fetch the document object out-of-process, everything works as expected. However, in-process, calling AccessibleObjectFromEvent succeeds but gives back the same child id (the document's unique id) in pvarChild. pvarChild *should* be CHILDID_SELF. This suggests that either we're being handed back an oleacc client (which means WM_GETOBJECT/ObjectFromLresult is failing) or IAccessible::accChild is failing.

Aaron, if you need me to poke further, I can try calling WM_GETOBJECT/LresultFromObject/accChild directly (rather than using AccessibleObjectFromEvent) which might give us a better idea of which bit is failing.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #6)
> Just took a quick look at this. When we try to fetch the document object
> out-of-process, everything works as expected. However, in-process, calling
> AccessibleObjectFromEvent succeeds but gives back the same child id (the
> document's unique id) in pvarChild. pvarChild *should* be CHILDID_SELF. This
> suggests that either we're being handed back an oleacc client (which means
> WM_GETOBJECT/ObjectFromLresult is failing) or IAccessible::accChild is
> failing.

Thanks Jamie, that's very useful info!

Comment 8

[James Teh [:Jamie]]

Did a bit more investigation. Calling IAccessible::get_accChild in-proc with the document's unique id fails with E_INVALIDARG. I also tried QueryService to IAccessible2 first (to be sure I was bypassing oleacc). Same result; accChild throws E_INVALIDARG.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

How's this for nutty: we get an accessible, we call LResultFromObject, which succeeds. But when oleacc calls ObjectFromLresult, it returns E_FAIL!

I'm adding debug code to check this on the Gecko side, but now I need to figure out why on earth Windows is able to marshal it to an LResult but not back again (in the same process and apartment no less)!

Comment 10

[James Teh [:Jamie]]

> How's this for nutty: we get an accessible, we call LResultFromObject, which
> succeeds. But when oleacc calls ObjectFromLresult, it returns E_FAIL!

That doesn't fit with what I'm seeing. I was able to successfully QS to IA2 on the object I got back (and CHILDID_SELF had a role of ROLE_SYSTEM_APPLICATION), which means ObjectFromLresult must have succeeded. It was only when I called accChild (even on the IA2 interface, which means I was definitely not talking to an oleacc proxy) that I got E_INVALIDARG. Or are you saying that internally, you use LresultFromObject even for accChild?

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

This was based on info from the windows sdk accevent utility, but further investigation contradicted those results.

Latest info suggests to me that the child id resolves to a valid object in the content process but not in chrome. I am now investigating our event firing code.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

I just realized that we're probably seeing ID leakage from the content process via IAccessible2::get_uniqueID.

Since NVDA assumes that those are child ids, but the chrome process does not recognize ids generated by content, that's probably why we're seeing this.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

I'll see if I can make that API return the chrome-generated child id when running in e10s content.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

I'm working on a solution based on a cleaner resolution of child IDs that should dovetail with the stuff we want to do in bug 1289852.

One snag that I'm hitting with my current fix is that sometimes outgoing COM calls fail with RPC_E_CANTCALLOUT_ININPUTSYNCCALL. According to https://support.microsoft.com/en-ca/kb/131056 and https://support.microsoft.com/en-ca/kb/198996 the com runtime checks to see if we are currently handling a blocking inter-thread SendMessage call via InSendMessageEx. If so, the call fails.

The reason why this happens in NVDA is because the rendering of the virtual buffer is initiated via a CallWndProc hook. By definition we are inside a SendMessage call when rendering is initiated.

Sadly this requires a client change but I think it is necessary: Jamie, how do you feel about the feasibility of either:

* Using a GetMessage hook to trigger rendering instead; or
* Adding an InSendMessageEx check inside the CallWndProc hook to avoid initiating rendering when there are blocked message senders (See the "Remarks" section of the InSendMessageEx docs at https://msdn.microsoft.com/en-us/library/windows/desktop/ms644942(v=vs.85).aspx for sample code)

Comment 15

[James Teh [:Jamie]]

(In reply to Aaron Klotz [:aklotz] from comment #14)
> One snag that I'm hitting with my current fix is that sometimes outgoing COM
> calls fail with RPC_E_CANTCALLOUT_ININPUTSYNCCALL. ...
> the com runtime checks to see
> if we are currently handling a blocking inter-thread SendMessage call via
> InSendMessageEx. If so, the call fails.

The article talks about OLE, but as I understand it, OLE is all STA based. Shouldn't this only apply to calls which use STA marshalling? I thought you'd proven that calls to content in the chrome process would use ALPC. Or is COM just a bit silly here?

> Sadly this requires a client change but I think it is necessary: Jamie, how
> do you feel about the feasibility of either:
> * Using a GetMessage hook to trigger rendering instead; or

We might be able to do that. We need the call to block (that's why we're using SendMessage in the first place), but we can probably use PostMessage and an event instead. The problem is that this change would affect all browsers, not just Firefox, so we'd need to make sure it didn't break things right across the board. Also, because PostMessage goes through the queue, it's possible it might delay the initial render a bit, though that hopefully shouldn't have any practical impact.

> * Adding an InSendMessageEx check inside the CallWndProc hook to avoid
> initiating rendering when there are blocked message senders

I don't quite follow this. We initiate rendering from a background (RPC) thread and we SendMessage to the main thread. Calling InSendMessageEx in our RPC thread isn't useful because our RPC thread never deals with messages. Calling it in the main thread will always say there's a blocked sender because we sent a message, so we'd never be able to render.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #15)
> (In reply to Aaron Klotz [:aklotz] from comment #14)
> > One snag that I'm hitting with my current fix is that sometimes outgoing COM
> > calls fail with RPC_E_CANTCALLOUT_ININPUTSYNCCALL. ...
> > the com runtime checks to see
> > if we are currently handling a blocking inter-thread SendMessage call via
> > InSendMessageEx. If so, the call fails.
> 
> The article talks about OLE, but as I understand it, OLE is all STA based.
> Shouldn't this only apply to calls which use STA marshalling? I thought
> you'd proven that calls to content in the chrome process would use ALPC. Or
> is COM just a bit silly here?

Exactly, COM is being silly. Even though we're using MTA marshalling, COM is basing its decision on the fact that the client thread is in the STA.

> 
> > Sadly this requires a client change but I think it is necessary: Jamie, how
> > do you feel about the feasibility of either:
> > * Using a GetMessage hook to trigger rendering instead; or
> 
> We might be able to do that. We need the call to block (that's why we're
> using SendMessage in the first place), but we can probably use PostMessage
> and an event instead. The problem is that this change would affect all
> browsers, not just Firefox, so we'd need to make sure it didn't break things
> right across the board. Also, because PostMessage goes through the queue,
> it's possible it might delay the initial render a bit, though that hopefully
> shouldn't have any practical impact.
> 
> > * Adding an InSendMessageEx check inside the CallWndProc hook to avoid
> > initiating rendering when there are blocked message senders
> 
> I don't quite follow this. We initiate rendering from a background (RPC)
> thread and we SendMessage to the main thread. Calling InSendMessageEx in our
> RPC thread isn't useful because our RPC thread never deals with messages.
> Calling it in the main thread will always say there's a blocked sender
> because we sent a message, so we'd never be able to render.

I have this call stack on the Firefox parent process main thread:

OLEACC!AccWrap_Base::get_accChild+0x4b
OLEACC!AccessibleObjectFromEvent+0xb8
OLEACC!EXTERNAL_AccessibleObjectFromEvent+0x22
VBufBackend_gecko_ia2!IAccessible2FromIdentifier+0x233
VBufBackend_gecko_ia2!GeckoVBufBackend_t::render+0x16
VBufBackend_gecko_ia2!VBufBackend_t::update+0x313
VBufBackend_gecko_ia2!VBufBackend_t::renderThread_initialize+0x1e
VBufBackend_gecko_ia2!GeckoVBufBackend_t::renderThread_initialize+0x1c
VBufBackend_gecko_ia2!VBufBackend_t::renderThread_callWndProcHook+0x2b
nvdaHelperRemote!inProcess_callWndProcHook+0xe0
USER32!fnHkINLPCWPSTRUCTW+0x93
USER32!__fnDWORD+0x49
ntdll!KiUserCallbackDispatcher+0x36
USER32!NtUserPeekMessage+0xc
USER32!_PeekMessage+0x2a
USER32!PeekMessageW+0x16f

So what I'm suggesting is that VBufBackend_t::renderThread_callWndProcHook does not call renderThread_initialize unless the InSendMessageEx shows that nothing is blocking... but based on your comment, should this code even be running on our main thread?

Comment 17

[James Teh [:Jamie]]

> So what I'm suggesting is that VBufBackend_t::renderThread_callWndProcHook does not call renderThread_initialize unless the InSendMessageEx shows that nothing is blocking...

renderThread_initialize is being called by the hook due to a SendMessage from our RPC thread. Because our RPC thread is doing a SendMessage, InSendMessageEx called from renderThread_callWndProcHook will always say something is blocking (our RPC thread). Thus, we will never render. Or am I misunderstanding something here?

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Yeah, sorry, I went and looked at your code and got a clearer picture of what's going on (plus, it's not 1AM when my comprehension is less than ideal).

Actually I think the best solution here would be if you used SendMessageCallback() instead of SendMessage(); if you pump messages on that RPC thread until the reply callback is invoked, we both get what we want: NVDA will be able to wait until the buffer rendering is complete, and COM won't fail.

Comment 19

[James Teh [:Jamie]]

I actually went with PostMessage and an event, since it was easier to implement than SendMessageCallback.

Here's a try build with the change:
https://ci.appveyor.com/project/NVAccess/nvda/build/try-vbufPostMessage-13547,637e31f8/artifacts
Let me know how you go.

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Yeah, things work much better now.

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

Current status:

I think I've fixed up all of the uniqueID issues such that content returns chrome's unique id.

NVDA still can't populate the VBuf, however, due to IAccessible2::get_windowHandle not working. Unfortunately the solution to this isn't as straightforward as one would like to believe:

* We can't implement this as a sync content-to-chrome call due to reentrancy concerns (we will deadlock). Also, it seems kind of absurd to do all of this "communicate directly with content" stuff only to turn around and synchronously query the chrome process anyway;

* I think that the best way to go forward with this is to send the Window handle down to the DocAccessibleChild. However, we also need to be able to update this field if the window handle changes (eg, tab moved between two distinct windows). We need to subscribe to an event to make this happen, however my understanding is that we don't have that event *yet*. mconley pointed out to me that billm's patch in bug 1279086 adds an "EndSwapDocShells" event that would be ideal for this situation: when we receive that event, chrome can query for the new hwnd and then asynchronously send that down to content.

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #21)
> * I think that the best way to go forward with this is to send the Window
> handle down to the DocAccessibleChild. However, we also need to be able to
> update this field if the window handle changes (eg, tab moved between two
> distinct windows). We need to subscribe to an event to make this happen,
> however my understanding is that we don't have that event *yet*. mconley
> pointed out to me that billm's patch in bug 1279086 adds an
> "EndSwapDocShells" event that would be ideal for this situation: when we
> receive that event, chrome can query for the new hwnd and then
> asynchronously send that down to content.

I managed to figure out how to hook into this without needing billm's patch. Once implemented, the NVDA VBuf worked! \o/

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Propagate HWND to TabChild

Jim, would you mind taking a peek at this and letting me know if I got this right?

I need TabParent to push the tab's HWND down to the TabChild whenever it changes. This is done in order to support an IAccessible2 API that retrieves the HWND of the top-level window that is associated with the accessible object.

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8788542 [details] [diff] [review]
Propagate HWND to TabChild

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

What are we going to do with this on the child side?

We have to be careful about apis that act on hwnds.. the sandbox breaks this stuff. we also want to be careful about apis that generate sync sned messages over to the browser.

If you need the hwnd though you need it, little harm in sending it over.

::: dom/ipc/PBrowser.ipdl
@@ +823,5 @@
>       * @param aPrintData the serialized settings to print with
>       */
>      async Print(uint64_t aOuterWindowID, PrintData aPrintData);
>  
> +    async UpdateHwnd(uintptr_t aNewHwnd);

I'd use more generic terms throughout - UpdateNativeWindowHandle or UpdateNaiveHandle.

Also this entry needs a comment explaining what this is for.

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #24)
> Comment on attachment 8788542 [details] [diff] [review]
> Propagate HWND to TabChild
> 
> Review of attachment 8788542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What are we going to do with this on the child side?

Literally the only thing we do is return it as the output in the IAccessible2 API. To an a11y client, the IAccessible2 object "belongs" to that HWND even though it is being served from content. Any requests involving that HWND go back through chrome when the client calls AccessibleObjectFromEvent using that HWND.

> 
> We have to be careful about apis that act on hwnds.. the sandbox breaks this
> stuff. we also want to be careful about apis that generate sync sned
> messages over to the browser.

For sure.

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Allocate child ids in chrome and ensure propagation to content

If you have other ideas about pushing child id's down into content, I'm all ears. I'm particularly grossed out by the SendShowEvent case.

Comment 27

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 - Propagate HWND to TabChild

Comment 28

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3 - Make AccessibleWrap get hwnd from TabChild

Comment 29

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8789016 [details] [diff] [review]
Part 3 - Make AccessibleWrap get hwnd from TabChild

># HG changeset patch
># User Aaron Klotz <[e-mail]>
># Date 1473182844 21600
>#      Tue Sep 06 11:27:24 2016 -0600
># Node ID 014cbac38485ab15e1225cb4b93dd27ff1bf7a1e
># Parent  f77abd71c64bfdd923de61465bf258808e961eff
>Bug 1297549: Part 3 - Modify Windows AccessibleWrap to get hwnd from TabChild under e10s; r=tbsaunde
>
>diff --git a/accessible/windows/msaa/AccessibleWrap.cpp b/accessible/windows/msaa/AccessibleWrap.cpp
>--- a/accessible/windows/msaa/AccessibleWrap.cpp
>+++ b/accessible/windows/msaa/AccessibleWrap.cpp
>@@ -4,16 +4,17 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "AccessibleWrap.h"
> #include "Accessible-inl.h"
> 
> #include "Compatibility.h"
> #include "DocAccessible-inl.h"
>+#include "mozilla/dom/TabChild.h"
> #include "mozilla/a11y/DocAccessibleChild.h"
> #include "mozilla/a11y/DocAccessibleParent.h"
> #include "EnumVariant.h"
> #include "nsAccUtils.h"
> #include "nsCoreUtils.h"
> #include "nsIAccessibleEvent.h"
> #include "nsWinUtils.h"
> #include "mozilla/a11y/ProxyAccessible.h"
>@@ -1290,16 +1291,30 @@ AccessibleWrap::GetChildIDFor(Accessible
> 
> HWND
> AccessibleWrap::GetHWNDFor(Accessible* aAccessible)
> {
>   if (!aAccessible) {
>     return nullptr;
>   }
> 
>+  if (XRE_IsContentProcess()) {

>+    if (!doc) {
>+      return nullptr;
>+    }
>+    DocAccessibleChild* ipcDoc = doc->IPCDoc();

personally I'd prefer a blank line after }

>+    if (!ipcDoc) {
>+      return nullptr;
>+    }

I doubt either of these should actually be null, but I guess we can live with the checks.

Comment 30

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8789013 [details] [diff] [review]
Part 1 - Allocate child ids in chrome and ensure propagation to content

So, I haven't carefully looked this over yet, but I don't see any real problems with it other than being kind of nasty as ou said.  On one hand I understand needing to fix this, but I think doing it the other way around would work a bit better.

My proposal is basically this, afaik there's no reason to prefer the id generated in the parent to the one generated in the child.  So when we are calculating NewTree for SendShowEvent() ask accessible for its id and shove it in AccessibleData.  You need to special case documents and pass another argument to PDocAccessibleConstructor I guess.  That should eliminate a lot of this ifdefery and stuff at the cost of a little bit of useless memseting on !windows.  Does that seem reasonable?

Comment 31

[James Teh [:Jamie]]

(In reply to Trevor Saunders (:tbsaunde) from comment #30)
> My proposal is basically this, afaik there's no reason to prefer the id
> generated in the parent to the one generated in the child. 

As I understand it, the browser chrome and content documents both use the same HWND. Unique ids need to be unique within the HWND. If you use ids from the child, they might conflict with ids generated for browser chrome in the parent.

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 8789015 [details] [diff] [review]
Part 2 - Propagate HWND to TabChild

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

::: dom/ipc/TabChild.h
@@ +650,5 @@
>        mAPZChild = aAPZChild;
>    }
>  
> +#if defined(XP_WIN) && defined(ACCESSIBILITY)
> +  HWND GetHwnd() const { return mHwnd; }

lets make this more generic, for example GetNativeWindowHandle() and have it return a uintptr_t. this way it's reusable on other platforms if we need it for something.

@@ +788,5 @@
>    layers::APZChild* mAPZChild;
>  
> +#if defined(XP_WIN) && defined(ACCESSIBILITY)
> +  // The Win32 HWND associated with the native window that contains this tab
> +  HWND mHwnd;

generic naming and store as a uintptr_t

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to James Teh [:Jamie] from comment #31)
> (In reply to Trevor Saunders (:tbsaunde) from comment #30)
> > My proposal is basically this, afaik there's no reason to prefer the id
> > generated in the parent to the one generated in the child. 
> 
> As I understand it, the browser chrome and content documents both use the
> same HWND. Unique ids need to be unique within the HWND. If you use ids from
> the child, they might conflict with ids generated for browser chrome in the
> parent.

good point.  I suppose we could section up the 2gb, but that seems complicated.  ugh.

So, I think where we are at is for now all ID must be generated in the parent.  This means PDocAccessible::ShowEvent() must be sync on windows (though I worry there is still a race here).  Since ShowEvent() must not be sync on !windows we seem to need most of the nastyness here.

My one concern with this is that the parent can / will fire a show event before returning from ShowEvent() if a client gets that event sync and tries to interigate a show accessible it still won't have the right id because ShowEvent() has not yet returned / the child hasn't handled the return from it yet.

Comment 34

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Trevor Saunders (:tbsaunde) from comment #33)
> (In reply to James Teh [:Jamie] from comment #31)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #30)
> > > My proposal is basically this, afaik there's no reason to prefer the id
> > > generated in the parent to the one generated in the child. 
> > 
> > As I understand it, the browser chrome and content documents both use the
> > same HWND. Unique ids need to be unique within the HWND. If you use ids from
> > the child, they might conflict with ids generated for browser chrome in the
> > parent.

Yes, in general I agree with Jamie's remarks: We need a "single namespace," if you will, for child IDs. Whether they come entirely from the parent or entirely from the child is mostly irrelevant except for the fact that chrome accessibles that comprise the root of the tree already have chrome-generated ids. Furthermore, multi-process e10s would introduce even more potential for collisions. It follows that parent-generated IDs are our only option.

> My one concern with this is that the parent can / will fire a show event
> before returning from ShowEvent() if a client gets that event sync and tries
> to interigate a show accessible it still won't have the right id because
> ShowEvent() has not yet returned / the child hasn't handled the return from
> it yet.

Oddly enough I agree that this should be possible but I do not recall seeing it when testing these patches. Let me take another look (specifically watching for this)...

Comment 35

[(No longer employed by Mozilla) Aaron Klotz]

Confirmed:

parent process:

00 ntdll!NtWaitForMultipleObjects+0xc
01 KERNELBASE!WaitForMultipleObjectsEx+0x10a
02 USER32!MsgWaitForMultipleObjectsEx+0x17b
03 combase!CCliModalLoop::BlockFn+0x11c
04 combase!ModalLoop+0xc6
05 combase!ClassicSTAThreadWaitForCall+0xc6
06 combase!ThreadSendReceive+0x3b1
07 combase!CSyncClientCall::SwitchAptAndDispatchCall+0x1e8
08 combase!CSyncClientCall::SendReceive2+0x417
09 combase!SyncClientCallRetryContext::SendReceiveWithRetry+0x2f
0a combase!CSyncClientCall::SendReceiveInRetryContext+0x2f
0b combase!ClassicSTAThreadSendReceive+0x24a
0c combase!CSyncClientCall::SendReceive+0x362
0d combase!CClientChannel::SendReceive+0xbb
0e combase!NdrExtpProxySendReceive+0x11b
0f RPCRT4!NdrpProxySendReceive+0x29
10 RPCRT4!NdrClientCall2+0x39e
11 combase!ObjectStublessClient+0x74
12 combase!ObjectStubless+0xf
13 VBufBackend_gecko_ia2!GeckoVBufBackend_t::fillVBuf+0x107
14 VBufBackend_gecko_ia2!GeckoVBufBackend_t::fillVBuf+0x21a9
15 VBufBackend_gecko_ia2!GeckoVBufBackend_t::render+0x52
16 VBufBackend_gecko_ia2!VBufBackend_t::update+0x313
17 VBufBackend_gecko_ia2!VBufBackend_t::renderThread_initialize+0x1e
18 VBufBackend_gecko_ia2!GeckoVBufBackend_t::renderThread_initialize+0x1c
19 VBufBackend_gecko_ia2!VBufBackend_t::renderThread_callWndProcHook+0x2b
1a nvdaHelperRemote!inProcess_callWndProcHook+0xe0
1b USER32!fnHkINLPCWPSTRUCTW+0x93
1c USER32!__fnDWORD+0x49
1d ntdll!KiUserCallbackDispatcher+0x36
1e USER32!NtUserPeekMessage+0xc
1f USER32!_PeekMessage+0x2a
20 USER32!PeekMessageW+0x16f
21 combase!CCliModalLoop::MyPeekMessage+0x31
22 combase!CCliModalLoop::PeekRPCAndDDEMessage+0x31
23 combase!CCliModalLoop::BlockFn+0x1a6
24 combase!ModalLoop+0xc6
25 combase!ClassicSTAThreadWaitForCall+0xc6
26 combase!ThreadSendReceive+0x3b1
27 combase!CSyncClientCall::SwitchAptAndDispatchCall+0x1e8
28 combase!CSyncClientCall::SendReceive2+0x417
29 combase!SyncClientCallRetryContext::SendReceiveWithRetry+0x2f
2a combase!CSyncClientCall::SendReceiveInRetryContext+0x2f
2b combase!ClassicSTAThreadSendReceive+0x24a
2c combase!CSyncClientCall::SendReceive+0x362
2d combase!CClientChannel::SendReceive+0xbb
2e combase!NdrExtpProxySendReceive+0x11b
2f RPCRT4!NdrpProxySendReceive+0x29
30 RPCRT4!NdrClientCall2+0x39e
31 combase!ObjectStublessClient+0x74
32 combase!ObjectStubless+0xf
33 nvdaHelperRemote!fetchIA2Attributes+0x44
34 nvdaHelperRemote!winEventProcHook+0x256
35 nvdaHelperRemote!inProcess_winEventCallback+0xa0
36 nvdaHelperRemote!inproc_winEventCallback+0x3b3
37 USER32!__ClientCallWinEventProc+0x43
38 ntdll!KiUserCallbackDispatcher+0x36
39 xul!mozilla::a11y::AccessibleWrap::FireWinEvent+0xf9
3a xul!mozilla::a11y::ProxyShowHideEvent+0x40
3b xul!mozilla::a11y::DocAccessibleParent::RecvShowEvent+0x305
3c xul!mozilla::a11y::PDocAccessibleParent::OnMessageReceived+0x292
3d xul!mozilla::dom::PContentParent::OnMessageReceived+0x89
3e xul!mozilla::ipc::MessageChannel::DispatchSyncMessage+0xda
3f xul!mozilla::ipc::MessageChannel::DispatchMessageW+0x17d
40 xul!mozilla::ipc::MessageChannel::OnMaybeDequeueOne+0x104
41 xul!mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void)>+0x9
42 xul!mozilla::detail::RunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void)>+0x1f
43 xul!mozilla::detail::RunnableMethodImpl<bool (__thiscall mozilla::ipc::MessageChannel::*)(void),0,1>::Run+0x30
44 xul!mozilla::ipc::MessageChannel::RefCountedTask::Run+0x25
45 xul!mozilla::ipc::MessageChannel::DequeueTask::Run+0x27
46 xul!nsThread::ProcessNextEvent+0x336
47 xul!NS_ProcessNextEvent+0x62
48 xul!mozilla::ipc::MessagePump::Run+0x1ec
49 xul!MessageLoop::RunInternal+0x4d
4a xul!MessageLoop::RunHandler+0x82
4b xul!MessageLoop::Run+0x1d
4c xul!nsBaseAppShell::Run+0x50
4d xul!nsAppShell::Run+0x23
4e xul!nsAppStartup::Run+0x6f
4f xul!XREMain::XRE_mainRun+0x15f0
50 xul!XREMain::XRE_main+0x543
51 xul!XRE_main+0x35
52 firefox!do_main+0x766
53 firefox!NS_internal_main+0x19a
54 firefox!wmain+0x138
55 firefox!invoke_main+0x1d
56 firefox!__scrt_common_main_seh+0xff
57 KERNEL32!BaseThreadInitThunk+0x24
58 ntdll!__RtlUserThreadStart+0x2f
59 ntdll!_RtlUserThreadStart+0x1b

content process:

00 xul!mozilla::a11y::AccessibleWrap::GetChildIDFor+0x61
01 xul!mozilla::a11y::ia2Accessible::get_uniqueID+0x7a
02 ole32!CallFrame::Invoke+0x63
03 xul!`anonymous namespace'::HandoffRunnable::Run+0x1e
04 xul!`anonymous namespace'::SyncRunnable::Run+0x20
05 xul!mozilla::mscom::MainThreadInvoker::MainThreadAPC+0xa5
06 ntdll!RtlDispatchAPC+0x6a195
07 ntdll!KiUserApcDispatcher+0x2d
08 USER32!MsgWaitForMultipleObjectsEx+0x17b
09 combase!CCliModalLoop::BlockFn+0x11c
0a combase!ClassicSTAThreadWaitForHandles+0xb4
0b combase!CoWaitForMultipleHandles+0x8e
0c xul!mozilla::ipc::MessageChannel::WaitForSyncNotifyWithA11yReentry+0x107
0d xul!mozilla::ipc::MessageChannel::WaitForSyncNotify+0xed
0e xul!mozilla::ipc::MessageChannel::Send+0x909
0f xul!mozilla::a11y::PDocAccessibleChild::SendShowEvent+0x133
10 xul!mozilla::a11y::DocAccessibleChildBase::ShowEvent+0xb0
11 xul!mozilla::a11y::Accessible::HandleAccEvent+0x141
12 xul!mozilla::a11y::AccessibleWrap::HandleAccEvent+0x15
13 xul!mozilla::a11y::HyperTextAccessibleWrap::HandleAccEvent+0xa0
14 xul!nsEventShell::FireEvent+0x156
15 xul!mozilla::a11y::EventTree::Process+0x2a0
16 xul!mozilla::a11y::EventTree::Process+0x42
17 xul!mozilla::a11y::NotificationController::WillRefresh+0xba3
18 xul!nsRefreshDriver::Tick+0x406
19 xul!mozilla::RefreshDriverTimer::TickDriver+0x81
1a xul!mozilla::RefreshDriverTimer::TickRefreshDrivers+0xad
1b xul!mozilla::RefreshDriverTimer::Tick+0x144
1c xul!mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers+0x97
1d xul!mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver+0x1bc
1e xul!mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync+0x153
1f xul!mozilla::layout::VsyncChild::RecvNotify+0x111
20 xul!mozilla::layout::PVsyncChild::OnMessageReceived+0x1d3
21 xul!mozilla::ipc::PBackgroundChild::OnMessageReceived+0x81
22 xul!mozilla::ipc::MessageChannel::DispatchAsyncMessage+0xf0
23 xul!mozilla::ipc::MessageChannel::DispatchMessageW+0x1b3
24 xul!mozilla::ipc::MessageChannel::OnMaybeDequeueOne+0x104
25 xul!mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void)>+0x9
26 xul!mozilla::detail::RunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void)>+0x1f
27 xul!mozilla::detail::RunnableMethodImpl<bool (__thiscall mozilla::ipc::MessageChannel::*)(void),0,1>::Run+0x30
28 xul!mozilla::ipc::MessageChannel::RefCountedTask::Run+0x25
29 xul!mozilla::ipc::MessageChannel::DequeueTask::Run+0x27
2a xul!nsThread::ProcessNextEvent+0x336
2b xul!NS_ProcessNextEvent+0x62
2c xul!mozilla::ipc::MessagePump::Run+0x1ec
2d xul!mozilla::ipc::MessagePumpForChildProcess::Run+0x1d4
2e xul!MessageLoop::RunInternal+0x4d
2f xul!MessageLoop::RunHandler+0x82
30 xul!MessageLoop::Run+0x1d
31 xul!nsBaseAppShell::Run+0x50
32 xul!nsAppShell::Run+0x23
33 xul!XRE_RunAppShell+0x7d
34 xul!mozilla::ipc::MessagePumpForChildProcess::Run+0x87
35 xul!MessageLoop::RunInternal+0x4d
36 xul!MessageLoop::RunHandler+0x82
37 xul!MessageLoop::Run+0x1d
38 xul!XRE_InitChildProcess+0x97c
39 firefox!content_process_main+0x9f
3a firefox!NS_internal_main+0xf2
3b firefox!wmain+0x138
3c firefox!invoke_main+0x1d
3d firefox!__scrt_common_main_seh+0xff
3e KERNEL32!BaseThreadInitThunk+0x24
3f ntdll!__RtlUserThreadStart+0x2f
40 ntdll!_RtlUserThreadStart+0x1b

Comment 36

[(No longer employed by Mozilla) Aaron Klotz]

Would it help if the parent process shoved the incoming event into the NotificationController instead of directly firing the event?

Comment 37

[(No longer employed by Mozilla) Aaron Klotz]

Comment 36

Comment 38

[Trevor Saunders (:tbsaunde)]

(In reply to Aaron Klotz [:aklotz] from comment #34)
> (In reply to Trevor Saunders (:tbsaunde) from comment #33)
> > (In reply to James Teh [:Jamie] from comment #31)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #30)
> > > > My proposal is basically this, afaik there's no reason to prefer the id
> > > > generated in the parent to the one generated in the child. 
> > > 
> > > As I understand it, the browser chrome and content documents both use the
> > > same HWND. Unique ids need to be unique within the HWND. If you use ids from
> > > the child, they might conflict with ids generated for browser chrome in the
> > > parent.
> 
> Yes, in general I agree with Jamie's remarks: We need a "single namespace,"
> if you will, for child IDs. Whether they come entirely from the parent or
> entirely from the child is mostly irrelevant except for the fact that chrome
> accessibles that comprise the root of the tree already have chrome-generated
> ids. Furthermore, multi-process e10s would introduce even more potential for
> collisions. It follows that parent-generated IDs are our only option.

Well, I think there is another option, we have 2^32 possible ids.  If we are willing to restrict the number of id available to each process we can assign each process a bucket of ids it may allocate from and then always use the win64 id generation stuff to generate within that range.  So the parent process must always generate id in the range 0 - 0x10000000 the first child process has the range 0x10000001 to 0x20000000 etc.

(In reply to Aaron Klotz [:aklotz] from comment #36)
> Would it help if the parent process shoved the incoming event into the
> NotificationController instead of directly firing the event?

Well, Notification controllers are per document, so basically per window in the main process.  Anyway I guess you could have a per window queue of windows events to fire, but we'd need to worry about in process event listeners that expect to be called sync.  It also seems to me while that would make this bug less likely it wouldn't actually solve the racey part,  Since the notification controller could fire its queue before the child handles the message return.  Though I guess you could add a message and block firing the events on that message.

Comment 39

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Allocate child ids in chrome and ensure propagation to content (r2)

OK, so I've restructured this a bit so that DocAccessibleChild::ShowEvent sends two IPC messages:

1) The first one calls SendShowEventInfo but this just does the prep work of adding the subtree and retrieves the MSAA IDs;
2) The second one is a SendEvent message that actually dispatches the native event.

I think that's the only way that we're going to be able to get the MSAA IDs into content prior to dispatching the native event, while still dispatching the show event synchronously in the parent.

Comment 40

[Trevor Saunders (:tbsaunde)]

(In reply to Aaron Klotz [:aklotz] from comment #39)
> Created attachment 8790493 [details] [diff] [review]
> Part 1 - Allocate child ids in chrome and ensure propagation to content (r2)
> 
> OK, so I've restructured this a bit so that DocAccessibleChild::ShowEvent
> sends two IPC messages:
> 
> 1) The first one calls SendShowEventInfo but this just does the prep work of
> adding the subtree and retrieves the MSAA IDs;
> 2) The second one is a SendEvent message that actually dispatches the native
> event.
> 
> I think that's the only way that we're going to be able to get the MSAA IDs
> into content prior to dispatching the native event, while still dispatching
> the show event synchronously in the parent.

not very nice, but it seems "reasonable".  At some point I'd like to consider the per process id pools idea, but that might be more involved than this.

Comment 41

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8790493 [details] [diff] [review]
Part 1 - Allocate child ids in chrome and ensure propagation to content (r2)

Sorry about the lag here, this stuff was complicated before, and now its kind of worse :/

>+/* static */ void
>+DocAccessibleChildBase::SetMsaaIds(Accessible* aRoot,
>+                                   uint32_t& aMsaaIdIndex,

I think I prefer the take a int and return a value instead of being cleaver with references, but whatever I guess.

>+                                   const nsTArray<MsaaMapping>& aNewMsaaIds)
>+{
>+  uint64_t id = reinterpret_cast<uint64_t>(aRoot->UniqueID());

DebugOnly?

Comment 42

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Allocate child ids in chrome and ensure propagation to content (r3)

Carrying forward r+

Comment 43

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 - Propagate HWND to TabChild (r2)

Carrying forward r+

Comment 44

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3 - Make AccessibleWrap get hwnd from TabChild (r2)

Carrying forward r+

Comment 45

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d515669d3e0a8de7403756b530899f26ae708f6
Bug 1297549: Part 1 - Use chrome-generated MSAA IDs in content; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/c643278d88c8f8e214b9224c2e1f0a661d6dfab8
Bug 1297549: Part 2 - Propagate changes in tab native window down to content for a11y; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f82cedfb27bb9a0b78fa10d99ec384538745c0
Bug 1297549: Part 3 - Modify Windows AccessibleWrap to get hwnd from TabChild under e10s; r=tbsaunde

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e3fa462347c8 for mn-e10s:

https://treeherder.mozilla.org/logviewer.html#?job_id=35946311&repo=mozilla-inbound

Comment 47

[(No longer employed by Mozilla) Aaron Klotz]

WTF?! The firing assertion is literally MOZ_ASSERT(XRE_IsContentProcess()), in the CONTENT process!

Is Marionette not setting this correctly or something?

*sigh* Time to run more tests...

Comment 48

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5643f6aebe251235398917776fe9441f745adf97
Bug 1297549: Part 1 - Use chrome-generated MSAA IDs in content; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1cfae63f7901cd551777588077f3003c3031320
Bug 1297549: Part 2 - Propagate changes in tab native window down to content for a11y; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/1970274b88905bc56d2f938aa027ec8a1f2a1e78
Bug 1297549: Part 3 - Modify Windows AccessibleWrap to get hwnd from TabChild under e10s; r=tbsaunde

Comment 49

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5643f6aebe25
https://hg.mozilla.org/mozilla-central/rev/d1cfae63f790
https://hg.mozilla.org/mozilla-central/rev/1970274b8890

Comment 50

[(No longer employed by Mozilla) Aaron Klotz]

So there is another a11yr spike with this landing. I think that Trevor's idea of partitioning the child id namespace based on content process has merit, but I also think that it is going to take some time to figure out the details. Part 1 of this patch is a correctness stopgap until we can talk to the multi-e10s folks to sort that out.

Comment 51

[Phil Ringnalda (:philor)]

At least you didn't get a Win8 e10s a11yr spike with it, since it broke that :)

https://treeherder.mozilla.org/logviewer.html#?job_id=36061430&repo=mozilla-inbound (the "this.tabs is undefined" isn't the actual failure, that's constant false-positive noise).

Backed out in https://hg.mozilla.org/mozilla-central/rev/de404845c4f7 https://hg.mozilla.org/mozilla-central/rev/66ab506ce61f https://hg.mozilla.org/mozilla-central/rev/f0f15b7c6aa7

Comment 52

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Phil Ringnalda (:philor) from comment #51)
> At least you didn't get a Win8 e10s a11yr spike with it, since it broke that
> :)

To clarify, a11yr was outright timing out on Win8 x64.

Comment 53

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for backing this out :philor, I just saw the perma fail for win8 talos other-e10s  as seen in the spike of reported failures in bug 1052467.

Comment 54

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10611fcdd32e0592397302afae63f842359394cb
Bug 1297549: Part 1 - Use chrome-generated MSAA IDs in content; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/345334eff0212ecb78a29a65c62b1e95d0279f3a
Bug 1297549: Part 2 - Propagate changes in tab native window down to content for a11y; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/03e22d3b2f43eb39b60720244728d98ef21c6408
Bug 1297549: Part 3 - Modify Windows AccessibleWrap to get hwnd from TabChild under e10s; r=tbsaunde

Comment 55

[(No longer employed by Mozilla) Aaron Klotz]

The Talos hang was caused by the content process attempting to call RemoveID on the 64-bit MSAA ID generator. That should not be done when this patch is applied because the ID was generated by chrome, not content.

Comment 56

[Ting-Yu Chou [:ting] (away)]

Please be noted there're crashes with signatures:

 [@mozilla::a11y::IDSet::ReleaseID], and
 [@InvalidArrayIndex_CRASH | mozilla::a11y::DocAccessibleChildBase::SetMsaaIds]

which are #3 and #5 of 0918 Nightly on Windows. I didn't file bugs for the crashes because it seems you already have fixes in inbound.

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10611fcdd32e
https://hg.mozilla.org/mozilla-central/rev/345334eff021
https://hg.mozilla.org/mozilla-central/rev/03e22d3b2f43

Comment 58

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Allocate child ids in chrome and ensure propagation to content (r4)

Part 1 as landed.

Comment 59

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 - Propagate HWND to TabChild (r3)

Part 2 as landed

Comment 60

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3 - Make AccessibleWrap get hwnd from TabChild (r3)

Part 3 as landed

Comment 61

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bug 1297549 seems to be about this. It's for the UpdateNativeWindowHandle message. Not sure if it's been fixed already or not.

Comment 62

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oops. Meant bug 1303972.

Comment 63

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=431b0b109aea

Comment 64

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8792951 [details] [diff] [review]
Part 2 - Propagate HWND to TabChild (r3)

Approval Request Comment
[Feature/regressing bug #]: a11y for Windows e10s
[User impact if declined]: We will not be able to enable this feature in 51
[Describe test coverage new/current, TreeHerder]: Currently on Nightly 52 and is working with the NVDA screen reader.
[Risks and why]: Low; this had already landed on 51 but was backed out at the last moment. It has since landed on 52 and is running smoothly.
[String/UUID change made/needed]: None

Comment 65

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8792952 [details] [diff] [review]
Part 3 - Make AccessibleWrap get hwnd from TabChild (r3)

Approval Request Comment
[Feature/regressing bug #]: a11y for Windows e10s
[User impact if declined]: We will not be able to enable this feature in 51
[Describe test coverage new/current, TreeHerder]: Currently on Nightly 52 and is working with the NVDA screen reader.
[Risks and why]: Low; this had already landed on 51 but was backed out at the last moment. It has since landed on 52 and is running smoothly.
[String/UUID change made/needed]: None

Comment 66

[(No longer employed by Mozilla) Aaron Klotz]

I am intentionally not requesting uplift for part 1 of this bug as bug 1304449 implements the better long-term solution.

Comment 67

[Gerry Chang [:gchang]]

Hi Aaron,
Can the patches of Part 2 & 3 be separated from the fix of bug 1304449?

Comment 68

[(No longer employed by Mozilla) Aaron Klotz]

They already are separate and can be landed completely independently.

Comment 69

[Gerry Chang [:gchang]]

Comment on attachment 8792951 [details] [diff] [review]
Part 2 - Propagate HWND to TabChild (r3)

Fix an a11y issue. Take it in 51 aurora.

Comment 70

[Carsten Book [:Tomcat]]

part 3 has problems to apply to aurora, can you take a look, thanks!

Comment 71

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffb4d42e6f5d
https://hg.mozilla.org/releases/mozilla-aurora/rev/21edb0642a6a

Comment 72

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This broke Windows builds like https://treeherder.mozilla.org/logviewer.html#?job_id=3668994&repo=mozilla-aurora

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/515ab50a00a2

Comment 73

[(No longer employed by Mozilla) Aaron Klotz]

Fixed a missing include.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef0d139bf62d

Comment 74

[u554753]

This issue is reproducible in the newest Nightly with JAWS screenreader

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
JAWS Version: 18.0.2738

Steps to Reproduce:
1. Open http://www.wikipedia.org/ or relevant website
2. Navigate to an article 
3. Press tab key and observe behavior
4. Press arrow keys and observe behavior of the screenreader

Comment 75

[(No longer employed by Mozilla) Aaron Klotz]

It's going to be a separate issue with JAWS.

Comment 76

[Jim Mathies [:jimm]]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #74)
> This issue is reproducible in the newest Nightly with JAWS screenreader
> 
> User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101
> Firefox/55.0
> JAWS Version: 18.0.2738
> 
> Steps to Reproduce:
> 1. Open http://www.wikipedia.org/ or relevant website
> 2. Navigate to an article 
> 3. Press tab key and observe behavior
> 4. Press arrow keys and observe behavior of the screenreader

Hey Grover, could you please clone this out to a JAWS specific bug and hook it up to the jaws track @ bug 1350984? Thanks!

Comment 77

[u554753]

Bug 1363881 filed.