nvda
6a4dd587 - Clear IA2InstallMap when terminating IA2 support. (#20291)

Commit
38 days ago
Clear IA2InstallMap when terminating IA2 support. (#20291) ### Link to issue number: Re #15862, #16020, https://bugzilla.mozilla.org/show_bug.cgi?id=1980754, https://bugzilla.mozilla.org/show_bug.cgi?id=2013244. ### Summary of the issue: Previously, when we terminated IA2 support, we didn't clear the map containing install data (IA2InstallMap). This could potentially cause problems when restarting NVDA if the dll didn't unload before the new copy of NVDA injected. In this case, the same instance of the dll would be used, including its global variables, which includes IA2InstallMap. This could potentially lead to several problems: 1. IA2Support_inProcess_initialize refuses to install IA2 if the map contains data. That could result in certain IA2 interface proxies becoming unregistered. This could be a potential cause of problems like #15862 (broken table navigation), #16020 (broken rich text editing) and broken native app selection mode. That said, at least one user has reported that this change doesn't fix the text editing problem for them. 2. IA2Support_inProcess_terminate tried to clean up using IA2InstallMap, even if IA2Support_inProcess_initialize refused to run because IA2InstallMap was populated by a previous instance of NVDA. That is, IA2Support_inProcess_terminate could run twice (or more!) with the same map data. This is particularly nasty because the map contains uiThreadHandle set by the installation code, which is closed by IA2Support_inProcess_terminate. That means we could be closing a handle which has been reused by something else; i.e. closing someone else's handle. This might explain the handle related crashes correlated to the presence of NVDA seen in Firefox bugs 1980754 and 2013244. ### Description of user facing changes: There's no solid evidence that this fixes any user facing problems at this stage. However, it is definitely a correctness bug that could lead to obscure problems. ### Description of development approach: Simply clear IA2InstallMap in IA2Support_inProcess_terminate. I also added comments to IA2Support_inProcess_initialize to explain why we can't simply treat the map already containing data as an error. ### Testing strategy: This issue is very difficult to reproduce reliably, so I ran a try build with this fix and asked several other users experiencing this problem to do the same. Unfortunately, one user reported that this did not fix the text editing issue for them, but nor did it cause any adverse effects. ### Known issues with pull request: None. Even if this doesn't fix any actual problem, it's still more correct and eliminates one variable from the problem space. ### Code Review Checklist: - [x] Documentation: - Change log entry - User Documentation - Developer / Technical Documentation - Context sensitive help for GUI changes - [x] Testing: - Unit tests - System (end to end) tests - Manual testing - [x] UX of all users considered: - Speech - Braille - Low Vision - Different web browsers - Localization in other languages / culture than English - [x] API is compatible with existing add-ons. - [x] Security precautions taken. --------- Co-authored-by: Michael Curran <mick@nvaccess.org>
Author
Parents
Loading