Closed Bug 1297549 Opened 8 years ago Closed 8 years ago

Arrow keys will not read the page with a11y and e10s

Categories

(Core :: Disability Access APIs, defect)

51 Branch
x86_64
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
e10s + ---
firefox51 --- fixed
firefox52 --- fixed
firefox55 --- affected

People

(Reporter: j.lancett, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: aes+)

Attachments

(4 files, 8 obsolete files)

108.79 KB, text/x-log
Details
22.01 KB, patch
bugzilla
: review+
Details | Diff | Splinter Review
5.51 KB, patch
bugzilla
: review+
Details | Diff | Splinter Review
2.01 KB, patch
bugzilla
: review+
Details | Diff | Splinter Review
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
Blocks: 1258839
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
tracking-e10s: --- → ?
screenreader used is NVDA next 13513,9e4ea1d3 Fri Aug 19 04:49:42 2016 http://www.nvaccess.org/files/nvda/snapshots/
OS is windows 10 X64 build 14905
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: flyweb@mozilla.org Name: Multi-process staged rollout Version: 1.0 Enabled: true ID: e10srollout@mozilla.org 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: uBlock0@raymondhill.net Name: Web Compat Version: 1.0 Enabled: true ID: webcompat@mozilla.org Name: Google™ Hangouts Version: 0.1.6 Enabled: false ID: jid1-uqbSKwXpf2K6yl@jetpack Name: HTTPS-Everywhere Version: 5.2.1 Enabled: false ID: https-everywhere-eff@eff.org Name: Pocket Version: 1.0.4 Enabled: false ID: firefox@getpocket.com 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: displayport-tuning-nightly@experiments.mozilla.org Description: Measuring the effect of different displayport sizes on checkerboarding. Active: false End Date: 1457186284462 Homepage: Branch: group3 Sandbox ------- Content Process Sandbox Level: 2
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Disability Access APIs
Ever confirmed: true
Product: Firefox → Core
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.
Whiteboard: aes+
(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!
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.
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)!
> 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?
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.
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.
I'll see if I can make that API return the chrome-generated child id when running in e10s content.
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)
Flags: needinfo?(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.
Flags: needinfo?(jamie)
(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?
Flags: needinfo?(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?
Flags: needinfo?(jamie)
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.
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.
Yeah, things work much better now.
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.
Depends on: 1279086
(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/
No longer depends on: 1279086
Attached patch Propagate HWND to TabChild (obsolete) — Splinter Review
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.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8788542 - Flags: feedback?(jmathies)
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.
Attachment #8788542 - Flags: feedback?(jmathies) → feedback+
(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.
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.
Attachment #8789013 - Flags: review?(tbsaunde+mozbugs)
Attachment #8788542 - Attachment is obsolete: true
Attachment #8789015 - Flags: review?(jmathies)
Attachment #8789016 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8789016 [details] [diff] [review] Part 3 - Make AccessibleWrap get hwnd from TabChild ># HG changeset patch ># User Aaron Klotz <aklotz@mozilla.com> ># 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.
Attachment #8789016 - Flags: review?(tbsaunde+mozbugs) → review+
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?
Flags: needinfo?(aklotz)
(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 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
Attachment #8789015 - Flags: review?(jmathies) → review+
(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.
(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)...
Flags: needinfo?(aklotz)
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
Would it help if the parent process shoved the incoming event into the NotificationController instead of directly firing the event?
Flags: needinfo?(tbsaunde+mozbugs)
(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.
Flags: needinfo?(tbsaunde+mozbugs)
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.
Attachment #8789013 - Attachment is obsolete: true
Attachment #8789013 - Flags: review?(tbsaunde+mozbugs)
Attachment #8790493 - Flags: review?(tbsaunde+mozbugs)
(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 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?
Attachment #8790493 - Flags: review?(tbsaunde+mozbugs) → review+
Carrying forward r+
Attachment #8790493 - Attachment is obsolete: true
Attachment #8791794 - Flags: review+
Carrying forward r+
Attachment #8789015 - Attachment is obsolete: true
Attachment #8791795 - Flags: review+
Carrying forward r+
Attachment #8789016 - Attachment is obsolete: true
Attachment #8791796 - Flags: review+
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
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...
Flags: needinfo?(aklotz)
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
(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.
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.
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
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.
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.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Part 2 as landed
Attachment #8791795 - Attachment is obsolete: true
Attachment #8792951 - Flags: review+
Part 3 as landed
Attachment #8791796 - Attachment is obsolete: true
Attachment #8792952 - Flags: review+
Bug 1297549 seems to be about this. It's for the UpdateNativeWindowHandle message. Not sure if it's been fixed already or not.
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
Attachment #8792951 - Flags: approval-mozilla-aurora?
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
Attachment #8792952 - Flags: approval-mozilla-aurora?
I am intentionally not requesting uplift for part 1 of this bug as bug 1304449 implements the better long-term solution.
Hi Aaron, Can the patches of Part 2 & 3 be separated from the fix of bug 1304449?
Flags: needinfo?(aklotz)
They already are separate and can be landed completely independently.
Flags: needinfo?(aklotz)
Comment on attachment 8792951 [details] [diff] [review] Part 2 - Propagate HWND to TabChild (r3) Fix an a11y issue. Take it in 51 aurora.
Attachment #8792951 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8792952 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
part 3 has problems to apply to aurora, can you take a look, thanks!
Flags: needinfo?(aklotz)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It's going to be a separate issue with JAWS.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
(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!
Flags: needinfo?(gwimberly)
Flags: needinfo?(gwimberly)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: