1695580 - DelayedRunnables will leak if target thread shuts down before delay has expired

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andreas Pehrson [:pehrsons]]

Consider delayedRunnable = DelayedRunnable(target, runnable, delay). Dispatching it works roughly like this:

• dispatch delayedRunnable to target, and starts a timer timer to run delayedRunnable on target after delay
• DelayedRunnable::Run checks whether delay has passed, if not waits for the timer timer
• when timer fires it dispatches delayedRunnable to target from the global TimerThread

Alas, if target has been shut down by the time the timer fires, the TimerThread releases the timer. In the case of DelayedRunnable it leaks. DelayedRunnable has a strong-ref to the timer, and the timer has a strong-ref to the DelayedRunnable through its callback member.

This seems deliberate; if the callback was released by the timer at this point, it would be released on a different thread than target, which could lead to a number of issues.

I think it would be reasonable to cancel any pending DelayedRunnable timers as part of target shutdown, probably as the last thing that happens on target.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - Break out DelayedRunnable into its own files. r?#xpcom-reviewers

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - Add gtests. r?#xpcom-reviewers

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - Simplify MozPromise code in nsThreadManager::Shutdown. r?#xpcom-reviewers

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - In xpcom, cancel pending DelayedRunnable timers on shutdown. r?KrisWright!

Because DelayedRunnables are fire-and-forget, there is no way for a targeted
EventTarget to clean them up on shutdown. Thus if a timer fires after
EventTarget shutdown it will fail to dispatch the timer event, and avoid
releasing the timer callback because it's not on the targeted thread. This
causes a leak as there is a ref-cycle between nsTimerImpl::mCallback and
DelayedRunnable::mTimer.

This patch adds nsIDelayedRunnableObserver for a target to observe which
DelayedRunnables are relying on their timer to run them. This allows the target
to schedule a shutdown task to cancel those timers and release the runnables on
the target thread.

Supported DelayedRunnable targets with this patch are TaskQueues,
eventqueue-based nsThreads and XPCOMThreadWrappers that wrap a supported
nsThread.

An assertion makes sure at runtime that future new uses of DelayedRunnable
target nsIDelayedRunnableObserver-supported event targets.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - In necko, cancel pending DelayedRunnable timers on shutdown. r?#necko-reviewers

This adds support for nsIDelayedRunnableObserver to nsStreamTransportService.

This is a bit special because nsStreamTransportService uses an nsThreadPool.
Because of race conditions we cannot dispatch a final cleanup task to cancel any
pending DelayedRunnables.

Because of the inherent raciness of threads in the thread pool we assume that
any pending DelayedRunnables can handle being released on any thread. Thus we
dispatch the cleanup task to the background event target once the thread pool
has been shut down and processed all its events. This ensures no races can occur
between the cleanup task and OnDelayedRunnableScheduled.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - Release WebSocketChannel::mTargetThread on main. r?#necko-reviewers

When mTargetThread is WebSocketImpl it must be released on main since it
implements nsISupportsWeakReference, and clearing weak references is not
threadsafe.

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - Remove mochitest rdd leak threshold. r?mjf,r?mccr8

It's unclear whether this bug fixes the leaks that prompted this leak threshold,
or whether they were already fixed. It is however clear that mochitests now run
without leaking.

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1695580 - Increase async shutdown timeout for ASAN to 5 minutes. r?#xpcom-reviewers

The patches on this bug make bug 1684441 increase in frequency. Presumable ASAN
shutdown is in many cases already close to the timeout, and this bug is making
it trip over the edge.

A 4 minute timeout made a broad linux x64 ASAN try run go from five occurrences
of bug 1684441 to two, whereas a 5 minute timeout made them go to zero.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae16f09be41b
Break out DelayedRunnable into its own files. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/de9a7dd7fc79
Add gtests. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/2103cd9e58b7
Simplify MozPromise code in nsThreadManager::Shutdown. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/d04f7a7ec375
In xpcom, cancel pending DelayedRunnable timers on shutdown. r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/6fdd154aa151
In necko, cancel pending DelayedRunnable timers on shutdown. r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/e38e8b90f119
Release WebSocketChannel::mTargetThread on main. r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/982a46056fcb
Remove mochitest rdd leak threshold. r=mjf,mccr8
https://hg.mozilla.org/integration/autoland/rev/ec0b0fcc8d88
Increase async shutdown timeout for ASAN to 5 minutes. r=xpcom-reviewers,mccr8

Comment 10

[Cristian Brindusan [:cbrindusan]]

Backed out 8 changesets (Bug 1695580) for causing build bustages on DataMutex.h.
https://hg.mozilla.org/integration/autoland/rev/2d5c17a4324bf88c2e7cf5030dbd7d9be4f34788

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=ec0b0fcc8d88eb688bd1d43d03d35a29875799d1&selectedTaskRun=bAY9ZTMFQd2sF75aBXOuCA.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=335566908&repo=autoland&lineNumber=40320

Comment 11

[Andreas Pehrson [:pehrsons]]

When you think you've covered all your bases.. Thanks for backing out, I'm taking a look.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19d803d5ab12
Break out DelayedRunnable into its own files. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/e263a02e58ea
Add gtests. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/2a88ea548a86
Simplify MozPromise code in nsThreadManager::Shutdown. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/3e501ab00f18
In xpcom, cancel pending DelayedRunnable timers on shutdown. r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/29f195804825
In necko, cancel pending DelayedRunnable timers on shutdown. r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/741be88ca028
Release WebSocketChannel::mTargetThread on main. r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/07e38871c80e
Remove mochitest rdd leak threshold. r=mjf,mccr8
https://hg.mozilla.org/integration/autoland/rev/503cd99c7318
Increase async shutdown timeout for ASAN to 5 minutes. r=xpcom-reviewers,mccr8

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/19d803d5ab12
https://hg.mozilla.org/mozilla-central/rev/e263a02e58ea
https://hg.mozilla.org/mozilla-central/rev/2a88ea548a86
https://hg.mozilla.org/mozilla-central/rev/3e501ab00f18
https://hg.mozilla.org/mozilla-central/rev/29f195804825
https://hg.mozilla.org/mozilla-central/rev/741be88ca028
https://hg.mozilla.org/mozilla-central/rev/07e38871c80e
https://hg.mozilla.org/mozilla-central/rev/503cd99c7318

Comment 15

[Andrew McCreight [:mccr8]]

Kris, is this something we should consider backing out, at least from beta? The patches in the bug this is blocking haven't landed yet, and there are an awful lot of regressions from these patches. It also sounds like fixing the regressions is not easy, and might introduce additional risk. If you want to keep working on the regressions, I could prepare some backout patches and see how they do on try.

Comment 16

[Kris Wright :KrisWright]

(In reply to Andrew McCreight [:mccr8] from comment #15)

Kris, is this something we should consider backing out, at least from beta? The patches in the bug this is blocking haven't landed yet, and there are an awful lot of regressions from these patches. It also sounds like fixing the regressions is not easy, and might introduce additional risk. If you want to keep working on the regressions, I could prepare some backout patches and see how they do on try.

The fix for the top crasher from my understanding is nontrivial so I think we should back this out from beta for the time being.

Comment 17

[Julien Cristau [:jcristau]]

Over to pascal for the beta backout.

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Backed out from beta https://hg.mozilla.org/releases/mozilla-beta/rev/21a23370449e01dcb4388f99bcd3778923db3def

Comment 19

[Julien Cristau [:jcristau]]

Backed out of beta again for 90:
https://hg.mozilla.org/releases/mozilla-beta/rev/3dabe43c4982a4759af9195a9eaf02d1062304eb

Comment 20

[Intermittent Failures Robot]

1 failures in 3711 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform and build breakdown:

• android-em-7-0-x86_64-lite-qr: 1
  • debug: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1695580&startday=2022-02-07&endday=2022-02-13&tree=all