Bug 1847298: IPC constructor cleanup r=nika,padenot,dom-storage-reviewers,necko-reviewers,cookie-reviewers,asuth,Jamie

jesup · jesup · commit 55cad8d48f52 · 2023-12-08T15:56:39.000Z
Differential Revision: https://phabricator.services.mozilla.com/D185472
diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
@@ -1601,17 +1601,12 @@ void DocAccessible::DoInitialUpdate() {
         // RootAccessibles.
         MOZ_ASSERT(IsRoot());
         DocAccessibleChild* ipcDoc = IPCDoc();
-        if (ipcDoc) {
-          browserChild->SetTopLevelDocAccessibleChild(ipcDoc);
-        } else {
+        if (!ipcDoc) {
           ipcDoc = new DocAccessibleChild(this, browserChild);
+          MOZ_RELEASE_ASSERT(browserChild->SendPDocAccessibleConstructor(
+              ipcDoc, nullptr, 0, mDocumentNode->GetBrowsingContext()));
+          // trying to recover from this failing is problematic
           SetIPCDoc(ipcDoc);
-          // Subsequent initialization might depend on being able to get the
-          // top level DocAccessibleChild, so set that as early as possible.
-          browserChild->SetTopLevelDocAccessibleChild(ipcDoc);
-
-          browserChild->SendPDocAccessibleConstructor(
-              ipcDoc, nullptr, 0, mDocumentNode->GetBrowsingContext());
         }
       }
     }
diff --git a/accessible/generic/DocAccessible.h b/accessible/generic/DocAccessible.h
@@ -782,6 +782,7 @@ class DocAccessible : public HyperTextAccessible,
   PresShell* mPresShell;
 
   // Exclusively owned by IPDL so don't manually delete it!
+  // Cleared in ActorDestroy
   DocAccessibleChild* mIPCDoc;
 
   // These data structures map between LocalAccessibles and CacheDomains,
diff --git a/dom/base/nsContentPermissionHelper.cpp b/dom/base/nsContentPermissionHelper.cpp
@@ -293,10 +293,12 @@ nsresult nsContentPermissionUtils::AskPermission(
     NS_ENSURE_SUCCESS(rv, rv);
 
     req->IPDLAddRef();
-    ContentChild::GetSingleton()->SendPContentPermissionRequestConstructor(
-        req, permArray, principal, topLevelPrincipal,
-        hasValidTransientUserGestureActivation,
-        isRequestDelegatedToUnsafeThirdParty, child->GetTabId());
+    if (!ContentChild::GetSingleton()->SendPContentPermissionRequestConstructor(
+            req, permArray, principal, topLevelPrincipal,
+            hasValidTransientUserGestureActivation,
+            isRequestDelegatedToUnsafeThirdParty, child->GetTabId())) {
+      return NS_ERROR_FAILURE;
+    }
     ContentPermissionRequestChildMap()[req.get()] = child->GetTabId();
 
     req->Sendprompt();
diff --git a/dom/broadcastchannel/BroadcastChannel.cpp b/dom/broadcastchannel/BroadcastChannel.cpp
@@ -255,10 +255,13 @@ already_AddRefed<BroadcastChannel> BroadcastChannel::Constructor(
 
   PBroadcastChannelChild* actor = actorChild->SendPBroadcastChannelConstructor(
       storagePrincipalInfo, origin, nsString(aChannel));
+  if (!actor) {
+    // The PBackground actor is shutting down, return a 'generic' error.
+    aRv.Throw(NS_ERROR_FAILURE);
+    return nullptr;
+  }
 
   bc->mActor = static_cast<BroadcastChannelChild*>(actor);
-  MOZ_ASSERT(bc->mActor);
-
   bc->mActor->SetParent(bc);
   bc->mOriginForEvents = std::move(originForEvents);
 
diff --git a/dom/file/FileCreatorHelper.cpp b/dom/file/FileCreatorHelper.cpp
@@ -59,6 +59,10 @@ already_AddRefed<Promise> FileCreatorHelper::CreateFile(
   PFileCreatorChild* actor = actorChild->SendPFileCreatorConstructor(
       path, aBag.mType, aBag.mName, lastModified, aBag.mExistenceCheck,
       aIsFromNsIFile);
+  if (!actor) {
+    aRv.Throw(NS_ERROR_FAILURE);
+    return nullptr;
+  }
 
   static_cast<FileCreatorChild*>(actor)->SetPromise(promise);
   return promise.forget();
diff --git a/dom/indexedDB/IDBDatabase.cpp b/dom/indexedDB/IDBDatabase.cpp
@@ -609,16 +609,20 @@ RefPtr<IDBTransaction> IDBDatabase::Transaction(
     return nullptr;
   }
 
-  BackgroundTransactionChild* actor =
+  RefPtr<BackgroundTransactionChild> actor =
       new BackgroundTransactionChild(transaction.clonePtr());
 
   IDB_LOG_MARK_CHILD_TRANSACTION(
       "database(%s).transaction(%s)", "IDBDatabase.transaction(%.0s%.0s)",
       transaction->LoggingSerialNumber(), IDB_LOG_STRINGIFY(this),
       IDB_LOG_STRINGIFY(*transaction));
 
-  MOZ_ALWAYS_TRUE(mBackgroundActor->SendPBackgroundIDBTransactionConstructor(
-      actor, sortedStoreNames, mode));
+  if (!mBackgroundActor->SendPBackgroundIDBTransactionConstructor(
+          actor, sortedStoreNames, mode)) {
+    IDB_REPORT_INTERNAL_ERR();
+    aRv.ThrowUnknownError("Failed to create IndexedDB transaction");
+    return nullptr;
+  }
 
   transaction->SetBackgroundActor(actor);
 
diff --git a/dom/ipc/BrowserChild.cpp b/dom/ipc/BrowserChild.cpp
@@ -291,9 +291,6 @@ BrowserChild::BrowserChild(ContentChild* aManager, const TabId& aTabId,
       mIsPreservingLayers(false),
 #if defined(XP_WIN) && defined(ACCESSIBILITY)
       mNativeWindowHandle(0),
-#endif
-#if defined(ACCESSIBILITY)
-      mTopLevelDocAccessibleChild(nullptr),
 #endif
       mCancelContentJSEpoch(0) {
   mozilla::HoldJSObjects(this);
diff --git a/dom/ipc/BrowserChild.h b/dom/ipc/BrowserChild.h
@@ -583,16 +583,6 @@ class BrowserChild final : public nsMessageManagerScriptExecutor,
 
   BrowsingContext* GetBrowsingContext() const { return mBrowsingContext; }
 
-#if defined(ACCESSIBILITY)
-  void SetTopLevelDocAccessibleChild(PDocAccessibleChild* aTopLevelChild) {
-    mTopLevelDocAccessibleChild = aTopLevelChild;
-  }
-
-  PDocAccessibleChild* GetTopLevelDocAccessibleChild() {
-    return mTopLevelDocAccessibleChild;
-  }
-#endif
-
   // The transform from the coordinate space of this BrowserChild to the
   // coordinate space of the native window its BrowserParent is in.
   mozilla::LayoutDeviceToLayoutDeviceMatrix4x4
@@ -839,9 +829,6 @@ class BrowserChild final : public nsMessageManagerScriptExecutor,
   uintptr_t mNativeWindowHandle;
 #endif  // defined(XP_WIN)
 
-#if defined(ACCESSIBILITY)
-  PDocAccessibleChild* mTopLevelDocAccessibleChild;
-#endif
   int32_t mCancelContentJSEpoch;
 
   Maybe<LayoutDeviceToLayoutDeviceMatrix4x4> mChildToParentConversionMatrix;
diff --git a/dom/media/ipc/MFCDMChild.cpp b/dom/media/ipc/MFCDMChild.cpp
@@ -103,7 +103,7 @@ RefPtr<MFCDMChild::RemotePromise> MFCDMChild::EnsureRemote() {
             }
 
             mIPDLSelfRef = this;
-            Unused << manager->SendPMFCDMConstructor(this, mKeySystem);
+            MOZ_ALWAYS_TRUE(manager->SendPMFCDMConstructor(this, mKeySystem));
             mState = NS_OK;
             mRemotePromiseHolder.ResolveIfExists(true, __func__);
           },
diff --git a/dom/media/ipc/RemoteAudioDecoder.cpp b/dom/media/ipc/RemoteAudioDecoder.cpp
@@ -56,8 +56,8 @@ MediaResult RemoteAudioDecoderChild::InitIPDL(
   }
 
   mIPDLSelfRef = this;
-  Unused << manager->SendPRemoteDecoderConstructor(
-      this, aAudioInfo, aOptions, Nothing(), aMediaEngineId, Nothing());
+  MOZ_ALWAYS_TRUE(manager->SendPRemoteDecoderConstructor(
+      this, aAudioInfo, aOptions, Nothing(), aMediaEngineId, Nothing()));
   return NS_OK;
 }
 
diff --git a/dom/media/ipc/RemoteVideoDecoder.cpp b/dom/media/ipc/RemoteVideoDecoder.cpp
@@ -124,8 +124,8 @@ MediaResult RemoteVideoDecoderChild::InitIPDL(
 
   mIPDLSelfRef = this;
   VideoDecoderInfoIPDL decoderInfo(aVideoInfo, aFramerate);
-  Unused << manager->SendPRemoteDecoderConstructor(
-      this, decoderInfo, aOptions, aIdentifier, aMediaEngineId, aTrackingId);
+  MOZ_ALWAYS_TRUE(manager->SendPRemoteDecoderConstructor(
+      this, decoderInfo, aOptions, aIdentifier, aMediaEngineId, aTrackingId));
 
   return NS_OK;
 }
diff --git a/dom/media/mediacapabilities/BenchmarkStorageChild.cpp b/dom/media/mediacapabilities/BenchmarkStorageChild.cpp
@@ -18,7 +18,9 @@ PBenchmarkStorageChild* BenchmarkStorageChild::Instance() {
     sChild = new BenchmarkStorageChild();
     PContentChild* contentChild = dom::ContentChild::GetSingleton();
     MOZ_ASSERT(contentChild);
-    contentChild->SendPBenchmarkStorageConstructor();
+    if (!contentChild->SendPBenchmarkStorageConstructor()) {
+      MOZ_CRASH("SendPBenchmarkStorageConstructor failed");
+    }
   }
   MOZ_ASSERT(sChild);
   return sChild;
diff --git a/dom/media/systemservices/CamerasChild.cpp b/dom/media/systemservices/CamerasChild.cpp
@@ -41,25 +41,21 @@ class InitializeIPCThread : public Runnable {
       : Runnable("camera::InitializeIPCThread"), mCamerasChild(nullptr) {}
 
   NS_IMETHOD Run() override {
-    // Try to get the PBackground handle
+    // Get the PBackground handle
     ipc::PBackgroundChild* existingBackgroundChild =
-        ipc::BackgroundChild::GetForCurrentThread();
-    // If it's not spun up yet, block until it is, and retry
+        ipc::BackgroundChild::GetOrCreateForCurrentThread();
+    LOG(("BackgroundChild: %p", existingBackgroundChild));
     if (!existingBackgroundChild) {
-      LOG(("No existingBackgroundChild"));
-      existingBackgroundChild =
-          ipc::BackgroundChild::GetOrCreateForCurrentThread();
-      LOG(("BackgroundChild: %p", existingBackgroundChild));
-      if (!existingBackgroundChild) {
-        return NS_ERROR_FAILURE;
-      }
+      return NS_ERROR_FAILURE;
     }
 
     // Create CamerasChild
     // We will be returning the resulting pointer (synchronously) to our caller.
     mCamerasChild = static_cast<mozilla::camera::CamerasChild*>(
         existingBackgroundChild->SendPCamerasConstructor());
-
+    if (!mCamerasChild) {
+      return NS_ERROR_FAILURE;
+    }
     return NS_OK;
   }
 
diff --git a/dom/media/webspeech/synth/nsSynthVoiceRegistry.cpp b/dom/media/webspeech/synth/nsSynthVoiceRegistry.cpp
@@ -146,9 +146,10 @@ NS_IMPL_ISUPPORTS(nsSynthVoiceRegistry, nsISynthVoiceRegistry)
 nsSynthVoiceRegistry::nsSynthVoiceRegistry()
     : mSpeechSynthChild(nullptr), mUseGlobalQueue(false), mIsSpeaking(false) {
   if (XRE_IsContentProcess()) {
-    mSpeechSynthChild = new SpeechSynthesisChild();
-    ContentChild::GetSingleton()->SendPSpeechSynthesisConstructor(
-        mSpeechSynthChild);
+    SpeechSynthesisChild* actor = new SpeechSynthesisChild();
+    if (ContentChild::GetSingleton()->SendPSpeechSynthesisConstructor(actor)) {
+      mSpeechSynthChild = actor;
+    }
   }
 }
 
diff --git a/dom/payments/PaymentRequestManager.cpp b/dom/payments/PaymentRequestManager.cpp
@@ -360,7 +360,10 @@ PaymentRequestChild* PaymentRequestManager::GetPaymentChild(
   aRequest->GetInternalId(requestId);
 
   PaymentRequestChild* paymentChild = new PaymentRequestChild(aRequest);
-  browserChild->SendPPaymentRequestConstructor(paymentChild);
+  if (!browserChild->SendPPaymentRequestConstructor(paymentChild)) {
+    // deleted by Constructor
+    return nullptr;
+  }
 
   return paymentChild;
 }
diff --git a/dom/serviceworkers/FetchEventOpProxyParent.cpp b/dom/serviceworkers/FetchEventOpProxyParent.cpp
@@ -133,7 +133,7 @@ ParentToParentFetchEventRespondWithResult ToParentToParent(
     copyArgs.preloadResponseEndArgs() = aArgs.preloadResponseEndArgs();
   }
 
-  FetchEventOpProxyParent* actor =
+  RefPtr<FetchEventOpProxyParent> actor =
       new FetchEventOpProxyParent(std::move(aReal), std::move(aPromise));
 
   // As long as the fetch event was pending, the FetchEventOpParent was
diff --git a/dom/storage/LocalStorageManager.cpp b/dom/storage/LocalStorageManager.cpp
@@ -212,6 +212,10 @@ nsresult LocalStorageManager::GetStorageInternal(
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
+
+    if (!backgroundActor->CanSend()) {
+      return NS_ERROR_FAILURE;
+    }
 #endif
 
     // There is always a single instance of a cache per scope
diff --git a/extensions/spellcheck/src/mozSpellChecker.cpp b/extensions/spellcheck/src/mozSpellChecker.cpp
@@ -53,8 +53,11 @@ nsresult mozSpellChecker::Init() {
     mozilla::dom::ContentChild* contentChild =
         mozilla::dom::ContentChild::GetSingleton();
     MOZ_ASSERT(contentChild);
+    // mEngine is nulled in RemoteSpellcheckEngineChild(), so we don't need to
+    // worry about SendPRemoveSpellcheckEngineConstructor failing
     mEngine = new RemoteSpellcheckEngineChild(this);
-    contentChild->SendPRemoteSpellcheckEngineConstructor(mEngine);
+    MOZ_ALWAYS_TRUE(
+        contentChild->SendPRemoteSpellcheckEngineConstructor(mEngine));
   } else {
     mPersonalDictionary =
         do_GetService("@mozilla.org/spellchecker/personaldictionary;1");
diff --git a/ipc/glue/IdleSchedulerChild.cpp b/ipc/glue/IdleSchedulerChild.cpp
@@ -140,8 +140,10 @@ IdleSchedulerChild* IdleSchedulerChild::GetMainThreadIdleScheduler() {
   ipc::PBackgroundChild* background =
       ipc::BackgroundChild::GetOrCreateForCurrentThread();
   if (background) {
+    // this is nulled out on our destruction, so we don't need to worry
     sMainThreadIdleScheduler = new ipc::IdleSchedulerChild();
-    background->SendPIdleSchedulerConstructor(sMainThreadIdleScheduler);
+    MOZ_ALWAYS_TRUE(
+        background->SendPIdleSchedulerConstructor(sMainThreadIdleScheduler));
   }
   return sMainThreadIdleScheduler;
 }
diff --git a/netwerk/cookie/CookieServiceChild.cpp b/netwerk/cookie/CookieServiceChild.cpp
@@ -47,6 +47,7 @@ static StaticRefPtr<CookieServiceChild> gCookieChildService;
 already_AddRefed<CookieServiceChild> CookieServiceChild::GetSingleton() {
   if (!gCookieChildService) {
     gCookieChildService = new CookieServiceChild();
+    gCookieChildService->Init();
     ClearOnShutdown(&gCookieChildService);
   }
 
@@ -56,9 +57,11 @@ already_AddRefed<CookieServiceChild> CookieServiceChild::GetSingleton() {
 NS_IMPL_ISUPPORTS(CookieServiceChild, nsICookieService,
                   nsISupportsWeakReference)
 
-CookieServiceChild::CookieServiceChild() {
-  NS_ASSERTION(IsNeckoChild(), "not a child process");
+CookieServiceChild::CookieServiceChild() { NeckoChild::InitNeckoChild(); }
 
+CookieServiceChild::~CookieServiceChild() { gCookieChildService = nullptr; }
+
+void CookieServiceChild::Init() {
   auto* cc = static_cast<mozilla::dom::ContentChild*>(gNeckoChild->Manager());
   if (cc->IsShuttingDown()) {
     return;
@@ -67,9 +70,8 @@ CookieServiceChild::CookieServiceChild() {
   // This corresponds to Release() in DeallocPCookieService.
   NS_ADDREF_THIS();
 
-  NeckoChild::InitNeckoChild();
-
-  // Create a child PCookieService actor.
+  // Create a child PCookieService actor. Don't do this in the constructor
+  // since it could release 'this' on failure
   gNeckoChild->SendPCookieServiceConstructor(this);
 
   mThirdPartyUtil = ThirdPartyUtil::GetInstance();
@@ -79,8 +81,6 @@ CookieServiceChild::CookieServiceChild() {
   NS_ASSERTION(mTLDService, "couldn't get TLDService");
 }
 
-CookieServiceChild::~CookieServiceChild() { gCookieChildService = nullptr; }
-
 void CookieServiceChild::TrackCookieLoad(nsIChannel* aChannel) {
   if (!CanSend()) {
     return;
diff --git a/netwerk/cookie/CookieServiceChild.h b/netwerk/cookie/CookieServiceChild.h
@@ -37,6 +37,8 @@ class CookieServiceChild final : public PCookieServiceChild,
 
   CookieServiceChild();
 
+  void Init();
+
   static already_AddRefed<CookieServiceChild> GetSingleton();
 
   void TrackCookieLoad(nsIChannel* aChannel);
diff --git a/netwerk/dns/DNSRequestChild.cpp b/netwerk/dns/DNSRequestChild.cpp
@@ -428,7 +428,7 @@ void DNSRequestSender::StartRequest() {
     return;
   }
 
-  if (DNSRequestChild* child = mIPCActor->AsDNSRequestChild()) {
+  if (RefPtr<DNSRequestChild> child = mIPCActor->AsDNSRequestChild()) {
     if (XRE_IsContentProcess()) {
       mozilla::dom::ContentChild* cc =
           static_cast<mozilla::dom::ContentChild*>(gNeckoChild->Manager());
@@ -450,8 +450,8 @@ void DNSRequestSender::StartRequest() {
         return;
       }
 
-      socketProcessChild->SendPDNSRequestConstructor(
-          child, mHost, mTrrServer, mPort, mType, mOriginAttributes, mFlags);
+      MOZ_ALWAYS_TRUE(socketProcessChild->SendPDNSRequestConstructor(
+          child, mHost, mTrrServer, mPort, mType, mOriginAttributes, mFlags));
     } else {
       MOZ_ASSERT(false, "Wrong process");
       return;
diff --git a/netwerk/protocol/gio/GIOChannelChild.cpp b/netwerk/protocol/gio/GIOChannelChild.cpp
@@ -123,13 +123,15 @@ GIOChannelChild::AsyncOpen(nsIStreamListener* aListener) {
   // This must happen before the constructor message is sent.
   SetupNeckoTarget();
 
-  gNeckoChild->SendPGIOChannelConstructor(
-      this, browserChild, IPC::SerializedLoadContext(this), openArgs);
-
   // The socket transport layer in the chrome process now has a logical ref to
   // us until OnStopRequest is called.
   AddIPDLReference();
 
+  if (!gNeckoChild->SendPGIOChannelConstructor(
+          this, browserChild, IPC::SerializedLoadContext(this), openArgs)) {
+    return NS_ERROR_FAILURE;
+  }
+
   mIsPending = true;
   mWasOpened = true;
 
diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -1374,6 +1374,10 @@ nsresult nsHttpChannel::SetupTransaction() {
     if (NS_WARN_IF(!gIOService->SocketProcessReady())) {
       return NS_ERROR_NOT_AVAILABLE;
     }
+    SocketProcessParent* socketProcess = SocketProcessParent::GetSingleton();
+    if (!socketProcess->CanSend()) {
+      return NS_ERROR_NOT_AVAILABLE;
+    }
 
     nsCOMPtr<nsIParentChannel> parentChannel;
     NS_QueryNotificationCallbacks(this, parentChannel);
@@ -1395,11 +1399,10 @@ nsresult nsHttpChannel::SetupTransaction() {
     transParent->SetRedirectTimestamp(mRedirectStartTimeStamp,
                                       mRedirectEndTimeStamp);
 
-    SocketProcessParent* socketProcess = SocketProcessParent::GetSingleton();
     if (socketProcess) {
-      Unused << socketProcess->SendPHttpTransactionConstructor(transParent);
+      MOZ_ALWAYS_TRUE(
+          socketProcess->SendPHttpTransactionConstructor(transParent));
     }
-
     mTransaction = transParent;
   } else {
     mTransaction = new nsHttpTransaction();
diff --git a/netwerk/protocol/http/nsHttpHandler.cpp b/netwerk/protocol/http/nsHttpHandler.cpp
diff --git a/netwerk/protocol/websocket/WebSocketChannelChild.cpp b/netwerk/protocol/websocket/WebSocketChannelChild.cpp

Original file line number	Diff line number	Diff line change
`@@ -103,7 +103,7 @@ RefPtr<MFCDMChild::RemotePromise> MFCDMChild::EnsureRemote() {`
`103`	`103`	`}`
`104`	`104`
`105`	`105`	`mIPDLSelfRef = this;`
`106`		`- Unused << manager->SendPMFCDMConstructor(this, mKeySystem);`
	`106`	`+ MOZ_ALWAYS_TRUE(manager->SendPMFCDMConstructor(this, mKeySystem));`
`107`	`107`	`mState = NS_OK;`
`108`	`108`	`mRemotePromiseHolder.ResolveIfExists(true, __func__);`
`109`	`109`	`},`
Original file line number	Diff line number	Diff line change
`@@ -56,8 +56,8 @@ MediaResult RemoteAudioDecoderChild::InitIPDL(`
`56`	`56`	`}`
`57`	`57`
`58`	`58`	`mIPDLSelfRef = this;`
`59`		`- Unused << manager->SendPRemoteDecoderConstructor(`
`60`		`- this, aAudioInfo, aOptions, Nothing(), aMediaEngineId, Nothing());`
	`59`	`+ MOZ_ALWAYS_TRUE(manager->SendPRemoteDecoderConstructor(`
	`60`	`+ this, aAudioInfo, aOptions, Nothing(), aMediaEngineId, Nothing()));`
`61`	`61`	`return NS_OK;`
`62`	`62`	`}`
`63`	`63`