Bug 1642141: When clearing the AccessibleHandlerControl Accessible cache, swap it into a temporary map first to avoid re-entry into the main map while clearing. r=MarcoZ, a=jcristau
authorJames Teh <jteh@mozilla.com>
Mon, 01 Jun 2020 05:45:44 +0000
changeset 596949 cf1cee074da0714d577339f4a272412f350f0e0a
parent 596948 a81711950f481af92cb735ed53aa05b364bde48d
child 596950 39d996685740f052ecee030ee4012a382945b645
push id13215
push userjcristau@mozilla.com
push dateThu, 04 Jun 2020 13:33:19 +0000
treeherdermozilla-beta@d6fc47eb7f3b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMarcoZ, jcristau
bugs1642141
milestone78.0
Bug 1642141: When clearing the AccessibleHandlerControl Accessible cache, swap it into a temporary map first to avoid re-entry into the main map while clearing. r=MarcoZ, a=jcristau Differential Revision: https://phabricator.services.mozilla.com/D77623
accessible/ipc/win/handler/AccessibleHandlerControl.cpp
accessible/ipc/win/handler/AccessibleHandlerControl.h
--- a/accessible/ipc/win/handler/AccessibleHandlerControl.cpp
+++ b/accessible/ipc/win/handler/AccessibleHandlerControl.cpp
@@ -113,17 +113,28 @@ AccessibleHandlerControl::AccessibleHand
   MOZ_ASSERT(mIA2Proxy);
 }
 
 IMPL_IUNKNOWN1(AccessibleHandlerControl, IHandlerControl)
 
 HRESULT
 AccessibleHandlerControl::Invalidate() {
   ++mCacheGen;
-  mAccessibleCache.clear();
+  // We can't just call mAccessibleCache.clear() because doing so would release
+  // remote objects, making remote COM calls. Since this is an STA, an incoming
+  // COM call might be handled which might marshal an AccessibleHandler,
+  // which in turn might add itself to mAccessibleCache. Since we'd be in the
+  // middle of mutating mAccessibleCache, that might cause a crash. Instead,
+  // swap mAccessibleCache into a temporary map first, which will empty
+  // mAccessibleCache without releasing remote objects. Once mAccessibleCache
+  // is empty, it's safe to let the temporary map be destroyed when it goes
+  // out of scope. Remote calls will be made, but nothing will re-enter
+  // the temporary map while it's being destroyed.
+  AccessibleCache oldCache;
+  mAccessibleCache.swap(oldCache);
   return S_OK;
 }
 
 HRESULT
 AccessibleHandlerControl::OnTextChange(long aHwnd, long aIA2UniqueId,
                                        VARIANT_BOOL aIsInsert,
                                        IA2TextSegment* aText) {
   if (!aText) {
--- a/accessible/ipc/win/handler/AccessibleHandlerControl.h
+++ b/accessible/ipc/win/handler/AccessibleHandlerControl.h
@@ -80,17 +80,18 @@ class AccessibleHandlerControl final : p
   ~AccessibleHandlerControl() = default;
 
   bool mIsRegistered;
   uint32_t mCacheGen;
   detail::TextChange mTextChange;
   UniquePtr<mscom::RegisteredProxy> mIA2Proxy;
   UniquePtr<mscom::RegisteredProxy> mHandlerProxy;
   // We can't use Gecko APIs in this dll, hence the use of std::unordered_map.
-  std::unordered_map<long, RefPtr<AccessibleHandler>> mAccessibleCache;
+  typedef std::unordered_map<long, RefPtr<AccessibleHandler>> AccessibleCache;
+  AccessibleCache mAccessibleCache;
 };
 
 extern mscom::SingletonFactory<AccessibleHandlerControl> gControlFactory;
 
 }  // namespace a11y
 }  // namespace mozilla
 
 #endif  // mozilla_a11y_AccessibleHandlerControl_h