1832820 - Remove uniqueness constraint from nsAsyncShutdown

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, task, P3)

Description

[Andreas Pehrson [:pehrsons]]

It would be great to avoid patterns like the below just to satisfy the uniqueness constraint:

• FileSystemShutdownBlocker generates a random id
• MediaRecorder appends its this pointer
• MediaTrackGraph appends its this pointer
• Document's UserInteractionTimer generates an ID and appends the document pointer
• BlobURLProtocolHandler's ReleasingTimerHolder appends its unique blob URL
• ContentParent appends its this pointer
• MediaDecoders use a singleton MediaShutdownManager but could probably be simplified to have one blocker per decoder
• SpeechRecognition appends its this pointer

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1832820 - In media::ShutdownBlocker don't make the name unique. r?karlt!

We should remove the uniqueness constraint in the underlying
nsAsyncShutdownService instead.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1832820 - Don't require unique names for blockers added through nsAsyncShutdownService. r?mak!, r?mccr8

nsAsyncShutdownService is a common way to handle async shutdown in C++. The
uniqueness constraint on the name of the blocker is however an ergonomy problem.
Several classes will append a pointer, or even generate a random id, to satisfy
the uniqueness constraint and allow registering multiple blockers of the same
type.

This patch removes the uniqueness constraint, and changes from registering
blockers in a Map to a WeakMap as it seems to be in line with the original
author's intention, and is now possible.

Some test coverage of this exists through media::ShutdownBlockingTicket, see
https://phabricator.services.mozilla.com/D177616.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to Andreas Pehrson [:pehrsons] from comment #0)

• Document's UserInteractionTimer generates an ID and appends the document pointer

This case is interesting because the author has included two kinds of information in the name:

1. The userInteractionTimerId is there for uniqueness, and is unlikely to be useful for any kind of debugging.
2. The aDocument is added even though the userInteractionTimerId is sufficient for uniqueness, presumably because this pointer may be useful to match against other pointers in debugging.

So while the uniqueness requirement is unnecessary and causes extra pain in implementing an nsIAsyncShutdownBlocker, some of the pointers in blocker names could be useful for debugging, so we don't want to remove them all.

I'd agree that nsIAsyncShutdownBlocker need not impose uniqueness, just as AsyncShutdown does not. The client can choose what information is readily available and useful for debugging.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc94b7583a39
In media::ShutdownBlocker don't make the name unique. r=karlt
https://hg.mozilla.org/integration/autoland/rev/5580f52f3e0b
Don't require unique names for blockers added through nsAsyncShutdownService. r=mak

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/dc94b7583a39
https://hg.mozilla.org/mozilla-central/rev/5580f52f3e0b