diff --git a/src/TMedia.h b/src/TMedia.h index 9fb74c062..51cf0474e 100644 --- a/src/TMedia.h +++ b/src/TMedia.h @@ -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()); } diff --git a/test/functional_tests/TMediaLoopTest.cpp b/test/functional_tests/TMediaLoopTest.cpp index d9a34c5a9..47942270c 100644 --- a/test/functional_tests/TMediaLoopTest.cpp +++ b/test/functional_tests/TMediaLoopTest.cpp @@ -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.