Bug 1509930 - Simplify CycleCollectWithLogsChild's lifetime, r=jld
authorNika Layzell <nika@thelayzells.com>
Fri, 23 Nov 2018 17:07:41 -0500
changeset 448512 a06e4cff09e6fe6ca41626310d6494a3d276772a
parent 448511 207746d120e9c8a9722d210fa96948472bce9425
child 448513 069d324080a54edeb3b8d76b16c9fe85c33a1d3b
push id110205
push usernika@thelayzells.com
push dateWed, 28 Nov 2018 17:59:22 +0000
treeherdermozilla-inbound@a06e4cff09e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjld
bugs1509930
milestone65.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 1509930 - Simplify CycleCollectWithLogsChild's lifetime, r=jld Previously this actor would call `Send__delete__` in its destructor, meaning that the lifetime of the actor is a bit hard to follow. This changes the actor to instead use a separate object for XPCOM, and use refcounting universally. This should also help with the transition to universally refcounted actors. Differential Revision: https://phabricator.services.mozilla.com/D12955
dom/ipc/ContentChild.cpp
--- a/dom/ipc/ContentChild.cpp
+++ b/dom/ipc/ContentChild.cpp
@@ -268,114 +268,123 @@ using mozilla::loader::PScriptCacheChild
 
 namespace mozilla {
 
 namespace dom {
 
 // IPC sender for remote GC/CC logging.
 class CycleCollectWithLogsChild final
   : public PCycleCollectWithLogsChild
-  , public nsICycleCollectorLogSink
 {
 public:
-  NS_DECL_ISUPPORTS
-
-  CycleCollectWithLogsChild(const FileDescriptor& aGCLog,
-                            const FileDescriptor& aCCLog)
-  {
-    mGCLog = FileDescriptorToFILE(aGCLog, "w");
-    mCCLog = FileDescriptorToFILE(aCCLog, "w");
-  }
-
-  NS_IMETHOD Open(FILE** aGCLog, FILE** aCCLog) override
-  {
-    if (NS_WARN_IF(!mGCLog) || NS_WARN_IF(!mCCLog)) {
-      return NS_ERROR_FAILURE;
-    }
-    *aGCLog = mGCLog;
-    *aCCLog = mCCLog;
-    return NS_OK;
-  }
-
-  NS_IMETHOD CloseGCLog() override
-  {
-    MOZ_ASSERT(mGCLog);
-    fclose(mGCLog);
-    mGCLog = nullptr;
-    SendCloseGCLog();
-    return NS_OK;
-  }
-
-  NS_IMETHOD CloseCCLog() override
+  NS_INLINE_DECL_REFCOUNTING(CycleCollectWithLogsChild)
+
+  class Sink final : public nsICycleCollectorLogSink
   {
-    MOZ_ASSERT(mCCLog);
-    fclose(mCCLog);
-    mCCLog = nullptr;
-    SendCloseCCLog();
-    return NS_OK;
-  }
-
-  NS_IMETHOD GetFilenameIdentifier(nsAString& aIdentifier) override
-  {
-    return UnimplementedProperty();
-  }
-
-  NS_IMETHOD SetFilenameIdentifier(const nsAString& aIdentifier) override
-  {
-    return UnimplementedProperty();
-  }
-
-  NS_IMETHOD GetProcessIdentifier(int32_t *aIdentifier) override
-  {
-    return UnimplementedProperty();
-  }
-
-  NS_IMETHOD SetProcessIdentifier(int32_t aIdentifier) override
-  {
-    return UnimplementedProperty();
-  }
-
-  NS_IMETHOD GetGcLog(nsIFile** aPath) override
-  {
-    return UnimplementedProperty();
-  }
-
-  NS_IMETHOD GetCcLog(nsIFile** aPath) override
-  {
-    return UnimplementedProperty();
-  }
+    NS_DECL_ISUPPORTS
+
+    Sink(CycleCollectWithLogsChild* aActor,
+         const FileDescriptor& aGCLog,
+         const FileDescriptor& aCCLog)
+    {
+      mActor = aActor;
+      mGCLog = FileDescriptorToFILE(aGCLog, "w");
+      mCCLog = FileDescriptorToFILE(aCCLog, "w");
+    }
+
+    NS_IMETHOD Open(FILE** aGCLog, FILE** aCCLog) override
+    {
+      if (NS_WARN_IF(!mGCLog) || NS_WARN_IF(!mCCLog)) {
+        return NS_ERROR_FAILURE;
+      }
+      *aGCLog = mGCLog;
+      *aCCLog = mCCLog;
+      return NS_OK;
+    }
+
+    NS_IMETHOD CloseGCLog() override
+    {
+      MOZ_ASSERT(mGCLog);
+      fclose(mGCLog);
+      mGCLog = nullptr;
+      mActor->SendCloseGCLog();
+      return NS_OK;
+    }
+
+    NS_IMETHOD CloseCCLog() override
+    {
+      MOZ_ASSERT(mCCLog);
+      fclose(mCCLog);
+      mCCLog = nullptr;
+      mActor->SendCloseCCLog();
+      return NS_OK;
+    }
+
+    NS_IMETHOD GetFilenameIdentifier(nsAString& aIdentifier) override
+    {
+      return UnimplementedProperty();
+    }
+
+    NS_IMETHOD SetFilenameIdentifier(const nsAString& aIdentifier) override
+    {
+      return UnimplementedProperty();
+    }
+
+    NS_IMETHOD GetProcessIdentifier(int32_t *aIdentifier) override
+    {
+      return UnimplementedProperty();
+    }
+
+    NS_IMETHOD SetProcessIdentifier(int32_t aIdentifier) override
+    {
+      return UnimplementedProperty();
+    }
+
+    NS_IMETHOD GetGcLog(nsIFile** aPath) override
+    {
+      return UnimplementedProperty();
+    }
+
+    NS_IMETHOD GetCcLog(nsIFile** aPath) override
+    {
+      return UnimplementedProperty();
+    }
+
+  private:
+    ~Sink()
+    {
+      if (mGCLog) {
+        fclose(mGCLog);
+        mGCLog = nullptr;
+      }
+      if (mCCLog) {
+        fclose(mCCLog);
+        mCCLog = nullptr;
+      }
+      // The XPCOM refcount drives the IPC lifecycle;
+      Unused << mActor->Send__delete__(mActor);
+    }
+
+    nsresult UnimplementedProperty()
+    {
+      MOZ_ASSERT(false, "This object is a remote GC/CC logger;"
+                        " this property isn't meaningful.");
+      return NS_ERROR_UNEXPECTED;
+    }
+
+    RefPtr<CycleCollectWithLogsChild> mActor;
+    FILE* mGCLog;
+    FILE* mCCLog;
+  };
 
 private:
-  ~CycleCollectWithLogsChild() override
-  {
-    if (mGCLog) {
-      fclose(mGCLog);
-      mGCLog = nullptr;
-    }
-    if (mCCLog) {
-      fclose(mCCLog);
-      mCCLog = nullptr;
-    }
-    // The XPCOM refcount drives the IPC lifecycle; see also
-    // DeallocPCycleCollectWithLogsChild.
-    Unused << Send__delete__(this);
-  }
-
-  nsresult UnimplementedProperty()
-  {
-    MOZ_ASSERT(false, "This object is a remote GC/CC logger;"
-                      " this property isn't meaningful.");
-    return NS_ERROR_UNEXPECTED;
-  }
-
-  FILE* mGCLog;
-  FILE* mCCLog;
+  ~CycleCollectWithLogsChild() {}
 };
 
-NS_IMPL_ISUPPORTS(CycleCollectWithLogsChild, nsICycleCollectorLogSink);
+NS_IMPL_ISUPPORTS(CycleCollectWithLogsChild::Sink, nsICycleCollectorLogSink);
 
 class AlertObserver
 {
 public:
 
   AlertObserver(nsIObserver *aObserver, const nsString& aData)
     : mObserver(aObserver)
     , mData(aData)
@@ -1316,44 +1325,42 @@ ContentChild::RecvRequestMemoryReport(co
   return IPC_OK();
 }
 
 PCycleCollectWithLogsChild*
 ContentChild::AllocPCycleCollectWithLogsChild(const bool& aDumpAllTraces,
                                               const FileDescriptor& aGCLog,
                                               const FileDescriptor& aCCLog)
 {
-  auto* actor = new CycleCollectWithLogsChild(aGCLog, aCCLog);
-  // Return actor with refcount 0, which is safe because it has a non-XPCOM type.
-  return actor;
+  return do_AddRef(new CycleCollectWithLogsChild()).take();
 }
 
 mozilla::ipc::IPCResult
 ContentChild::RecvPCycleCollectWithLogsConstructor(PCycleCollectWithLogsChild* aActor,
                                                    const bool& aDumpAllTraces,
                                                    const FileDescriptor& aGCLog,
                                                    const FileDescriptor& aCCLog)
 {
-  // Take a reference here, where the XPCOM type is regained.
-  RefPtr<CycleCollectWithLogsChild> sink = static_cast<CycleCollectWithLogsChild*>(aActor);
+  // The sink's destructor is called when the last reference goes away, which
+  // will cause the actor to be closed down.
+  auto* actor = static_cast<CycleCollectWithLogsChild*>(aActor);
+  RefPtr<CycleCollectWithLogsChild::Sink> sink =
+    new CycleCollectWithLogsChild::Sink(actor, aGCLog, aCCLog);
+
+  // Invoke the dumper, which will take a reference to the sink.
   nsCOMPtr<nsIMemoryInfoDumper> dumper = do_GetService("@mozilla.org/memory-info-dumper;1");
-
   dumper->DumpGCAndCCLogsToSink(aDumpAllTraces, sink);
-
-  // The actor's destructor is called when the last reference goes away...
   return IPC_OK();
 }
 
 bool
-ContentChild::DeallocPCycleCollectWithLogsChild(PCycleCollectWithLogsChild* /* aActor */)
+ContentChild::DeallocPCycleCollectWithLogsChild(PCycleCollectWithLogsChild* aActor)
 {
-  // ...so when we get here, there's nothing for us to do.
-  //
-  // Also, we're already in ~CycleCollectWithLogsChild (q.v.) at
-  // this point, so we shouldn't touch the actor in any case.
+  RefPtr<CycleCollectWithLogsChild> actor =
+    dont_AddRef(static_cast<CycleCollectWithLogsChild*>(aActor));
   return true;
 }
 
 mozilla::ipc::IPCResult
 ContentChild::RecvInitContentBridgeChild(Endpoint<PContentBridgeChild>&& aEndpoint)
 {
   ContentBridgeChild::Create(std::move(aEndpoint));
   return IPC_OK();