Bug 1416986 part 3: AccessibleHandler: Avoid cross-process QI calls for interfaces which we know don't exist. r?aklotz draft
authorJames Teh <jteh@mozilla.com>
Wed, 15 Nov 2017 12:28:45 +1000
changeset 701020 a9aff569687d53ce41a19983855436da1e7174db
parent 701019 6828ff2a766a5effe7b235e54766f5aecf5e9694
child 701021 29ada2f1b0d5746b12875820a8ae4564123c1c16
push id90030
push userbmo:jteh@mozilla.com
push dateTue, 21 Nov 2017 03:57:12 +0000
reviewersaklotz
bugs1416986
milestone59.0a1
Bug 1416986 part 3: AccessibleHandler: Avoid cross-process QI calls for interfaces which we know don't exist. r?aklotz The proxy manager caches interfaces marshaled in the payload and returns them on QI without a cross-process call. However, it doesn't know about interfaces which don't exist. We can determine this from the payload, since interfaces which don't exist will have a null pointer. We use this information to avoid querying the proxy in this case. MozReview-Commit-ID: FnzDetmTiPP
accessible/ipc/win/handler/AccessibleHandler.cpp
--- a/accessible/ipc/win/handler/AccessibleHandler.cpp
+++ b/accessible/ipc/win/handler/AccessibleHandler.cpp
@@ -24,17 +24,21 @@
 #include <uiautomation.h>
 #include <winreg.h>
 
 #include "AccessibleHypertext.h"
 #include "AccessibleHypertext2.h"
 #include "Accessible2_i.c"
 #include "Accessible2_2_i.c"
 #include "Accessible2_3_i.c"
+#include "AccessibleAction_i.c"
 #include "AccessibleHyperlink_i.c"
+#include "AccessibleTable_i.c"
+#include "AccessibleTable2_i.c"
+#include "AccessibleTableCell_i.c"
 
 namespace mozilla {
 namespace a11y {
 
 static mscom::Factory<AccessibleHandler> sHandlerFactory;
 
 HRESULT
 AccessibleHandler::Create(IUnknown* aOuter, REFIID aIid, void** aOutInterface)
@@ -205,16 +209,42 @@ AccessibleHandler::QueryHandlerInterface
   }
 
   if (aIid == IID_IServiceProvider) {
     RefPtr<IServiceProvider> svcProv(static_cast<IServiceProvider*>(this));
     svcProv.forget(aOutInterface);
     return S_OK;
   }
 
+  if (HasPayload()) {
+    // The proxy manager caches interfaces marshaled in the payload
+    // and returns them on QI without a cross-process call.
+    // However, it doesn't know about interfaces which don't exist.
+    // We can determine this from the payload.
+    if ((aIid == IID_IEnumVARIANT &&
+         !mCachedData.mStaticData.mIEnumVARIANT) ||
+        ((aIid == IID_IAccessibleText || aIid == IID_IAccessibleHypertext ||
+          aIid == IID_IAccessibleHypertext2) &&
+         !mCachedData.mStaticData.mIAHypertext) ||
+        ((aIid == IID_IAccessibleAction || aIid == IID_IAccessibleHyperlink) &&
+         !mCachedData.mStaticData.mIAHyperlink) ||
+        (aIid == IID_IAccessibleTable &&
+         !mCachedData.mStaticData.mIATable) ||
+        (aIid == IID_IAccessibleTable2 &&
+         !mCachedData.mStaticData.mIATable2) ||
+        (aIid == IID_IAccessibleTableCell &&
+         !mCachedData.mStaticData.mIATableCell)) {
+      // We already know this interface is not available, so don't query
+      // the proxy, thus avoiding a pointless cross-process call.
+      // If we return E_NOINTERFACE here, mscom::Handler will try the COM
+      // proxy. S_FALSE signals that the proxy should not be tried.
+      return S_FALSE;
+    }
+  }
+
   if (aIid == IID_IAccessibleText || aIid == IID_IAccessibleHypertext ||
       aIid == IID_IAccessibleHypertext2) {
     RefPtr<IAccessibleHypertext2> textTearoff(new AccessibleTextTearoff(this));
     textTearoff.forget(aOutInterface);
     return S_OK;
   }
 
   if (aIid == IID_IProvideClassInfo) {
@@ -236,23 +266,34 @@ AccessibleHandler::ReadHandlerPayload(IS
   mscom::StructFromStream deserializer(aStream);
   if (!deserializer) {
     return E_FAIL;
   }
   if (deserializer.IsEmpty()) {
     return S_FALSE;
   }
 
-  if (!deserializer.Read(&mCachedData, &IA2Payload_Decode)) {
+  // QueryHandlerInterface might get called while we deserialize the payload,
+  // but that checks the interface pointers in the payload to determine what
+  // interfaces are available. Therefore, deserialize into a temporary struct
+  // and update mCachedData only after deserialization completes.
+  // The decoding functions can misbehave if their target memory is not zeroed
+  // beforehand, so ensure we do that.
+  IA2Payload newData{};
+  if (!deserializer.Read(&newData, &IA2Payload_Decode)) {
     return E_FAIL;
   }
+  mCachedData = newData;
 
   // 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.
+  // However, we don't null them out because we use their presence
+  // to determine whether the interface is available
+  // so as to avoid pointless cross-proc QI calls returning E_NOINTERFACE.
   // 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();