Bug 1509930 - Simplify CycleCollectWithLogsChild's lifetime, r=jld
☠☠ backed out by 9bfe29337ffe ☠ ☠
authorNika Layzell <nika@thelayzells.com>
Fri, 23 Nov 2018 17:07:41 -0500
changeset 504880 bce195f98895f82bd5ace5fc5ed5a5ca3b9f1afd
parent 504879 66db131264086346c31236ec00d02557dc5b4f06
child 504881 123b5d5a36377f409cb5c0a5e691e5eadb31626a
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [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)
@@ -1318,44 +1327,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();