mirror of
https://github.com/Mudlet/Mudlet
synced 2026-08-13 18:26:27 -04:00
infrastructure: a media player no longer announces its own destruction (#9746)
#### Brief overview of PR changes/additions - `TMediaPlayer::~TMediaPlayer()` blocks its `QMediaPlayer`'s signals before the `stop()`/`setSource(QUrl())` that unloads the media, so no handler is called with a player whose members are going away. The unload itself is unchanged, and Qt still emits `destroyed()`. - New `TMediaLoopTest::test_destroyingAPlayerAnnouncesNothing()`, with a control unload on a player that is not being destroyed so it cannot pass vacuously. - `test_continuingToTheNextPassClearsTheEarlierAnnouncement()` declares its flag ahead of the player, so a lambda connected to that player always has live stack to write to. #### Motivation for adding to Mudlet Emitting signals from a destructor is a landmine for any connected code: today TMedia's own handlers survive it only because each one locks an already-expired `weak_ptr`, and the test suite, which does not, aborted. #### Other info (issues closed, discussion etc) Closes #9740 "TMediaLoopTest aborts under AddressSanitizer with stack-use-after-scope". Reproduced and verified on Linux with a clang ASan Debug build (`USE_SANITIZER=address`): pre-fix `TMediaLoopTest` aborts with `stack-use-after-scope` in `~TMediaPlayer` -> `QMediaPlayer::setSource()`, post-fix the suite is clean. The new test fails without the destructor change (counts 1 announcement) and passes with it. GCC does not poison per-variable within a scope, so the abort only shows up under clang/AppleClang. **Test case:** `cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_COMPILER=clang++` then `cmake --build build --target TMediaLoopTest && ctest --test-dir build -R TMediaLoopTest` - passes instead of `***Exception`. Assisted-by: Claude:claude-opus-5
This commit is contained in:
parent
b58a93fa38
commit
15e52faef9
2 changed files with 67 additions and 1 deletions
|
|
@ -57,6 +57,14 @@ public:
|
|||
~TMediaPlayer()
|
||||
{
|
||||
if (mMediaPlayer) {
|
||||
// The unload below releases the media file, and announces itself wherever it
|
||||
// changes something - sourceChanged from clearing the source, playbackStateChanged
|
||||
// from a stop that had something to stop - synchronously, into handlers reading a
|
||||
// TMediaPlayer whose members are about to go. The handlers TMedia installs decline
|
||||
// by way of a weak_ptr that has already expired by now; blocking makes that
|
||||
// structural rather than something each new handler has to remember. The unload
|
||||
// still happens, and Qt unblocks in ~QObject so destroyed() still arrives.
|
||||
mMediaPlayer->blockSignals(true);
|
||||
mMediaPlayer->stop();
|
||||
mMediaPlayer->setSource(QUrl());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -209,10 +209,12 @@ private slots:
|
|||
file.close();
|
||||
|
||||
TMediaData data{};
|
||||
// Declared ahead of the player: should a player ever emit while being destroyed again,
|
||||
// the lambda below has to have somewhere live to write to.
|
||||
bool announcedFromInsideSetSource = false;
|
||||
TMediaPlayer player(nullptr, data);
|
||||
QVERIFY(player.isInitialized());
|
||||
|
||||
bool announcedFromInsideSetSource = false;
|
||||
connect(player.mediaPlayer(), &QMediaPlayer::sourceChanged, player.mediaPlayer(), [&](const QUrl&) {
|
||||
player.noteEndAnnounced();
|
||||
announcedFromInsideSetSource = true;
|
||||
|
|
@ -225,6 +227,62 @@ private slots:
|
|||
QVERIFY2(!player.endAnnounced(), "The new pass started already counted as announced, so its own ending would be swallowed as a duplicate.");
|
||||
}
|
||||
|
||||
// Destroying a player unloads whatever it still holds, and nothing may hear that: a handler
|
||||
// that does would be reading a TMediaPlayer whose members are about to go. The handlers
|
||||
// TMedia installs decline anyway, each by way of a weak_ptr already expired by then, so
|
||||
// what this holds to is that no future one has to. Staged rather than played, since the
|
||||
// unload needs a source and not a backend that can decode it (#9740).
|
||||
void test_destroyingAPlayerAnnouncesNothing()
|
||||
{
|
||||
const QString path = qsl("%1/teardown.wav").arg(mProbeDir.path());
|
||||
QFile file(path);
|
||||
QVERIFY(file.open(QIODevice::WriteOnly));
|
||||
file.write(wavBytes());
|
||||
file.close();
|
||||
|
||||
int announcementsWhileBeingDestroyed = 0;
|
||||
bool beingDestroyed = false;
|
||||
const auto count = [&]() {
|
||||
if (beingDestroyed) {
|
||||
++announcementsWhileBeingDestroyed;
|
||||
}
|
||||
};
|
||||
|
||||
TMediaData data{};
|
||||
|
||||
{
|
||||
TMediaPlayer player(nullptr, data);
|
||||
QVERIFY(player.isInitialized());
|
||||
|
||||
connect(player.mediaPlayer(), &QMediaPlayer::sourceChanged, player.mediaPlayer(), count);
|
||||
connect(player.mediaPlayer(), &QMediaPlayer::playbackStateChanged, player.mediaPlayer(), count);
|
||||
|
||||
player.continuePlaying(QUrl::fromLocalFile(path));
|
||||
QVERIFY2(!player.mediaPlayer()->source().isEmpty(), "The player holds no source, so its destructor has nothing to unload and this test proves nothing.");
|
||||
|
||||
beingDestroyed = true;
|
||||
}
|
||||
|
||||
QVERIFY2(announcementsWhileBeingDestroyed == 0, "A player announced its own teardown, so every handler connected to it ran against a player being deleted.");
|
||||
|
||||
// The same unload on a player that is not being destroyed, to keep the assertion above
|
||||
// from passing on a Qt that has stopped announcing unloads at all.
|
||||
int announcementsFromAnUnblockedUnload = 0;
|
||||
const auto countUnblocked = [&]() {
|
||||
++announcementsFromAnUnblockedUnload;
|
||||
};
|
||||
|
||||
QMediaPlayer unblocked;
|
||||
connect(&unblocked, &QMediaPlayer::sourceChanged, &unblocked, countUnblocked);
|
||||
connect(&unblocked, &QMediaPlayer::playbackStateChanged, &unblocked, countUnblocked);
|
||||
unblocked.setSource(QUrl::fromLocalFile(path));
|
||||
announcementsFromAnUnblockedUnload = 0;
|
||||
unblocked.stop();
|
||||
unblocked.setSource(QUrl());
|
||||
|
||||
QVERIFY2(announcementsFromAnUnblockedUnload > 0, "An unload announced nothing even unblocked, so the assertion above passes without the destructor having to block anything.");
|
||||
}
|
||||
|
||||
// The deferred cleanup must still fire for a genuinely finished track, otherwise the
|
||||
// media source release added by #9237 is lost. Releasing the source is what this asserts
|
||||
// on because playingMedia() has already dropped the player by the time the cleanup runs.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue