<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Updated

•

8 months ago

status-firefox147: --- → unaffected

status-firefox148: --- → unaffected

status-firefox149: --- → affected

status-firefox-esr140: --- → unaffected

Flags: needinfo?(alwu)

rperta

Comment 2

•

8 months ago

Tier 1 failure here

Summary: Intermittent TV Assertion failure: mResponseTarget->IsOnCurrentThread(), at /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:628 → Intermittent Assertion failure: mResponseTarget->IsOnCurrentThread(), at /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:628

Comment 3

•

8 months ago

•

Edited

Following is a plausible explanation. I am a bit hesitant to paper over this in MozPromise, this feels like a worker weirdness we should discuss first.

Analysis

Summary

Intermittent MOZ_DIAGNOSTIC_ASSERT(mResponseTarget->IsOnCurrentThread()) crash in MozPromise.h:628, triggered by crashtest dom/media/test/crashtests/2014861.html. Regressed by bug 2014861.

Root cause

A race between MozPromise dispatch and worker shutdown:

Inner promises resolve on taskQueue. With All(taskQueue, ...), the AllPromise also resolves on the taskQueue and asynchronously dispatches a ResolveOrRejectRunnable to the worker's HybridEventTarget.
The worker starts shutting down. The StrongWorkerRef callback fires within DoRunLoop, calling holder->DisconnectIfExists() which sets mDisconnected = true.
The worker exits DoRunLoop, then RunShutdownTasks() calls ForgetWorkerPrivate(), which nulls out mWorkerPrivate on the HybridEventTarget.
NS_ProcessPendingEvents() drains remaining events, including the ResolveOrRejectRunnable dispatched in step 1.
In DoResolveOrReject(), the thread assertion at line 628 fires before the mDisconnected early-return at line 630, because WorkerEventTarget::IsOnCurrentThreadInfallible() returns false when mWorkerPrivate is null — even though the code is running on the worker thread.

The deeper issue: `WorkerEventTarget::IsOnCurrentThreadInfallible`

WorkerEventTarget::IsOnCurrentThreadInfallible() conflates two meanings of "false":

"You are on the wrong thread" (thread-safety)
"The worker is dead" (lifecycle)

After ForgetWorkerPrivate(), it returns false unconditionally, which is incorrect for thread-identity purposes when code is still running on the worker thread (during the NS_ProcessPendingEvents cleanup phase).

Callers relying on the current behavior

All callers that branch on IsOnCurrentThread() do so from the main thread, where the result is false regardless of whether the worker is alive:

PermissionStatusSink::PermissionChangedOnMainThread() — called from main thread (MOZ_ASSERT(NS_IsMainThread())), uses !IsOnCurrentThread() to enter a branch that checks mWorkerRef for liveness. Would return false regardless.
StorageAccessPermissionStatus — same pattern, main-thread caller.
ThreadSafeRequestHandle::~ThreadSafeRequestHandle() — if destroyed on the worker thread after ForgetWorkerPrivate, IsOnCurrentThread() lies (returns false), causing a proxy-release to a dead target with MOZ_ALWAYS_SUCCEEDS. This is arguably already a latent bug.

No caller was found that relies on IsOnCurrentThread() returning false from the worker thread as a "worker is dead" signal.

Possible fixes

MozPromise assertion reorder — move the MOZ_DIAGNOSTIC_ASSERT in DoResolveOrReject after the mDisconnected check. Minimal, safe, but papers over the semantic issue in WorkerEventTarget.
Fix WorkerEventTarget::IsOnCurrentThreadInfallible — preserve the PRThread* in ForgetWorkerPrivate so IsOnCurrentThread() continues to answer correctly after shutdown. Fixes the problem at the source and may fix latent bugs (e.g. WorkerLoadContext), but wider blast radius.

Flags: needinfo?(alwu) → needinfo?(echuang)

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 6

•

8 months ago

Hi Jens. Thanks for the detailed information about the root cause.

Calling WorkerPrivate::mWorkerHybridEventTarget::ForgetWorkerPrivate() in WorkerPrivate::RunShutdownTasks() means that any runnables dispatched to HybridEventTarget should be completed before ShutdownTasks. I think this is based on the concept that keeping a WorkerRef keeps the worker alive until you complete your shutdown. Although we should check if any runnables are on the WorkerPrivate::mWorkerHybridEventTarget at the moment, we can not do it since the runnables are dispatched to the different underlying event queues(mThread::EventQueue and mControlQueue), and some runnables are dispatched from other EventTargets, such as mWorkerControlEventTarget. It is hard to maintain the runnables/EventTarget mapping, especially when the runnable is in the PRThread's queue.

So, I still want to highlight the concept, keep the WorkerRef until your shutdown completes.

Generally, we thought MozPromiseRequestHolder::Disconnect() to prevent the execution of the corresponding resolve/reject runnable. And, the implementation of MozPromiseRequestHolder::Disconnect() achieves this by using mDisconnected within resolve/reject runnable::Run() to determine whether execution should occur. This means that MozPromiseRequestHolder::Disconnect() doesn't actually prevent the dispatching of resolve/reject runnables.

In this case, since the resolve/reject runnable is dispatched regardless, from the Worker's perspective, the entire runnable is part of shutdown of this functionality, the WorkerRef should be kept until the runnable::Run() completes. Therefore, calling MozPromiseRequestHolder::Disconnect() and releasing the WorkerRef in the WorkerRef's callback implicitly violates the design principles of WorkerRef.

So, I suggest preventing the dispatching of resolve/reject runnables. That is, before https://searchfox.org/firefox-main/rev/871325b8460362073cb2069874bef7dde3170cb2/xpcom/threads/MozPromise.h#586,607, it should be checked whether disconnection has occurred. Only in this way can it be confirmed that Disconnect() truly blocks the Resolve/Reject behavior in the target thread.

Flags: needinfo?(echuang)

Comment 7

•

8 months ago

(In reply to Eden Chuang[:edenchuang] from comment #6)

Generally, we thought MozPromiseRequestHolder::Disconnect() to prevent the execution of the corresponding resolve/reject runnable. And, the implementation of MozPromiseRequestHolder::Disconnect() achieves this by using mDisconnected within resolve/reject runnable::Run() to determine whether execution should occur. This means that MozPromiseRequestHolder::Disconnect() doesn't actually prevent the dispatching of resolve/reject runnables.

IIUC it does prevent it from dispatching to the target inside the ResolveOrRejectRunnable - we just assert on the is on target check too early. If that is all intentional on the worker side, we may just move the assertion until after the bail out (it still feels all semantically a bit weird on the worker side, though).

So, I suggest preventing the dispatching of resolve/reject runnables. That is, before https://searchfox.org/firefox-main/rev/871325b8460362073cb2069874bef7dde3170cb2/xpcom/threads/MozPromise.h#586,607, it should be checked whether disconnection has occurred. Only in this way can it be confirmed that Disconnect() truly blocks the Resolve/Reject behavior in the target thread.

I think that check is already in place inside the ResolveOrRejectRunnable / DoResolveOrReject ?

So I think if workers need to rely on that Disconnected bail out, we should just move the assertion.

Flags: needinfo?(echuang)

Assignee

Comment 8

•

8 months ago

(In reply to Jens Stutte [:jstutte] from comment #7)

(In reply to Eden Chuang[:edenchuang] from comment #6)

Generally, we thought MozPromiseRequestHolder::Disconnect() to prevent the execution of the corresponding resolve/reject runnable. And, the implementation of MozPromiseRequestHolder::Disconnect() achieves this by using mDisconnected within resolve/reject runnable::Run() to determine whether execution should occur. This means that MozPromiseRequestHolder::Disconnect() doesn't actually prevent the dispatching of resolve/reject runnables.

IIUC it does prevent it from dispatching to the target inside the ResolveOrRejectRunnable - we just assert on the is on target check too early. If that is all intentional on the worker side, we may just move the assertion until after the bail out (it still feels all semantically a bit weird on the worker side, though).

No, I mean ResolveOrRejectRunnable probably should not be dispatched after Disconnect() is called.

The codes you linked is just an early return in the execution of ResolveOrRejectRunnable::Run(), ThenValueBase::DoResolveOrReject()'s only caller is ResolveOrRejectRunnable::Run(). So when you get the if statement in ThenValueBase::DoResolveOrReject(), that means the runnable is already dispatched to the target and in the execution.

Following, I am going to explain why I say ThenValueBase::ResolveOrRejectRunnable could still be dispatched after ThenValueBase::Disconnect() is called.

When ThenValue::Disconnect() is called, ThenValueBase::Disconnect() is called to set Request::mDisconnected as true.

Then when the promise get resolved or rejected at some time passed after Disconnect(), MozPromise::Private::Resolve() or MozPromise::Private::Reject() is called.

Resolve() or Reject() will call MozPromise::Private::DispatchAll() to make the ThenValueBase::Dispatch() be called to dispatch the resolve or reject behavior to the target. In ThenValueBase::Dispatch(), you can see ResolveOrRejectRunnable is created and dispatched to the target thread no matter what the Request::mDisconnected value is.

Since the WorkerRef is released between ThenValue::Disconnect() and MozPromise::Private::Resolve()/Reject(), Worker treats the functionality shutdown had already completed, and Worker can continue its own shutdown. If unfortunately MozPromise::Private::Resolve()/Reject() is called in some bad moment, I mean the Worker is still in "Canceling" status, and the ResolveOrRejectRunnable is dispatched to the Worker thread. and more worse its execution is after WorkerPrivate::RunShutdownTasks(), then we hit the assertion in ResolveOrRejectRunnable::Run().

That why I say we should treat ResolveOrRejectRunnable::Run() as a part of functionality shutdown, and must have a StrongWorkerRef to protect its execution even though we set Request::mDisconnected as true to make it do nothing.

Here also something I need to highlight in MediaCapabilities.cpp.
As you can see that we create a StrongWorkerRef for the promises resolve/reject, and only call MozPromiseRequestHolder::Disconnect() in the WorkerRef's callback. It looks like we don't release the WorkerRef between ThenValue::Disconnect() and MozPromise::Private::Resolve()/Reject(). However, it is not true. This is because we just keep the WorkerRef in the ResolveFunction, unfortunately, when ThanValue::Disconnect() is called, the Resolve/Reject function could be reset and make WorkerRef be released implicitly. This is not easy to be noticed during the implementation.

So, I think the easiest fix is to make MediaCapabilities hold WorkerRef as a member variable, not just keep it in the Resolve lambda, which guarantees the WorkerRef will not be released by ThenValue::Disconnect() implicitly.

But since this could happen in other places, the best fix is to not dispatch ResolveOrRejectRunnable to the target if Request::mDisconnected is true. This would make the functionality shutdown completes when Disconnect() is called.

Hope this explains clearly.

Flags: needinfo?(echuang) → needinfo?(jstutte)

Comment 9

•

8 months ago

The functionality of Disconnect is a bit weird, but we definitely need at least the current behaviour of checking if we've disconnected in the runnable (as otherwise an in-flight runnable might fire the event). I'm guessing that the check wasn't added in the dispatching site because it seemed redundant.

I don't think adding a check at the dispatch site will actually fully prevent this issue, as the disconnect can still happen after the dispatch but before the runnable is executed on the target thread.

I think that whatever event target you're using for worker stuff here is probably buggy. It is generally assumed that the following does not assert, and it sounds like in this situation, that call would be failing right now:

eventTarget->Dispatch(NS_NewRunnableFunction("", [eventTarget] { MOZ_ASSERT(eventTarget->IsOnCurrentThread()); }));

Overall my intuition is saying that WorkerTargetThread::IsOnCurrentThread should be changed to no longer take the lifecycle of the worker thread into account if it is still executing runnables which were dispatched to it.

Flags: needinfo?(echuang)

Updated

•

8 months ago

Flags: needinfo?(jstutte)

Assignee

Comment 10

•

8 months ago

•

Edited

(In reply to Nika Layzell [:nika] (ni? for response) from comment #9)

The functionality of Disconnect is a bit weird, but we definitely need at least the current behaviour of checking if we've disconnected in the runnable (as otherwise an in-flight runnable might fire the event). I'm guessing that the check wasn't added in the dispatching site because it seemed redundant.

I agree, so I didn't say we should remove the mDisconnected checking in the ResolveOrRejectRunnable::Run().
But I wonder: if mDisconnected is true before dispatching ResolveOrRejectRunnable, why do we still dispatch a ResolveOrRejectRunnable that almost does nothing in the target thread?

I don't think adding a check at the dispatch site will actually fully prevent this issue, as the disconnect can still happen after the dispatch but before the runnable is executed on the target thread.

Keep WorkerRef, this issue would be fixed.

I think that whatever event target you're using for worker stuff here is probably buggy. It is generally assumed that the following does not assert, and it sounds like in this situation, that call would be failing right now:
eventTarget->Dispatch(NS_NewRunnableFunction("", [eventTarget] { MOZ_ASSERT(eventTarget->IsOnCurrentThread()); }));
Overall my intuition is saying that WorkerTargetThread::IsOnCurrentThread should be changed to no longer take the lifecycle of the worker thread into account if it is still executing runnables which were dispatched to it.

Yes, I also agree this is a bit weird. For Worker, we probably should have a better way to check that WorkerHybridEventTarget should be empty after WorkerPrivate::RunShutdownTasks(), instead of calling WorkerHybridEventTarget::ForgetWorkerPrivate() to invalidate all dispatched runnables. But currently, I will say it could be done in another bug.

As I said, we can move WorkerRef as a member variable of MediaCapabilities in this bug. That will use the WorkerRef correctly, keeping the worker alive until the corresponding behavior completes, whether or not ResolveOrRejectRunnable is dispatched.

How do you think about?

Flags: needinfo?(echuang) → needinfo?(nika)

Comment 11

•

8 months ago

(In reply to Eden Chuang[:edenchuang] from comment #10)

I agree, so I didn't say we should remove the mDisconnected checking in the ResolveOrRejectRunnable::Run().
But I wonder: if mDisconnected is true before dispatching ResolveOrRejectRunnable, why do we still dispatch a ResolveOrRejectRunnable that almost does nothing in the target thread?

mDisconnected is a non-atomic bool. As it's implemented right now, it's not safe to read it from any thread other than the target thread, which is why it's only read after dispatching to the target thread.

I don't think adding a check at the dispatch site will actually fully prevent this issue, as the disconnect can still happen after the dispatch but before the runnable is executed on the target thread.

Keep WorkerRef, this issue would be fixed.

I still think that there's a serious issue with WorkerEventTarget if it can enter a state where you can dispatch to it, and the dispatched runnable is run, but upon running IsOnCurrentThread() is false, and I think that should be fixed. I don't think that this would fix things, as we're already failing the dispatch in that situation.

Looking at the implementation, WorkerEventTarget stops allowing dispatches after mWorkerPrivate is cleared (https://searchfox.org/firefox-main/rev/4bc9c2f9b62e7cb44894d581c8171edbf0f7e27f/dom/workers/WorkerEventTarget.cpp#170-172), meaning that the dispatch is happening before ForgetWorkerPrivate has been called, and is running on the target thread after mWorkerPrivate has been cleared (https://searchfox.org/firefox-main/rev/4bc9c2f9b62e7cb44894d581c8171edbf0f7e27f/dom/workers/WorkerEventTarget.cpp#264-266).

To my understanding this means that whatever runnables are being dispatched are not keeping a WorkerRef which would keep the worker alive while the message is in flight (otherwise we wouldn't be calling ForgetWorkerPrivate - there's also no reference to WorkerRef in ExternalRunnableWrapper). Because of that I don't think that making mDisconnected atomic and reading it from the dispatching thread will fix anything here.

Yes, I also agree this is a bit weird. For Worker, we probably should have a better way to check that WorkerHybridEventTarget should be empty after WorkerPrivate::RunShutdownTasks(), instead of calling WorkerHybridEventTarget::ForgetWorkerPrivate() to invalidate all dispatched runnables. But currently, I will say it could be done in another bug.

As I said, we can move WorkerRef as a member variable of MediaCapabilities in this bug. That will use the WorkerRef correctly, keeping the worker alive until the corresponding behavior completes, whether or not ResolveOrRejectRunnable is dispatched.

How do you think about?

Maybe that could work (not following the specific MediaCapabilities logic here), but I still think that the WorkerEventTarget type is breaking nsIEventTarget expectations, and that it is likely the "real" source of the bug here.

Flags: needinfo?(nika) → needinfo?(echuang)

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 15

•

8 months ago

Okay, I think I understand what you're trying to say. I'll try to provide a patch that makes HybridEventTarget follow the expectations of nsIEventTarget. That is, when a runnable is sent to HybridEventTarget, HybridEventTarget::IsOnCurrentThread() will always be "true" while the runnable executes.

However, I want to emphasize that, although the patch makes HybridEventTarget meet nsIEventTarget's expectations, it only prevents the Assertion from issuing warnings. It doesn't mean the problem is fixed. Because for Worker, a runnable dispatched to HybridEventTarget must absolutely not be executed after WorkerPrivate::RunShundownTasks(). Otherwise, it will only lead to unpredictable behavior, such as UAF, memory leaks, or shutdown hangs, because the Worker cannot guarantee that the resources used by the runnable. No one holding a WorkerRef means the Worker can safely release the resources it owns.

And the only way to ensure the resources are alive is by holding the WorkerRef until the runnable finishes execution. Therefore, MediaCapabilities still need to hold WorkerRef as a member variable.

Flags: needinfo?(nika)

Flags: needinfo?(jstutte)

Flags: needinfo?(echuang)

Comment 16

•

8 months ago

•

Edited

I agree that MediaCapabilities should hold its WorkerRef as a member, and also that fixing HybridEventTarget's IsOnCurrentThread is the right thing to do.

However we might lose some (pretty random) visibility for "dangling" promises like this. Stepping back and looking at bug 1979268 comment 0 — we already have nsIEventTarget::RegisterShutdownTask and WorkerPrivate implements it. RunShutdownTasks() fires the registered tasks right before ForgetWorkerPrivate(), which is exactly the point where we'd want to handle dangling promises.

What if MozPromise::ThenValue registered a shutdown task with mResponseTarget (when the target supports it) that rejects the promise? The shutdown task would fire before ForgetWorkerPrivate(), while the worker is still in a good state, giving the consumer a chance to handle the situation. This would be a general safety net: any MozPromise targeting a worker (or any event target that shuts down) would auto-reject rather than relying on each consumer to manually hold a WorkerRef and disconnect in their shutdown callback.

This would probably happen in a different bug, FWIW.

Flags: needinfo?(jstutte)

Updated

•

8 months ago

Updated

•

8 months ago

status-firefox150: --- → affected

Assignee

Comment 18

•

8 months ago

That's what I want to improve the HybridEventTarget. So it would not just let HybridEventTarget follow nsIEventTarget's expectation.

By tracking the dispatched runnables on HybridEventTarget, a more specific judgment is made in WorkerPrivate::DoRunLoop() to ensure that all dispatched HybridEventTargets are executed before entering "Killing" status.

That should be fixed in most cases for HybridEventTarget. And the specific runnable tracking on HybridEventTarget should warn out if someone tries to dispatch a runnable on HybridEventTarget but does not hold a WorkerRef to keep the Worker alive, such that we can get the debug information be more specific about who violates the Worker's playing rules.

status-firefox150: affected → ---

Updated

•

8 months ago

status-firefox150: --- → affected

Comment 20

•

8 months ago

I think that the approach as you have it described right now is reasonable. The runnable dispatched in this particular case is probably OK (as it won't do anything with the worker's resources after being dispatched, as it has been discarded so will early return early in the runnable), but I think it makes sense to add these extra assertions and make sure that HybridEventTarget is not being dispatched runnables at the wrong times and without holding WorkerRef.

Flags: needinfo?(nika)

Comment hidden (Intermittent Failures Robot)

Comment 40

•

6 months ago

Three more intermittent bugs have been filed with the same root cause (WorkerEventTarget::IsOnCurrentThread() returning false during worker shutdown while dispatched runnables are still executing):

bug 2021424 — MozPromise<CopyableTArray<MediaCapabilitiesInfo>, MediaResult, true> (Mac amd64, content process, DOM Worker)
bug 2031366 — MozPromise<long, IOUtils::IOError, true> (Linux amd64, content process, DOM Worker)
bug 2032312 — MozPromise<OSInfo, nsresult, 0> (Windows x86, content process, DOM Worker)

All three share the identical stack shape: ExternalRunnableWrapper::WorkerRun → ResolveOrRejectRunnable::Run → DoResolveOrReject assertion. The different MozPromise template types in the signatures are ICF (Identical Code Folding) artifacts — the Mac 64-bit crash in bug 2021424 is the most reliable since ICF is less aggressive there, and both frames agree on MediaCapabilitiesInfo.

They appear on different tests (dom/media/test/crashtests/2014861.html and 2016504.html), confirming the worker is unrelated to the specific test.

Blocks: 2021424, 2031366, 2032312

Updated

•

6 months ago

Duplicate of this bug: 2032312

Updated

•

6 months ago

Duplicate of this bug: 2031366

Comment 43

•

6 months ago

Copying crash signatures from duplicate bugs.

Crash Signature: [@ mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::DoResolveOrReject]

Matthew Gregan [:kinetik]

Comment 44

•

6 months ago

Copying crash signatures from duplicate bugs.

Crash Signature: [@ mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::DoResolveOrReject] → [@ mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::DoResolveOrReject] [@ mozilla::MozPromise<long, mozilla::IOUtils::IOError, true>::ThenValueBase::DoResolveOrReject]

Updated

•

6 months ago

Blocks: 2032490

Comment hidden (Intermittent Failures Robot)

Emilio Cobos Álvarez [:emilio]

Updated

•

6 months ago

Crash Signature: [@ mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::DoResolveOrReject] [@ mozilla::MozPromise<long, mozilla::IOUtils::IOError, true>::ThenValueBase::DoResolveOrReject] → [@ mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::DoResolveOrReject] [@ mozilla::MozPromise<long, mozilla::IOUtils::IOError, true>::ThenValueBase::DoResolveOrReject]

Emilio Cobos Álvarez [:emilio]

Updated

•

6 months ago

Duplicate of this bug: 2032721

Emilio Cobos Álvarez [:emilio]

Comment 47

•

6 months ago

Eden, what's the status here? I assume per comment 18 / comment 20 you're looking at this?

Flags: needinfo?(echuang)

Paul Adenot (:padenot)

Updated

•

6 months ago

Blocks: 2005868

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 66

•

6 months ago

Attached file Bug 2017517 - Cleaning dispatched WorkerRunnables first after Worker switches to "Killing" state. r=asuth — Details

Comment hidden (Intermittent Failures Robot)

Updated

•

6 months ago

Assignee: nobody → echuang

Status: NEW → ASSIGNED

https://hg.mozilla.org/mozilla-central/rev/ba77a45a8172

Updated

•

6 months ago

Attachment #9585131 - Attachment description: WIP: Bug 2017517 - Cleaning dispatched WorkerRunnables first after Worker switches to "Killing" state. r=asuth → Bug 2017517 - Cleaning dispatched WorkerRunnables first after Worker switches to "Killing" state. r=asuth

Comment hidden (Intermittent Failures Robot)

Pulsebot

Comment 73

•

5 months ago

Pushed by echuang@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/913af0f6df51 https://hg.mozilla.org/integration/autoland/rev/ba77a45a8172 Cleaning dispatched WorkerRunnables first after Worker switches to "Killing" state. r=asuth,dom-worker-reviewers

Norisz Fay [:noriszfay]

Comment 74

•

5 months ago

bugherder

Status: ASSIGNED → RESOLVED

Closed: 5 months ago

status-firefox153: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 153 Branch

Ryan VanderMeulen [:RyanVM]

Updated

•

5 months ago

status-firefox149: affected → wontfix

status-firefox150: affected → wontfix

status-firefox151: --- → wontfix

status-firefox152: --- → affected

Flags: needinfo?(echuang)

Ryan VanderMeulen [:RyanVM]

Comment 75

•

5 months ago

Please add a Beta uplift request to this bug.

Flags: needinfo?(echuang)

Updated

•

5 months ago

Comment 77

•

5 months ago

Attached file Bug 2017517 - Cleaning dispatched WorkerRunnables first after Worker switches to "Killing" state. — Details

Currently, we have no good way to track the relationship between StrongWorkerRef and WorkerThreadRunnables for shutdown to ensure there is a StrongWorkerRef for the WorkerThreadRunnable dispatching and execution during Worker's shutdown.

This patch is a workaround for the general situation of performing shutdown work on the Worker thread without holding or unexpectedly releasing the StrongWorkerRef.

The idea of this patch is that the WorkerThreadRunnable dispatched to the WorkerEventTarget should be executed before breaking the connection between the WorkerEventTarget and the WorkerPrivate, especially for the Hybrid one. So this patch cleans up the dispatched WorkerThreadRunnables during the Worker state transition, by executing runnables after switching to the "Killing" state.

Because once the Worker is in "Killing," dispatching WorkerThreadRunnables will fail, the Worker will not hang by continuously runnable dispatching.

The following are the detailed steps of the patch.

When the Worker is in "Canceling," and there are no WorkerRefs, Timeouts, ChildWorkers, and pending runnables, the Worker transitions to the "Killing" state.

After the Worker is in "Killing", if runnables are dispatched in other threads between step 1 and step 2, execute the dispatched runnables.

Calling WorkerGlobalScopeBase::NoteShuttingDown() to shut down the StorageManager and releasing WorkerPrivate::mRemoteWorkerNonLifeCycleOpController, what we perform in WorkerPrivate::NotifyInternal() for "Killing" state.

The patch is a workaround because it can not perfectly handle the case of async shutdown behavior without StrongWorkerRef.

Original Revision: https://phabricator.services.mozilla.com/D299723

Updated

•

5 months ago

Attachment #9589670 - Flags: approval-mozilla-beta?

Comment 78

•

5 months ago

firefox-beta Uplift Approval Request

User impact if declined/Reason for urgency: User could meet content process crash caused by MOZ_DIGNOSTIC_ASSERT when using Workers.
Code covered by automated testing?: unknown
Fix verified in Nightly?: no
Needs manual QE testing?: no
Steps to reproduce for manual QE testing:
Risk associated with taking this patch: low
Explanation of risk level: This patch provides a workaround for cleaning the dispatched runnables on the Worker when the Worker gets into the Killing state.

This is a mitigation method when someone else sends a runnable to a Worker, but does not hold a WorkerRef.

This should not be high-risk.

String changes made/needed?: No
Is Android affected?: yes

Assignee

Updated

•

5 months ago

Flags: needinfo?(echuang)

Assignee

Updated

•

5 months ago

Duplicate of this bug: 2038615

Comment 80

•

5 months ago

Copying crash signatures from duplicate bugs.

Crash Signature: [@ mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::DoResolveOrReject] [@ mozilla::MozPromise<long, mozilla::IOUtils::IOError, true>::ThenValueBase::DoResolveOrReject] → [@ mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::DoResolveOrReject] [@ mozilla::MozPromise<long, mozilla::IOUtils::IOError, true>::ThenValueBase::DoResolveOrReject] [@ mozilla::MozPromise<nsTArray<mozilla::net::Http3ConnectionStatsParams>, mozil…

https://github.com/mozilla-firefox/firefox/commit/039cf15a03b2 https://hg.mozilla.org/releases/mozilla-beta/rev/5f0029160e2c

Updated

•

5 months ago

Attachment #9589670 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Lando Automation

Updated

•

5 months ago

status-firefox152: affected → fixed

Pulsebot

Comment 81

•

5 months ago

uplift