Bug 1416986 part 2: Include interfaces the client is likely to request in the accessible handler payload. r=aklotz a=gchang
authorJames Teh <jteh@mozilla.com>
Wed, 15 Nov 2017 12:18:18 +1000
changeset 442515 9d771f65f35d6bcfcd7a412aff6f60061482cbe9
parent 442514 ee921fb100b395920f8a5cdcb0c92f070d56aa33
child 442516 30014722ca5303b725978d66fb651273a0cbc85c
push id8239
push userarchaeopteryx@coole-files.de
push dateMon, 27 Nov 2017 10:55:23 +0000
treeherdermozilla-beta@66c82ad8b8a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz, gchang
bugs1416986
milestone58.0
Bug 1416986 part 2: Include interfaces the client is likely to request in the accessible handler payload. r=aklotz a=gchang Now that virtual buffers have to render across processes, we want to eliminate as many cross-process calls as possible. This includes QueryInterface calls, since buffers query for several interfaces on every node they visit. To avoid these cross-process QI calls, we include interfaces clients are likely to request in the handler payload. This way, they get marshaled in the single call used to retrieve the object. This patch does the following: 1. Passes the interceptor when building the payload. We need this so we can get interceptors for other interfaces. 2. Splits the payload into two main parts: a static part and a dynamic part. The (new) static part contains the interface pointers. The dynamic part contains the rest. This is necessary because the refresh call cannot pass the interceptor, but the interceptor is needed to build the static part. Also, re-building the static part is pointless when refreshing. 3. Includes the interface pointers in the payload (BuildStaticIA2Data). The pointers also have to be cleaned up after marshaling. 4. Releases the interface pointers in the handler after the payload is received. We do this because they're aggregated by the proxy manager as they're unmarshaled. MozReview-Commit-ID: 6VRLMNScgwW
accessible/ipc/win/HandlerProvider.cpp
accessible/ipc/win/HandlerProvider.h
accessible/ipc/win/handler/AccessibleHandler.cpp
accessible/ipc/win/handler/HandlerData.idl
--- a/accessible/ipc/win/HandlerProvider.cpp
+++ b/accessible/ipc/win/HandlerProvider.cpp
@@ -16,16 +16,17 @@
 #include "HandlerData.h"
 #include "HandlerData_i.c"
 #include "mozilla/Assertions.h"
 #include "mozilla/a11y/AccessibleWrap.h"
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/Move.h"
 #include "mozilla/mscom/AgileReference.h"
 #include "mozilla/mscom/FastMarshaler.h"
+#include "mozilla/mscom/Interceptor.h"
 #include "mozilla/mscom/MainThreadInvoker.h"
 #include "mozilla/mscom/Ptr.h"
 #include "mozilla/mscom/StructStream.h"
 #include "mozilla/mscom/Utils.h"
 #include "nsThreadUtils.h"
 
 #include <memory.h>
 
@@ -92,62 +93,65 @@ HandlerProvider::GetHandler(NotNull<CLSI
     return E_NOINTERFACE;
   }
 
   *aHandlerClsid = CLSID_AccessibleHandler;
   return S_OK;
 }
 
 void
-HandlerProvider::GetAndSerializePayload(const MutexAutoLock&)
+HandlerProvider::GetAndSerializePayload(const MutexAutoLock&,
+    NotNull<mscom::IInterceptor*> aInterceptor)
 {
   MOZ_ASSERT(mscom::IsCurrentThreadMTA());
 
   if (mSerializer) {
     return;
   }
 
   IA2Payload payload{};
 
-  if (!mscom::InvokeOnMainThread("HandlerProvider::BuildIA2Data",
-                                 this, &HandlerProvider::BuildIA2Data,
-                                 &payload.mData) ||
-      !payload.mData.mUniqueId) {
+  if (!mscom::InvokeOnMainThread("HandlerProvider::BuildInitialIA2Data",
+                                 this, &HandlerProvider::BuildInitialIA2Data,
+                                 aInterceptor,
+                                 &payload.mStaticData, &payload.mDynamicData) ||
+      !payload.mDynamicData.mUniqueId) {
     return;
   }
 
   // But we set mGeckoBackChannel on the current thread which resides in the
   // MTA. This is important to ensure that COM always invokes
   // IGeckoBackChannel methods in an MTA background thread.
 
   RefPtr<IGeckoBackChannel> payloadRef(this);
   // AddRef/Release pair for this reference is handled by payloadRef
   payload.mGeckoBackChannel = this;
 
   mSerializer = MakeUnique<mscom::StructToStream>(payload, &IA2Payload_Encode);
 
-  // Now that we have serialized payload, we should free any BSTRs that were
-  // allocated in BuildIA2Data.
-  ClearIA2Data(payload.mData);
+  // Now that we have serialized payload, we should clean up any
+  // BSTRs, interfaces, etc. fetched in BuildInitialIA2Data.
+  CleanupStaticIA2Data(payload.mStaticData);
+  CleanupDynamicIA2Data(payload.mDynamicData);
 }
 
 HRESULT
 HandlerProvider::GetHandlerPayloadSize(NotNull<mscom::IInterceptor*> aInterceptor,
                                        NotNull<DWORD*> aOutPayloadSize)
 {
   MOZ_ASSERT(mscom::IsCurrentThreadMTA());
 
   if (!IsTargetInterfaceCacheable()) {
     *aOutPayloadSize = mscom::StructToStream::GetEmptySize();
     return S_OK;
   }
 
   MutexAutoLock lock(mMutex);
 
-  GetAndSerializePayload(lock);
+  GetAndSerializePayload(lock, aInterceptor);
 
   if (!mSerializer || !(*mSerializer)) {
     // Failed payload serialization is non-fatal
     *aOutPayloadSize = mscom::StructToStream::GetEmptySize();
     return S_OK;
   }
 
   *aOutPayloadSize = mSerializer->GetSize();
@@ -177,17 +181,82 @@ public:
   ExecuteWhen& operator=(ExecuteWhen&&) = delete;
 
 private:
   CondFnT&  mCondFn;
   ExeFnT&   mExeFn;
 };
 
 void
-HandlerProvider::BuildIA2Data(IA2Data* aOutIA2Data)
+HandlerProvider::BuildStaticIA2Data(
+  NotNull<mscom::IInterceptor*> aInterceptor,
+  StaticIA2Data* aOutData)
+{
+  MOZ_ASSERT(aOutData);
+  MOZ_ASSERT(NS_IsMainThread());
+  MOZ_ASSERT(mTargetUnk);
+  MOZ_ASSERT(IsTargetInterfaceCacheable());
+
+  // Include interfaces the client is likely to request.
+  // This is cheap here and saves multiple cross-process calls later.
+  // These interfaces must be released in CleanupStaticIA2Data!
+
+  // If the target is already an IAccessible2, this pointer is redundant.
+  // However, the target might be an IAccessibleHyperlink, etc., in which
+  // case the client will almost certainly QI for IAccessible2.
+  HRESULT hr = aInterceptor->GetInterceptorForIID(NEWEST_IA2_IID,
+                                          (void**)&aOutData->mIA2);
+  if (FAILED(hr)) {
+    // IA2 should always be present, so something has
+    // gone very wrong if this fails.
+    aOutData->mIA2 = nullptr;
+    return;
+  }
+
+  // Some of these interfaces aren't present on all accessibles,
+  // so it's not a failure if these interfaces can't be fetched.
+  hr = aInterceptor->GetInterceptorForIID(IID_IEnumVARIANT,
+                                          (void**)&aOutData->mIEnumVARIANT);
+  if (FAILED(hr)) {
+    aOutData->mIEnumVARIANT = nullptr;
+  }
+
+  hr = aInterceptor->GetInterceptorForIID(IID_IAccessibleHypertext2,
+                                          (void**)&aOutData->mIAHypertext);
+  if (FAILED(hr)) {
+    aOutData->mIAHypertext = nullptr;
+  }
+
+  hr = aInterceptor->GetInterceptorForIID(IID_IAccessibleHyperlink,
+                                          (void**)&aOutData->mIAHyperlink);
+  if (FAILED(hr)) {
+    aOutData->mIAHyperlink = nullptr;
+  }
+
+  hr = aInterceptor->GetInterceptorForIID(IID_IAccessibleTable,
+                                          (void**)&aOutData->mIATable);
+  if (FAILED(hr)) {
+    aOutData->mIATable = nullptr;
+  }
+
+  hr = aInterceptor->GetInterceptorForIID(IID_IAccessibleTable2,
+                                          (void**)&aOutData->mIATable2);
+  if (FAILED(hr)) {
+    aOutData->mIATable2 = nullptr;
+  }
+
+  hr = aInterceptor->GetInterceptorForIID(IID_IAccessibleTableCell,
+                                          (void**)&aOutData->mIATableCell);
+  if (FAILED(hr)) {
+    aOutData->mIATableCell = nullptr;
+  }
+}
+
+void
+HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data)
 {
   MOZ_ASSERT(aOutIA2Data);
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(mTargetUnk);
   MOZ_ASSERT(IsTargetInterfaceCacheable());
 
   RefPtr<NEWEST_IA2_INTERFACE> target;
   HRESULT hr = mTargetUnk.get()->QueryInterface(NEWEST_IA2_IID,
@@ -198,17 +267,17 @@ HandlerProvider::BuildIA2Data(IA2Data* a
 
   hr = E_UNEXPECTED;
 
   auto hasFailed = [&hr]() -> bool {
     return FAILED(hr);
   };
 
   auto cleanup = [this, aOutIA2Data]() -> void {
-    ClearIA2Data(*aOutIA2Data);
+    CleanupDynamicIA2Data(*aOutIA2Data);
   };
 
   ExecuteWhen<decltype(hasFailed), decltype(cleanup)> onFail(hasFailed, cleanup);
 
   const VARIANT kChildIdSelf = {VT_I4};
   VARIANT varVal;
 
   hr = target->accLocation(&aOutIA2Data->mLeft, &aOutIA2Data->mTop,
@@ -286,20 +355,67 @@ HandlerProvider::BuildIA2Data(IA2Data* a
 
   // NB: get_uniqueID should be the final property retrieved in this method,
   // as its presence is used to determine whether the rest of this data
   // retrieval was successful.
   hr = target->get_uniqueID(&aOutIA2Data->mUniqueId);
 }
 
 void
-HandlerProvider::ClearIA2Data(IA2Data& aData)
+HandlerProvider::CleanupStaticIA2Data(StaticIA2Data& aData)
+{
+  // When CoMarshalInterface writes interfaces out to a stream, it AddRefs.
+  // Therefore, we must release our references after this.
+  if (aData.mIA2) {
+    aData.mIA2->Release();
+  }
+  if (aData.mIEnumVARIANT) {
+    aData.mIEnumVARIANT->Release();
+  }
+  if (aData.mIAHypertext) {
+    aData.mIAHypertext->Release();
+  }
+  if (aData.mIAHyperlink) {
+    aData.mIAHyperlink->Release();
+  }
+  if (aData.mIATable) {
+    aData.mIATable->Release();
+  }
+  if (aData.mIATable2) {
+    aData.mIATable2->Release();
+  }
+  if (aData.mIATableCell) {
+    aData.mIATableCell->Release();
+  }
+  ZeroMemory(&aData, sizeof(StaticIA2Data));
+}
+
+void
+HandlerProvider::CleanupDynamicIA2Data(DynamicIA2Data& aData)
 {
   ::VariantClear(&aData.mRole);
-  ZeroMemory(&aData, sizeof(IA2Data));
+  ZeroMemory(&aData, sizeof(DynamicIA2Data));
+}
+
+void
+HandlerProvider::BuildInitialIA2Data(
+  NotNull<mscom::IInterceptor*> aInterceptor,
+  StaticIA2Data* aOutStaticData,
+  DynamicIA2Data* aOutDynamicData)
+{
+  BuildStaticIA2Data(aInterceptor, aOutStaticData);
+  if (!aOutStaticData->mIA2) {
+    return;
+  }
+  BuildDynamicIA2Data(aOutDynamicData);
+  if (!aOutDynamicData->mUniqueId) {
+    // Building dynamic data failed, which means building the payload failed.
+    // However, we've already built static data, so we must clean this up.
+    CleanupStaticIA2Data(*aOutStaticData);
+  }
 }
 
 bool
 HandlerProvider::IsTargetInterfaceCacheable()
 {
   return MarshalAs(mTargetUnkIid) == NEWEST_IA2_IID ||
          mTargetUnkIid == IID_IAccessibleHyperlink;
 }
@@ -402,22 +518,22 @@ HandlerProvider::put_HandlerControl(long
                                  static_cast<DWORD>(aPid), Move(ptrProxy))) {
     return E_FAIL;
   }
 
   return S_OK;
 }
 
 HRESULT
-HandlerProvider::Refresh(IA2Data* aOutData)
+HandlerProvider::Refresh(DynamicIA2Data* aOutData)
 {
   MOZ_ASSERT(mscom::IsCurrentThreadMTA());
 
-  if (!mscom::InvokeOnMainThread("HandlerProvider::BuildIA2Data",
-                                 this, &HandlerProvider::BuildIA2Data,
+  if (!mscom::InvokeOnMainThread("HandlerProvider::BuildDynamicIA2Data",
+                                 this, &HandlerProvider::BuildDynamicIA2Data,
                                  aOutData)) {
     return E_FAIL;
   }
 
   return S_OK;
 }
 
 } // namespace a11y
--- a/accessible/ipc/win/HandlerProvider.h
+++ b/accessible/ipc/win/HandlerProvider.h
@@ -49,26 +49,33 @@ public:
   STDMETHODIMP_(REFIID) GetEffectiveOutParamIid(REFIID aCallIid,
                                                 ULONG aCallMethod) override;
   STDMETHODIMP NewInstance(REFIID aIid,
                            mscom::InterceptorTargetPtr<IUnknown> aTarget,
                            NotNull<mscom::IHandlerProvider**> aOutNewPayload) override;
 
   // IGeckoBackChannel
   STDMETHODIMP put_HandlerControl(long aPid, IHandlerControl* aCtrl) override;
-  STDMETHODIMP Refresh(IA2Data* aOutData) override;
+  STDMETHODIMP Refresh(DynamicIA2Data* aOutData) override;
 
 private:
   ~HandlerProvider() = default;
 
   void SetHandlerControlOnMainThread(DWORD aPid,
                                      mscom::ProxyUniquePtr<IHandlerControl> aCtrl);
-  void GetAndSerializePayload(const MutexAutoLock&);
-  void BuildIA2Data(IA2Data* aOutIA2Data);
-  static void ClearIA2Data(IA2Data& aData);
+  void GetAndSerializePayload(const MutexAutoLock&,
+                              NotNull<mscom::IInterceptor*> aInterceptor);
+  void BuildStaticIA2Data(NotNull<mscom::IInterceptor*> aInterceptor,
+                          StaticIA2Data* aOutData);
+  void BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data);
+  void BuildInitialIA2Data(NotNull<mscom::IInterceptor*> aInterceptor,
+                           StaticIA2Data* aOutStaticData,
+                           DynamicIA2Data* aOutDynamicData);
+  static void CleanupStaticIA2Data(StaticIA2Data& aData);
+  static void CleanupDynamicIA2Data(DynamicIA2Data& aData);
   bool IsTargetInterfaceCacheable();
 
   Atomic<uint32_t>                  mRefCnt;
   Mutex                             mMutex; // Protects mSerializer
   const IID                         mTargetUnkIid;
   mscom::InterceptorTargetPtr<IUnknown> mTargetUnk; // Constant, main thread only
   UniquePtr<mscom::StructToStream>  mSerializer;
   RefPtr<IUnknown>                  mFastMarshalUnk;
--- a/accessible/ipc/win/handler/AccessibleHandler.cpp
+++ b/accessible/ipc/win/handler/AccessibleHandler.cpp
@@ -142,17 +142,17 @@ AccessibleHandler::MaybeUpdateCachedData
   if (gen == mCacheGen) {
     return S_OK;
   }
 
   if (!mCachedData.mGeckoBackChannel) {
     return E_POINTER;
   }
 
-  return mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mData);
+  return mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mDynamicData);
 }
 
 HRESULT
 AccessibleHandler::ResolveIDispatch()
 {
   if (mDispatch) {
     return S_OK;
   }
@@ -240,16 +240,44 @@ AccessibleHandler::ReadHandlerPayload(IS
   if (deserializer.IsEmpty()) {
     return S_FALSE;
   }
 
   if (!deserializer.Read(&mCachedData, &IA2Payload_Decode)) {
     return E_FAIL;
   }
 
+  // These interfaces have been aggregated into the proxy manager.
+  // The proxy manager will resolve these interfaces now on QI,
+  // so we can release these pointers.
+  // Note that if pointers to other objects (in contrast to
+  // interfaces of *this* object) are added in future, we should not release
+  // those pointers.
+  if (mCachedData.mStaticData.mIA2) {
+    mCachedData.mStaticData.mIA2->Release();
+  }
+  if (mCachedData.mStaticData.mIEnumVARIANT) {
+    mCachedData.mStaticData.mIEnumVARIANT->Release();
+  }
+  if (mCachedData.mStaticData.mIAHypertext) {
+    mCachedData.mStaticData.mIAHypertext->Release();
+  }
+  if (mCachedData.mStaticData.mIAHyperlink) {
+    mCachedData.mStaticData.mIAHyperlink->Release();
+  }
+  if (mCachedData.mStaticData.mIATable) {
+    mCachedData.mStaticData.mIATable->Release();
+  }
+  if (mCachedData.mStaticData.mIATable2) {
+    mCachedData.mStaticData.mIATable2->Release();
+  }
+  if (mCachedData.mStaticData.mIATableCell) {
+    mCachedData.mStaticData.mIATableCell->Release();
+  }
+
   if (!mCachedData.mGeckoBackChannel) {
     return S_OK;
   }
 
   RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
   if (!ctl) {
     return E_OUTOFMEMORY;
   }
@@ -406,22 +434,22 @@ CopyBSTR(BSTR aSrc)
     HRESULT hr; \
     if (FAILED(hr = MaybeUpdateCachedData())) { \
       return hr; \
     } \
   }
 
 #define GET_FIELD(member, assignTo) \
   { \
-    assignTo = mCachedData.mData.member; \
+    assignTo = mCachedData.mDynamicData.member; \
   }
 
 #define GET_BSTR(member, assignTo) \
   { \
-    assignTo = CopyBSTR(mCachedData.mData.member); \
+    assignTo = CopyBSTR(mCachedData.mDynamicData.member); \
   }
 
 /*** IAccessible ***/
 
 HRESULT
 AccessibleHandler::get_accParent(IDispatch **ppdispParent)
 {
   HRESULT hr = ResolveIA2();
@@ -542,17 +570,17 @@ AccessibleHandler::get_accRole(VARIANT v
     HRESULT hr = ResolveIA2();
     if (FAILED(hr)) {
       return hr;
     }
     return mIA2PassThru->get_accRole(varChild, pvarRole);
   }
 
   BEGIN_CACHE_ACCESS;
-  return ::VariantCopy(pvarRole, &mCachedData.mData.mRole);
+  return ::VariantCopy(pvarRole, &mCachedData.mDynamicData.mRole);
 }
 
 
 HRESULT
 AccessibleHandler::get_accState(VARIANT varChild, VARIANT *pvarState)
 {
   if (!pvarState) {
     return E_INVALIDARG;
@@ -914,17 +942,17 @@ AccessibleHandler::get_uniqueID(long* un
   }
   if (!HasPayload()) {
     HRESULT hr = ResolveIA2();
     if (FAILED(hr)) {
       return hr;
     }
     return mIA2PassThru->get_uniqueID(uniqueID);
   }
-  *uniqueID = mCachedData.mData.mUniqueId;
+  *uniqueID = mCachedData.mDynamicData.mUniqueId;
   return S_OK;
 }
 
 HRESULT
 AccessibleHandler::get_windowHandle(HWND* windowHandle)
 {
   if (!windowHandle) {
     return E_INVALIDARG;
--- a/accessible/ipc/win/handler/HandlerData.idl
+++ b/accessible/ipc/win/handler/HandlerData.idl
@@ -2,22 +2,39 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * 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 "mozilla-config.h"
 #include "AccessibleHandler.h"
 
+import "oaidl.idl";
 import "ocidl.idl";
 import "ServProv.idl";
 
-import "AccessibleText.idl";
+import "Accessible2_3.idl";
+import "AccessibleHypertext2.idl";
+import "AccessibleHyperlink.idl";
+import "AccessibleTable.idl";
+import "AccessibleTable2.idl";
+import "AccessibleTableCell.idl";
 
-typedef struct _IA2Data
+typedef struct _StaticIA2Data
+{
+  NEWEST_IA2_INTERFACE* mIA2;
+  IEnumVARIANT* mIEnumVARIANT;
+  IAccessibleHypertext2* mIAHypertext;
+  IAccessibleHyperlink* mIAHyperlink;
+  IAccessibleTable* mIATable;
+  IAccessibleTable2* mIATable2;
+  IAccessibleTableCell* mIATableCell;
+} StaticIA2Data;
+
+typedef struct _DynamicIA2Data
 {
   VARIANT           mRole;
   long              mState;
   long              mChildCount;
   long              mIA2Role;
   AccessibleStates  mIA2States;
   long              mLeft;
   long              mTop;
@@ -27,17 +44,17 @@ typedef struct _IA2Data
   BSTR              mKeyboardShortcut;
   BSTR              mName;
   BSTR              mDescription;
   BSTR              mDefaultAction;
   BSTR              mValue;
   BSTR              mAttributes;
   IA2Locale         mIA2Locale;
   long              mUniqueId;
-} IA2Data;
+} DynamicIA2Data;
 
 interface IGeckoBackChannel;
 
 // We define different CLSIDs and IIDs depending on channel and officiality.
 // This prevents handlers from installing overtop one another when multiple
 // channels are present. Note that we do not do this for all UUIDs in this IDL,
 // just the ones that are written to the registry (coclass and interfaces that
 // have the [object] annotation)
@@ -95,17 +112,18 @@ interface IGeckoBackChannel;
 
 #endif
 
 [uuid(2b0e83b3-fd1a-443f-9ed6-c00d39055b58)]
 interface HandlerData
 {
   typedef struct _IA2Payload
   {
-    IA2Data mData;
+    StaticIA2Data mStaticData;
+    DynamicIA2Data mDynamicData;
     IGeckoBackChannel* mGeckoBackChannel;
   } IA2Payload;
 }
 
 [object,
  uuid(IHANDLERCONTROL_IID),
  async_uuid(ASYNCIHANDLERCONTROL_IID),
  pointer_default(unique)]
@@ -118,17 +136,17 @@ interface IHandlerControl : IUnknown
 }
 
 [object,
  uuid(IGECKOBACKCHANNEL_IID),
  pointer_default(unique)]
 interface IGeckoBackChannel : IUnknown
 {
   [propput] HRESULT HandlerControl([in] long aPid, [in] IHandlerControl* aCtrl);
-  HRESULT Refresh([out] IA2Data* aOutData);
+  HRESULT Refresh([out] DynamicIA2Data* aOutData);
 }
 
 [uuid(1e545f07-f108-4912-9471-546827a80983)]
 library AccessibleHandlerTypeLib
 {
   /**
    * This definition is required in order for the handler implementation to
    * support IDispatch (aka Automation). This is used by interpreted language