Backed out changeset 81e6aadf9911 (bug 1838779) for causing build bustage at ContentParent.cpp

2023-06-21 18:57:32 +03:00 · 2023-06-21 18:57:32 +03:00 · 6cebdf340e
--- a/dom/ipc/BrowserParent.cpp
+++ b/dom/ipc/BrowserParent.cpp
@ -56,7 +56,6 @@
 #include "mozilla/Preferences.h"
 #include "mozilla/PresShell.h"
 #include "mozilla/ProcessHangMonitor.h"
-#include "mozilla/RecursiveMutex.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/StaticPrefs_dom.h"
 #include "mozilla/TextEventDispatcher.h"
@ -696,15 +695,7 @@ void BrowserParent::Deactivated() {
  ProcessPriorityManager::BrowserPriorityChanged(this, /* aPriority = */ false);
 }

-void BrowserParent::Destroy() {
-  // Aggressively release the window to avoid leaking the world in shutdown
-  // corner cases.
-  mBrowserDOMWindow = nullptr;
-
-  if (mIsDestroyed) {
-    return;
-  }
-
+void BrowserParent::DestroyInternal() {
  Deactivated();

  RemoveWindowListeners();
@ -718,26 +709,32 @@ void BrowserParent::Destroy() {
  }
 #endif

-  {
-    // The following sequence assumes that the keepalive state does not change
-    // between the calls, but our ThreadsafeHandle might be accessed from other
-    // threads in the meantime.
-    RecursiveMutexAutoLock lock(Manager()->ThreadsafeHandleMutex());
+  // If this fails, it's most likely due to a content-process crash,
+  // and auto-cleanup will kick in.  Otherwise, the child side will
+  // destroy itself and send back __delete__().
+  Unused << SendDestroy();
+}

-    // If we are shutting down everything or we know to be the last
-    // BrowserParent, signal the impending shutdown early to the content process
-    // to avoid to run the SendDestroy before we know we are ExpectingShutdown.
-    Manager()->NotifyTabWillDestroy();
+void BrowserParent::Destroy() {
+  // Aggressively release the window to avoid leaking the world in shutdown
+  // corner cases.
+  mBrowserDOMWindow = nullptr;

-    // If this fails, it's most likely due to a content-process crash, and
-    // auto-cleanup will kick in.  Otherwise, the child side will destroy itself
-    // and send back __delete__().
-    (void)SendDestroy();
-    mIsDestroyed = true;
-
-    Manager()->NotifyTabDestroying();
+  if (mIsDestroyed) {
+    return;
  }

+  // If we are shutting down everything or we know to be the last
+  // BrowserParent, signal the impending shutdown early to the content process
+  // to avoid to run the SendDestroy before we know we are ExpectingShutdown.
+  Manager()->NotifyTabWillDestroy();
+
+  DestroyInternal();
+
+  mIsDestroyed = true;
+
+  Manager()->NotifyTabDestroying();
+
  // This `AddKeepAlive` will be cleared if `mMarkedDestroying` is set in
  // `ActorDestroy`. Out of caution, we don't add the `KeepAlive` if our IPC
  // actor has somehow already been destroyed, as that would mean `ActorDestroy`
--- a/dom/ipc/BrowserParent.h
+++ b/dom/ipc/BrowserParent.h
@ -758,6 +758,8 @@ class BrowserParent final : public PBrowserParent,
 private:
  void SuppressDisplayport(bool aEnabled);

+  void DestroyInternal();
+
  void SetRenderLayersInternal(bool aEnabled);

  already_AddRefed<nsFrameLoader> GetFrameLoader(
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@ -69,7 +69,6 @@
 #include "mozilla/ProcessHangMonitorIPC.h"
 #include "mozilla/ProfilerLabels.h"
 #include "mozilla/ProfilerMarkers.h"
-#include "mozilla/RecursiveMutex.h"
 #include "mozilla/ScopeExit.h"
 #include "mozilla/ScriptPreloader.h"
 #include "mozilla/Components.h"
@ -85,7 +84,6 @@
 #include "mozilla/TaskController.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/TelemetryIPC.h"
-#include "mozilla/ThreadSafety.h"
 #include "mozilla/Unused.h"
 #include "mozilla/WebBrowserPersistDocumentParent.h"
 #include "mozilla/devtools/HeapSnapshotTempFileHelperParent.h"
@ -796,41 +794,37 @@ void ContentParent::ReleaseCachedProcesses() {
             cps.GetData()->Length()));
  }
 #endif
-
-  // First let's collect all processes and keep a grip.
-  AutoTArray<RefPtr<ContentParent>, 32> fixArray;
+  // We process the toRelease array outside of the iteration to avoid modifying
+  // the list (via RemoveFromList()) while we're iterating it.
+  nsTArray<ContentParent*> toRelease;
  for (const auto& contentParents : sBrowserContentParents->Values()) {
+    // Shutting down these processes will change the array so let's use another
+    // array for the removal.
    for (auto* cp : *contentParents) {
-      fixArray.AppendElement(cp);
+      if (cp->ManagedPBrowserParent().Count() == 0 &&
+          !cp->HasActiveWorkerOrJSPlugin() &&
+          cp->mRemoteType == DEFAULT_REMOTE_TYPE) {
+        toRelease.AppendElement(cp);
+      } else {
+        MOZ_LOG(ContentParent::GetLog(), LogLevel::Debug,
+                ("  Skipping %p (%s), count %d, HasActiveWorkerOrJSPlugin %d",
+                 cp, cp->mRemoteType.get(), cp->ManagedPBrowserParent().Count(),
+                 cp->HasActiveWorkerOrJSPlugin()));
+      }
    }
  }

-  for (const auto& cp : fixArray) {
-    // Ensure the process cannot be claimed between check and MarkAsDead.
-    RecursiveMutexAutoLock lock(cp->ThreadsafeHandleMutex());
-
-    if (cp->ManagedPBrowserParent().Count() == 0 &&
-        !cp->HasActiveWorkerOrJSPlugin() &&
-        cp->mRemoteType == DEFAULT_REMOTE_TYPE) {
-      MOZ_LOG(
-          ContentParent::GetLog(), LogLevel::Debug,
-          ("  Shutdown %lul (%s)", cp->GetChildID(), cp->mRemoteType.get()));
-
-      PreallocatedProcessManager::Erase(cp);
-      // Make sure we don't select this process for new tabs or workers.
-      cp->MarkAsDead();
-      // Start a soft shutdown.
-      cp->ShutDownProcess(SEND_SHUTDOWN_MESSAGE);
-      // Make sure that this process is no longer accessible from JS by its
-      // message manager.
-      cp->ShutDownMessageManager();
-    } else {
-      MOZ_LOG(ContentParent::GetLog(), LogLevel::Debug,
-              ("  Skipping %lul (%s), count %d, HasActiveWorkerOrJSPlugin %d",
-               cp->GetChildID(), cp->mRemoteType.get(),
-               cp->ManagedPBrowserParent().Count(),
-               cp->HasActiveWorkerOrJSPlugin()));
-    }
+  for (auto* cp : toRelease) {
+    MOZ_LOG(ContentParent::GetLog(), LogLevel::Debug,
+            ("  Shutdown %p (%s)", cp, cp->mRemoteType.get()));
+    PreallocatedProcessManager::Erase(cp);
+    // Start a soft shutdown.
+    cp->ShutDownProcess(SEND_SHUTDOWN_MESSAGE);
+    // Make sure we don't select this process for new tabs.
+    cp->MarkAsDead();
+    // Make sure that this process is no longer accessible from JS by its
+    // message manager.
+    cp->ShutDownMessageManager();
  }
 }

@ -1000,7 +994,7 @@ already_AddRefed<ContentParent> ContentParent::GetUsedBrowserProcess(
    // it finishes starting
    preallocated->mRemoteType.Assign(aRemoteType);
    {
-      RecursiveMutexAutoLock lock(preallocated->mThreadsafeHandle->mMutex);
+      MutexAutoLock lock(preallocated->mThreadsafeHandle->mMutex);
      preallocated->mThreadsafeHandle->mRemoteType = preallocated->mRemoteType;
    }
    preallocated->mRemoteTypeIsolationPrincipal =
@ -1715,10 +1709,6 @@ bool ContentParent::CheckTabDestroyWillKeepAlive(
         ShouldKeepProcessAlive();
 }

-RecursiveMutex& ContentParent::ThreadsafeHandleMutex() {
-  return mThreadsafeHandle->mMutex;
-}
-
 void ContentParent::NotifyTabWillDestroy() {
  if (AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed)
 #if !defined(MOZ_WIDGET_ANDROID)
@ -1749,13 +1739,6 @@ void ContentParent::MaybeBeginShutDown(uint32_t aExpectedBrowserCount,
           ManagedPBrowserParent().Count(), aExpectedBrowserCount));
  MOZ_ASSERT(NS_IsMainThread());

-  // We need to lock our mutex here to ensure the state does not change
-  // between the check and the MarkAsDead.
-  // Note that if we come through BrowserParent::Destroy our mutex is
-  // already locked.
-  // TODO: We want to get rid of the ThreadsafeHandle, see bug 1683595.
-  RecursiveMutexAutoLock lock(mThreadsafeHandle->mMutex);
-
  // Both CheckTabDestroyWillKeepAlive and TryToRecycleE10SOnly will return
  // false if IsInOrBeyond(AppShutdownConfirmed), so if the parent shuts
  // down we will always shutdown the child.
@ -1773,7 +1756,7 @@ void ContentParent::MaybeBeginShutDown(uint32_t aExpectedBrowserCount,
  SignalImpendingShutdownToContentJS();

  if (aSendShutDown) {
-    AsyncSendShutDownMessage();
+    MaybeAsyncSendShutDownMessage();
  } else {
    // aSendShutDown is false only when we get called from
    // NotifyTabDestroying where we expect a subsequent call from
@ -1783,17 +1766,43 @@ void ContentParent::MaybeBeginShutDown(uint32_t aExpectedBrowserCount,
  }
 }

-void ContentParent::AsyncSendShutDownMessage() {
+void ContentParent::MaybeAsyncSendShutDownMessage() {
  MOZ_LOG(ContentParent::GetLog(), LogLevel::Verbose,
-          ("AsyncSendShutDownMessage %p", this));
+          ("MaybeAsyncSendShutDownMessage %p", this));
  MOZ_ASSERT(NS_IsMainThread());
  MOZ_ASSERT(sRecycledE10SProcess != this);

-  // In the case of normal shutdown, send a shutdown message to child to
-  // allow it to perform shutdown tasks.
-  GetCurrentSerialEventTarget()->Dispatch(NewRunnableMethod<ShutDownMethod>(
-      "dom::ContentParent::ShutDownProcess", this,
-      &ContentParent::ShutDownProcess, SEND_SHUTDOWN_MESSAGE));
+#ifdef DEBUG
+  // Calling this below while the lock is acquired will deadlock.
+  bool shouldKeepProcessAlive = ShouldKeepProcessAlive();
+#endif
+
+  {
+    MutexAutoLock lock(mThreadsafeHandle->mMutex);
+    MOZ_ASSERT_IF(!mThreadsafeHandle->mRemoteWorkerActorCount,
+                  !shouldKeepProcessAlive);
+
+    if (mThreadsafeHandle->mRemoteWorkerActorCount) {
+      return;
+    }
+
+    MOZ_ASSERT(!mThreadsafeHandle->mShutdownStarted);
+    mThreadsafeHandle->mShutdownStarted = true;
+  }
+
+  if (mSendShutdownTimer) {
+    mSendShutdownTimer->Cancel();
+    mSendShutdownTimer = nullptr;
+  }
+
+  if (!mSentShutdownMessage) {
+    // In the case of normal shutdown, send a shutdown message to child to
+    // allow it to perform shutdown tasks.
+    GetCurrentSerialEventTarget()->Dispatch(NewRunnableMethod<ShutDownMethod>(
+        "dom::ContentParent::ShutDownProcess", this,
+        &ContentParent::ShutDownProcess, SEND_SHUTDOWN_MESSAGE));
+    mSentShutdownMessage = true;
+  }
 }

 void MaybeLogBlockShutdownDiagnostics(ContentParent* aSelf, const char* aMsg,
@ -2019,14 +2028,6 @@ void ContentParent::MarkAsDead() {
  MOZ_DIAGNOSTIC_ASSERT(!sInProcessSelector);
  RemoveFromList();

-  // Flag shutdown has started for us to our threadsafe handle.
-  {
-    // Depending on how we get here, the lock might or might not be set.
-    RecursiveMutexAutoLock lock(mThreadsafeHandle->mMutex);
-
-    mThreadsafeHandle->mShutdownStarted = true;
-  }
-
  // Prevent this process from being re-used.
  PreallocatedProcessManager::Erase(this);
  StopRecyclingE10SOnly(false);
@ -2324,8 +2325,7 @@ bool ContentParent::HasActiveWorkerOrJSPlugin() {

  // If we have active workers, we need to stay alive.
  {
-    // Most of the times we'll get here with the mutex acquired, but still.
-    RecursiveMutexAutoLock lock(mThreadsafeHandle->mMutex);
+    MutexAutoLock lock(mThreadsafeHandle->mMutex);
    if (mThreadsafeHandle->mRemoteWorkerActorCount) {
      return true;
    }
@ -4448,7 +4448,7 @@ bool ContentParent::DeallocPRemoteSpellcheckEngineParent(
 void ContentParent::SendShutdownTimerCallback(nsITimer* aTimer,
                                              void* aClosure) {
  auto* self = static_cast<ContentParent*>(aClosure);
-  self->AsyncSendShutDownMessage();
+  self->MaybeAsyncSendShutDownMessage();
 }

 /* static */
@ -7292,7 +7292,7 @@ void ContentParent::UnregisterRemoveWorkerActor() {
  MOZ_ASSERT(NS_IsMainThread());

  {
-    RecursiveMutexAutoLock lock(mThreadsafeHandle->mMutex);
+    MutexAutoLock lock(mThreadsafeHandle->mMutex);
    if (--mThreadsafeHandle->mRemoteWorkerActorCount) {
      return;
    }
@ -8223,15 +8223,14 @@ IPCResult ContentParent::RecvSignalFuzzingReady() {
 #endif

 nsCString ThreadsafeContentParentHandle::GetRemoteType() {
-  RecursiveMutexAutoLock lock(mMutex);
+  MutexAutoLock lock(mMutex);
  return mRemoteType;
 }

 bool ThreadsafeContentParentHandle::MaybeRegisterRemoteWorkerActor(
    MoveOnlyFunction<bool(uint32_t, bool)> aCallback) {
-  RecursiveMutexAutoLock lock(mMutex);
+  MutexAutoLock lock(mMutex);
  if (aCallback(mRemoteWorkerActorCount, mShutdownStarted)) {
-    // TODO: I'd wish we could assert here that our ContentParent is alive.
    ++mRemoteWorkerActorCount;
    return true;
  }
--- a/dom/ipc/ContentParent.h
+++ b/dom/ipc/ContentParent.h
@ -28,7 +28,6 @@
 #include "mozilla/Maybe.h"
 #include "mozilla/MemoryReportingProcess.h"
 #include "mozilla/MozPromise.h"
-#include "mozilla/RecursiveMutex.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/UniquePtr.h"
@ -345,8 +344,6 @@ class ContentParent final : public PContentParent,
  virtual nsresult DoSendAsyncMessage(const nsAString& aMessage,
                                      StructuredCloneData& aData) override;

-  RecursiveMutex& ThreadsafeHandleMutex();
-
  /** Notify that a tab is about to send Destroy to its child. */
  void NotifyTabWillDestroy();

@ -851,7 +848,7 @@ class ContentParent final : public PContentParent,
    CLOSE_CHANNEL,
  };

-  void AsyncSendShutDownMessage();
+  void MaybeAsyncSendShutDownMessage();

  /**
   * Exit the subprocess and vamoose.  After this call IsAlive()
@ -1684,15 +1681,13 @@ class ThreadsafeContentParentHandle final {
    MaybeRegisterRemoteWorkerActor([](uint32_t, bool) { return true; });
  }

-  RecursiveMutex& Mutex() { return mMutex; }
-
 private:
  ThreadsafeContentParentHandle(ContentParent* aActor, ContentParentId aChildID,
                                const nsACString& aRemoteType)
      : mChildID(aChildID), mRemoteType(aRemoteType), mWeakActor(aActor) {}
  ~ThreadsafeContentParentHandle() { MOZ_ASSERT(!mWeakActor); }

-  mozilla::RecursiveMutex mMutex{"ContentParentIdentity"};
+  mozilla::Mutex mMutex{"ContentParentIdentity"};

  const ContentParentId mChildID;

--- a/dom/workers/remoteworkers/RemoteWorkerManager.cpp
+++ b/dom/workers/remoteworkers/RemoteWorkerManager.cpp
@ -550,7 +550,7 @@ void RemoteWorkerManager::ForEachActor(
 * remove worker actor unregistering
 * (see `ContentParent::UnregisterRemoveWorkerActor`).
 *
- * In `ContentParent::MaybeBeginShutdown` we only dispatch a runnable
+ * In `ContentParent::MaybeAsyncSendShutDownMessage` we only dispatch a runnable
 * to call `ContentParent::ShutDownProcess` if there are no registered remote
 * worker actors, and we ensure that the check for the number of registered
 * actors and the dispatching of the runnable are atomic. That happens on the