Bug 1754004 - Part 15: Don't track mOriginalInput in nsPipe, r=asuth

As discovered by TSan, this member could be raced on in some edge-cases if
cloned pipe streams raced on multiple threads to be destroyed. This change
instead moves the `nsIPipe` implementation to be on a seperate type from
`nsPipe`, allowing `mOriginalInput` to no longer need to be tracked.

This technically has slightly different behaviour than the previous
implementation for JS code, as code keeping the `nsIPipe` alive will now also
be keeping the output stream reference alive directly, rather than only holding
the `nsIPipe`, meaning that `pipe.outputStream` can be read multiple times
independently without implicitly closing the underlying stream. Based on a
quick read of the few remaining uses of `nsIPipe` in JS code, I don't think
this will be an issue, and it may actually be more intuitive and consistent.

Differential Revision: https://phabricator.services.mozilla.com/D145362

xpcom/io/nsPipe3.cpp

@@ -269,19 +269,20 @@ class nsPipeOutputStream : public nsIAsyncOutputStream, public nsIClassInfo {
 
 //-----------------------------------------------------------------------------
 
-class nsPipe final : public nsIPipe {
+class nsPipe final {
  public:
   friend class nsPipeInputStream;
   friend class nsPipeOutputStream;
   friend class AutoReadSegment;
 
-  NS_DECL_THREADSAFE_ISUPPORTS
-  NS_DECL_NSIPIPE
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsPipe)
 
-  // nsPipe methods:
-  nsPipe();
+  // public constructor
+  friend nsresult NS_NewPipe2(nsIAsyncInputStream**, nsIAsyncOutputStream**,
+                              bool, bool, uint32_t, uint32_t);
 
  private:
+  nsPipe(uint32_t aSegmentSize, uint32_t aSegmentCount);
   ~nsPipe();
 
   //
@@ -336,12 +337,6 @@ class nsPipe final : public nsIPipe {
   // Only usable while mReentrantMonitor is locked.
   nsTArray<nsPipeInputStream*> mInputList;
 
-  // But hold a strong ref to our original input stream.  For backward
-  // compatibility we need to be able to consistently return this same
-  // object from GetInputStream().  Note, mOriginalInput is also stored
-  // in mInputList as a weak ref.
-  RefPtr<nsPipeInputStream> mOriginalInput;
-
   ReentrantMonitor mReentrantMonitor MOZ_UNANNOTATED;
   nsSegmentedBuffer mBuffer;
 
@@ -356,7 +351,6 @@ class nsPipe final : public nsIPipe {
 
   // |mStatus| is protected by |mReentrantMonitor|.
   nsresult mStatus;
-  bool mInited;
 };
 
 //-----------------------------------------------------------------------------
@@ -483,96 +477,24 @@ class MOZ_STACK_CLASS AutoReadSegment final {
 // nsPipe methods:
 //-----------------------------------------------------------------------------
 
-nsPipe::nsPipe()
+nsPipe::nsPipe(uint32_t aSegmentSize, uint32_t aSegmentCount)
     : mOutput(this),
-      mOriginalInput(new nsPipeInputStream(this)),
       mReentrantMonitor("nsPipe.mReentrantMonitor"),
-      mMaxAdvanceBufferSegmentCount(0),
+      // protect against overflow
+      mMaxAdvanceBufferSegmentCount(
+          std::min(aSegmentCount, UINT32_MAX / aSegmentSize)),
       mWriteSegment(-1),
       mWriteCursor(nullptr),
       mWriteLimit(nullptr),
-      mStatus(NS_OK),
-      mInited(false) {
-  mInputList.AppendElement(mOriginalInput);
-}
-
-nsPipe::~nsPipe() = default;
-
-NS_IMPL_ADDREF(nsPipe)
-NS_IMPL_QUERY_INTERFACE(nsPipe, nsIPipe)
-
-NS_IMETHODIMP_(MozExternalRefCountType)
-nsPipe::Release() {
-  MOZ_DIAGNOSTIC_ASSERT(int32_t(mRefCnt) > 0, "dup release");
-  nsrefcnt count = --mRefCnt;
-  NS_LOG_RELEASE(this, count, "nsPipe");
-  if (count == 0) {
-    delete (this);
-    return 0;
-  }
-  // Avoid racing on |mOriginalInput| by only looking at it when
-  // the refcount is 1, that is, we are the only pointer (hence only
-  // thread) to access it.
-  if (count == 1 && mOriginalInput) {
-    mOriginalInput = nullptr;
-    return 1;
-  }
-  return count;
-}
-
-NS_IMETHODIMP
-nsPipe::Init(bool aNonBlockingIn, bool aNonBlockingOut, uint32_t aSegmentSize,
-             uint32_t aSegmentCount) {
-  mInited = true;
-
-  if (aSegmentSize == 0) {
-    aSegmentSize = DEFAULT_SEGMENT_SIZE;
-  }
-  if (aSegmentCount == 0) {
-    aSegmentCount = DEFAULT_SEGMENT_COUNT;
-  }
-
-  // protect against overflow
-  uint32_t maxCount = uint32_t(-1) / aSegmentSize;
-  if (aSegmentCount > maxCount) {
-    aSegmentCount = maxCount;
-  }
-
+      mStatus(NS_OK) {
   // The internal buffer is always "infinite" so that we can allow
   // the size to expand when cloned streams are read at different
   // rates.  We enforce a limit on how much data can be buffered
   // ahead of the fastest reader in GetWriteSegment().
-  nsresult rv = mBuffer.Init(aSegmentSize, UINT32_MAX);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
-  mMaxAdvanceBufferSegmentCount = aSegmentCount;
-
-  mOutput.SetNonBlocking(aNonBlockingOut);
-  mOriginalInput->SetNonBlocking(aNonBlockingIn);
-
-  return NS_OK;
+  MOZ_ALWAYS_SUCCEEDS(mBuffer.Init(aSegmentSize, UINT32_MAX));
 }
 
-NS_IMETHODIMP
-nsPipe::GetInputStream(nsIAsyncInputStream** aInputStream) {
-  if (NS_WARN_IF(!mInited)) {
-    return NS_ERROR_NOT_INITIALIZED;
-  }
-  RefPtr<nsPipeInputStream> ref = mOriginalInput;
-  ref.forget(aInputStream);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsPipe::GetOutputStream(nsIAsyncOutputStream** aOutputStream) {
-  if (NS_WARN_IF(!mInited)) {
-    return NS_ERROR_NOT_INITIALIZED;
-  }
-  NS_ADDREF(*aOutputStream = &mOutput);
-  return NS_OK;
-}
+nsPipe::~nsPipe() = default;
 
 void nsPipe::PeekSegment(const nsPipeReadState& aReadState, uint32_t aIndex,
                          char*& aCursor, char*& aLimit) {
@@ -1816,29 +1738,75 @@ nsresult NS_NewPipe2(nsIAsyncInputStream** aPipeIn,
                      nsIAsyncOutputStream** aPipeOut, bool aNonBlockingInput,
                      bool aNonBlockingOutput, uint32_t aSegmentSize,
                      uint32_t aSegmentCount) {
-  nsPipe* pipe = new nsPipe();
-  nsresult rv = pipe->Init(aNonBlockingInput, aNonBlockingOutput, aSegmentSize,
-                           aSegmentCount);
-  if (NS_FAILED(rv)) {
-    NS_ADDREF(pipe);
-    NS_RELEASE(pipe);
-    return rv;
-  }
+  RefPtr<nsPipe> pipe =
+      new nsPipe(aSegmentSize ? aSegmentSize : DEFAULT_SEGMENT_SIZE,
+                 aSegmentCount ? aSegmentCount : DEFAULT_SEGMENT_COUNT);
 
-  // These always succeed because the pipe is initialized above.
-  MOZ_ALWAYS_SUCCEEDS(pipe->GetInputStream(aPipeIn));
-  MOZ_ALWAYS_SUCCEEDS(pipe->GetOutputStream(aPipeOut));
+  RefPtr<nsPipeInputStream> pipeIn = new nsPipeInputStream(pipe);
+  pipe->mInputList.AppendElement(pipeIn);
+  RefPtr<nsPipeOutputStream> pipeOut = &pipe->mOutput;
+
+  pipeIn->SetNonBlocking(aNonBlockingInput);
+  pipeOut->SetNonBlocking(aNonBlockingOutput);
+
+  pipeIn.forget(aPipeIn);
+  pipeOut.forget(aPipeOut);
   return NS_OK;
 }
 
+////////////////////////////////////////////////////////////////////////////////
+
+// Thin nsIPipe implementation for consumers of the component manager interface
+// for creating pipes. Acts as a thin wrapper around NS_NewPipe2 for JS callers.
+class nsPipeHolder final : public nsIPipe {
+ public:
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSIPIPE
+
+ private:
+  ~nsPipeHolder() = default;
+
+  nsCOMPtr<nsIAsyncInputStream> mInput;
+  nsCOMPtr<nsIAsyncOutputStream> mOutput;
+};
+
+NS_IMPL_ISUPPORTS(nsPipeHolder, nsIPipe)
+
+NS_IMETHODIMP
+nsPipeHolder::Init(bool aNonBlockingInput, bool aNonBlockingOutput,
+                   uint32_t aSegmentSize, uint32_t aSegmentCount) {
+  if (mInput || mOutput) {
+    return NS_ERROR_ALREADY_INITIALIZED;
+  }
+  return NS_NewPipe2(getter_AddRefs(mInput), getter_AddRefs(mOutput),
+                     aNonBlockingInput, aNonBlockingOutput, aSegmentSize,
+                     aSegmentCount);
+}
+
+NS_IMETHODIMP
+nsPipeHolder::GetInputStream(nsIAsyncInputStream** aInputStream) {
+  if (mInput) {
+    *aInputStream = do_AddRef(mInput).take();
+    return NS_OK;
+  }
+  return NS_ERROR_NOT_INITIALIZED;
+}
+
+NS_IMETHODIMP
+nsPipeHolder::GetOutputStream(nsIAsyncOutputStream** aOutputStream) {
+  if (mOutput) {
+    *aOutputStream = do_AddRef(mOutput).take();
+    return NS_OK;
+  }
+  return NS_ERROR_NOT_INITIALIZED;
+}
+
 nsresult nsPipeConstructor(nsISupports* aOuter, REFNSIID aIID, void** aResult) {
   if (aOuter) {
     return NS_ERROR_NO_AGGREGATION;
   }
-  nsPipe* pipe = new nsPipe();
-  NS_ADDREF(pipe);
+  RefPtr<nsPipeHolder> pipe = new nsPipeHolder();
   nsresult rv = pipe->QueryInterface(aIID, aResult);
-  NS_RELEASE(pipe);
   return rv;
 }