Bug 1654969: Always disable COM pings for mscom::FastMarshaler (and thus mscom::Interceptor). r=aklotz
authorJames Teh <jteh@mozilla.com>
Wed, 29 Jul 2020 21:11:14 +0000
changeset 542562 8274426a159c9b88ac440ac6c64e083fb79d713c
parent 542561 9b7136d538b6789eeeff6401893b8379b0a36c43
child 542563 bc16528bfcb87d54c73a9affcbef30e097665816
push id122956
push userjteh@mozilla.com
push dateThu, 30 Jul 2020 01:55:10 +0000
treeherderautoland@8274426a159c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz
bugs1654969, 1627084
milestone81.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1654969: Always disable COM pings for mscom::FastMarshaler (and thus mscom::Interceptor). r=aklotz Previously, we only did this when IsCallerExternalProcess() returned false. There are three reasons for changing this: 1. There seem to be cases where IsCallerExternalProcess() returns true even when marshaling for a COM query in the MTA. 2. After bug 1627084, we pre-build a11y handler payloads on the main thread for bulk fetch calls. That will marshal interceptors. However, IsCallerExternalProcess() can't work in that case because it's not running on the thread on which the COM call is being handled. 3. If MSHLFLAGS_NOPING is used, Release calls from remote clients are never sent to the server. So, as soon as we use NOPING for our parent process, we're already going to leak references, even if we don't use NOPING for external callers. Put another way, as soon as we use NOPING for one caller, we may as well use it for all callers because COM pinging will never release the object anyway. Differential Revision: https://phabricator.services.mozilla.com/D84778
ipc/mscom/FastMarshaler.cpp
ipc/mscom/FastMarshaler.h
ipc/mscom/Interceptor.h
--- a/ipc/mscom/FastMarshaler.cpp
+++ b/ipc/mscom/FastMarshaler.cpp
@@ -86,21 +86,16 @@ FastMarshaler::InternalRelease() {
 
 DWORD
 FastMarshaler::GetMarshalFlags(DWORD aDestContext, DWORD aMshlFlags) {
   // Only worry about local contexts.
   if (aDestContext != MSHCTX_LOCAL) {
     return aMshlFlags;
   }
 
-  if (IsCallerExternalProcess()) {
-    return aMshlFlags;
-  }
-
-  // The caller is our parent main thread. Disable ping functionality.
   return aMshlFlags | MSHLFLAGS_NOPING;
 }
 
 HRESULT
 FastMarshaler::GetUnmarshalClass(REFIID riid, void* pv, DWORD dwDestContext,
                                  void* pvDestContext, DWORD mshlflags,
                                  CLSID* pCid) {
   if (!mStdMarshalWeak) {
--- a/ipc/mscom/FastMarshaler.h
+++ b/ipc/mscom/FastMarshaler.h
@@ -12,23 +12,26 @@
 #include "mozilla/RefPtr.h"
 
 #include <objidl.h>
 
 namespace mozilla {
 namespace mscom {
 
 /**
- * When we are marshaling to the parent process main thread, we want to turn
- * off COM's ping functionality. That functionality is designed to free
- * strong references held by defunct client processes. Since our content
- * processes cannot outlive our parent process, we turn off pings when we know
- * that the COM client is going to be our parent process. This provides a
- * significant performance boost in a11y code due to large numbers of remote
- * objects being created and destroyed within a short period of time.
+ * COM ping functionality is enabled by default and is designed to free strong
+ * references held by defunct client processes. However, this incurs a
+ * significant performance penalty in a11y code due to large numbers of remote
+ * objects being created and destroyed within a short period of time. Thus, we
+ * turn off pings to improve performance.
+ * ACHTUNG! When COM pings are disabled, Release calls from remote clients are
+ * never sent to the server! If you use this marshaler, you *must* explicitly
+ * disconnect clients using CoDisconnectObject when the object is no longer
+ * relevant. Otherwise, references to the object will never be released, causing
+ * a leak.
  */
 class FastMarshaler final : public IMarshal {
  public:
   static HRESULT Create(IUnknown* aOuter, IUnknown** aOutMarshalerUnk);
 
   // IMarshal
   STDMETHODIMP GetUnmarshalClass(REFIID riid, void* pv, DWORD dwDestContext,
                                  void* pvDestContext, DWORD mshlflags,
--- a/ipc/mscom/Interceptor.h
+++ b/ipc/mscom/Interceptor.h
@@ -58,16 +58,21 @@ struct IInterceptor : public IUnknown {
  * function, which instantiates an ICallInterceptor. Note, however, that
  * ICallInterceptor only works on a single interface; we need to be able to
  * interpose QueryInterface calls so that we can instantiate a new
  * ICallInterceptor for each new interface that is requested.
  *
  * We accomplish this by using COM aggregation, which means that the
  * ICallInterceptor delegates its IUnknown implementation to its outer object
  * (the mscom::Interceptor we implement and control).
+ *
+ * ACHTUNG! mscom::Interceptor uses FastMarshaler to disable COM garbage
+ * collection. If you use this class, you *must* call
+ * Interceptor::DisconnectRemotesForTarget when an object is no longer relevant.
+ * Otherwise, the object will never be released, causing a leak.
  */
 class Interceptor final : public WeakReferenceSupport,
                           public IStdMarshalInfo,
                           public IMarshal,
                           public IInterceptor {
  public:
   static HRESULT Create(STAUniquePtr<IUnknown> aTarget, IInterceptorSink* aSink,
                         REFIID aInitialIid, void** aOutInterface);