Bug 1838779 - Overhaul the thread safety and synchronization of our keepalive... (7cec6117) · Commits · The Tor Project / Applications / Mullvad Browser

dom/ipc/BrowserParent.cpp

+26 −23

Original line number	Diff line number	Diff line
		@@ -56,6 +56,7 @@
		#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"
		@@ -695,7 +696,15 @@ void BrowserParent::Deactivated() {
		ProcessPriorityManager::BrowserPriorityChanged(this, /* aPriority = */ false);
		}

		void BrowserParent::DestroyInternal() {
		void BrowserParent::Destroy() {
		// Aggressively release the window to avoid leaking the world in shutdown
		// corner cases.
		mBrowserDOMWindow = nullptr;

		if (mIsDestroyed) {
		return;
		}

		Deactivated();

		RemoveWindowListeners();
		@@ -709,31 +718,25 @@ void BrowserParent::DestroyInternal() {
		}
		#endif

		// 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();
		}

		void BrowserParent::Destroy() {
		// Aggressively release the window to avoid leaking the world in shutdown
		// corner cases.
		mBrowserDOMWindow = nullptr;

		if (mIsDestroyed) {
		return;
		}
		{
		// 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 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();

		// 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();
		}

		// This `AddKeepAlive` will be cleared if `mMarkedDestroying` is set in
		// `ActorDestroy`. Out of caution, we don't add the `KeepAlive` if our IPC

dom/ipc/BrowserParent.h

+0 −2

Original line number	Diff line number	Diff line
		@@ -758,8 +758,6 @@ class BrowserParent final : public PBrowserParent,
		private:
		void SuppressDisplayport(bool aEnabled);

		void DestroyInternal();

		void SetRenderLayersInternal(bool aEnabled);

		already_AddRefed<nsFrameLoader> GetFrameLoader(

dom/ipc/ContentParent.cpp

+66 −66

Original line number	Diff line number	Diff line
		@@ -69,6 +69,7 @@
		#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"
		@@ -84,6 +85,7 @@
		#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"
		@@ -794,37 +796,40 @@ void ContentParent::ReleaseCachedProcesses() {
		cps.GetData()->Length()));
		}
		#endif
		// We process the toRelease array outside of the iteration to avoid modifying
		// the list (via RemoveFromList()) while we're iterating it.
		nsTArray<ContentParent*> toRelease;

		// First let's collect all processes and keep a grip.
		AutoTArray<RefPtr<ContentParent>, 32> fixArray;
		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);
		}
		}

		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) {
		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()));
		}
		}
		}
		(" Shutdown %p (%s)", cp.get(), cp->mRemoteType.get()));

		for (auto* cp : toRelease) {
		MOZ_LOG(ContentParent::GetLog(), LogLevel::Debug,
		(" Shutdown %p (%s)", cp, 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 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();
		} else {
		MOZ_LOG(
		ContentParent::GetLog(), LogLevel::Debug,
		(" Skipping %p (%s), count %d, HasActiveWorkerOrJSPlugin %d",
		cp.get(), cp->mRemoteType.get(), cp->ManagedPBrowserParent().Count(),
		cp->HasActiveWorkerOrJSPlugin()));
		}
		}
		}

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

		RecursiveMutex& ContentParent::ThreadsafeHandleMutex() {
		return mThreadsafeHandle->mMutex;
		}

		void ContentParent::NotifyTabWillDestroy() {
		if (AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed)
		#if !defined(MOZ_WIDGET_ANDROID)
		@@ -1739,6 +1748,13 @@ 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.
		@@ -1756,7 +1772,7 @@ void ContentParent::MaybeBeginShutDown(uint32_t aExpectedBrowserCount,
		SignalImpendingShutdownToContentJS();

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

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

		#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,
		@@ -2028,6 +2018,14 @@ 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);
		@@ -2325,7 +2323,8 @@ bool ContentParent::HasActiveWorkerOrJSPlugin() {

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

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

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

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

		bool ThreadsafeContentParentHandle::MaybeRegisterRemoteWorkerActor(
		MoveOnlyFunction<bool(uint32_t, bool)> aCallback) {
		MutexAutoLock lock(mMutex);
		RecursiveMutexAutoLock lock(mMutex);
		if (aCallback(mRemoteWorkerActorCount, mShutdownStarted)) {
		// TODO: I'd wish we could assert here that our ContentParent is alive.
		++mRemoteWorkerActorCount;
		return true;
		}

dom/ipc/ContentParent.h

+7 −2

Original line number	Diff line number	Diff line
		@@ -28,6 +28,7 @@
		#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"
		@@ -344,6 +345,8 @@ 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();

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

		void MaybeAsyncSendShutDownMessage();
		void AsyncSendShutDownMessage();

		/**
		* Exit the subprocess and vamoose. After this call IsAlive()
		@@ -1681,13 +1684,15 @@ 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::Mutex mMutex{"ContentParentIdentity"};
		mozilla::RecursiveMutex mMutex{"ContentParentIdentity"};

		const ContentParentId mChildID;

dom/workers/remoteworkers/RemoteWorkerManager.cpp

+1 −1

Original line number	Diff line number	Diff line
		@@ -550,7 +550,7 @@ void RemoteWorkerManager::ForEachActor(
		* remove worker actor unregistering
		* (see `ContentParent::UnregisterRemoveWorkerActor`).
		*
		* In `ContentParent::MaybeAsyncSendShutDownMessage` we only dispatch a runnable
		* In `ContentParent::MaybeBeginShutdown` 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