From 6240fd3d0aa3832efb3cf9c17ebe16676523715a Mon Sep 17 00:00:00 2001 From: Jim Broadus Date: Fri, 18 Jun 2021 22:21:36 -0700 Subject: [PATCH] player: Fix crash on UrlHandler error. In a case where a playlist is composed entirely of unresolvable internet service URLs and the playlist is set to repeat, playing an item will result in an infinite (until crash) recursive condition. HandleLoadResult is called with a NoMoreTracks result. It then calls NextItem, which calls PlayAt for the next item, which, again, calls HandleLoadResult. This can be reproduced by logging into a subsonic server, adding items to an empty playlist, then signing out. To solve this, separate the error condition from the NoMoreTracks result. Handle URL resolution errors the same way that media playback errors are handled, where an error count is incremented and the player stops if a limit is reached. The common code also notifies the playlist of the error and provides user feedback by graying out the item. --- src/core/player.cpp | 14 +++++++++++++- src/core/player.h | 2 ++ src/core/urlhandler.h | 11 ++++++++--- .../digitally/digitallyimportedurlhandler.cpp | 12 +++++------- src/internet/googledrive/googledriveurlhandler.cpp | 4 ++-- .../intergalacticfm/intergalacticfmurlhandler.cpp | 4 ++-- .../radiobrowser/radiobrowserurlhandler.cpp | 4 ++-- src/internet/somafm/somafmurlhandler.cpp | 4 ++-- src/internet/subsonic/subsonicurlhandler.cpp | 2 +- 9 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/core/player.cpp b/src/core/player.cpp index f94b5fc8b..223d1e109 100644 --- a/src/core/player.cpp +++ b/src/core/player.cpp @@ -165,6 +165,13 @@ void Player::HandleLoadResult(const UrlHandler::LoadResult& result) { // We'll get called again later with either NoMoreTracks or TrackAvailable loading_async_ = result.original_url_; break; + + case UrlHandler::LoadResult::Error: + qLog(Debug) << "URL handler error for" << result.original_url_; + nb_errors_received_++; + loading_async_ = QUrl(); + HandleInvalidItem(result.original_url_); + break; } } @@ -651,6 +658,7 @@ void Player::TrackAboutToEnd() { UrlHandler::LoadResult result = handler->LoadNext(req.RequestUrl()); switch (result.type_) { case UrlHandler::LoadResult::NoMoreTracks: + case UrlHandler::LoadResult::Error: return; case UrlHandler::LoadResult::WillLoadAsynchronously: @@ -674,8 +682,12 @@ void Player::ValidMediaRequested(const MediaPlaybackRequest& req) { } void Player::InvalidMediaRequested(const MediaPlaybackRequest& req) { + HandleInvalidItem(req.RequestUrl()); +} + +void Player::HandleInvalidItem(const QUrl& url) { // first send the notification to others... - emit SongChangeRequestProcessed(req.RequestUrl(), false); + emit SongChangeRequestProcessed(url, false); // ... and now when our listeners have completed their processing of the // current item we can change the current item by skipping to the next song diff --git a/src/core/player.h b/src/core/player.h index fc1fd3a2f..81956e664 100644 --- a/src/core/player.h +++ b/src/core/player.h @@ -202,6 +202,8 @@ class Player : public PlayerInterface { // Returns true if we were supposed to stop after this track. bool HandleStopAfter(); + void HandleInvalidItem(const QUrl& url); + private: Application* app_; Scrobbler* lastfm_; diff --git a/src/core/urlhandler.h b/src/core/urlhandler.h index 721119dcf..74d9bec5e 100644 --- a/src/core/urlhandler.h +++ b/src/core/urlhandler.h @@ -50,9 +50,12 @@ class UrlHandler : public QObject { // There was a track available. Its url is in media_url. TrackAvailable, + + // Failed to resolve URL. + Error }; - LoadResult(const QUrl& original_url, Type type = NoMoreTracks, + LoadResult(const QUrl& original_url, Type type, const QUrl& media_url = QUrl(), qint64 length_nanosec_ = -1); // The url that the playlist item has in Url(). @@ -73,11 +76,13 @@ class UrlHandler : public QObject { // Called by the Player when a song starts loading - gives the handler // a chance to do something clever to get a playable track. - virtual LoadResult StartLoading(const QUrl& url) { return LoadResult(url); } + virtual LoadResult StartLoading(const QUrl& url) = 0; // Called by the player when a song finishes - gives the handler a chance to // get another track to play. - virtual LoadResult LoadNext(const QUrl& url) { return LoadResult(url); } + virtual LoadResult LoadNext(const QUrl& url) { + return LoadResult(url, LoadResult::NoMoreTracks); + } // Functions to be warned when something happen to a track handled by // UrlHandler. diff --git a/src/internet/digitally/digitallyimportedurlhandler.cpp b/src/internet/digitally/digitallyimportedurlhandler.cpp index a98c0a5b8..261866c4d 100644 --- a/src/internet/digitally/digitallyimportedurlhandler.cpp +++ b/src/internet/digitally/digitallyimportedurlhandler.cpp @@ -51,15 +51,14 @@ QIcon DigitallyImportedUrlHandler::icon() const { UrlHandler::LoadResult DigitallyImportedUrlHandler::StartLoading( const QUrl& url) { - LoadResult ret(url); if (task_id_ != -1) { - return ret; + // Already loading. + return LoadResult(url, LoadResult::NoMoreTracks); } if (!service_->is_premium_account()) { service_->StreamError(tr("A premium account is required")); - ret.type_ = LoadResult::NoMoreTracks; - return ret; + return LoadResult(url, LoadResult::Error); } // Start loading the station @@ -73,8 +72,7 @@ UrlHandler::LoadResult DigitallyImportedUrlHandler::StartLoading( // Tell the user what's happening task_id_ = app_->task_manager()->StartTask(tr("Loading stream")); - ret.type_ = LoadResult::WillLoadAsynchronously; - return ret; + return LoadResult(url, LoadResult::WillLoadAsynchronously); } void DigitallyImportedUrlHandler::LoadPlaylistFinished(QIODevice* device) { @@ -94,7 +92,7 @@ void DigitallyImportedUrlHandler::LoadPlaylistFinished(QIODevice* device) { // Failed to get playlist? if (songs.count() == 0) { service_->StreamError(tr("Error loading di.fm playlist")); - emit AsyncLoadComplete(LoadResult(last_original_url_)); + emit AsyncLoadComplete(LoadResult(last_original_url_, LoadResult::Error)); return; } diff --git a/src/internet/googledrive/googledriveurlhandler.cpp b/src/internet/googledrive/googledriveurlhandler.cpp index 7fa0f58c1..496f9981e 100644 --- a/src/internet/googledrive/googledriveurlhandler.cpp +++ b/src/internet/googledrive/googledriveurlhandler.cpp @@ -28,8 +28,8 @@ GoogleDriveUrlHandler::GoogleDriveUrlHandler(GoogleDriveService* service, UrlHandler::LoadResult GoogleDriveUrlHandler::StartLoading(const QUrl& url) { QString file_id = url.path().remove(QChar('/')); QUrl real_url = service_->GetStreamingUrlFromSongId(file_id); - LoadResult::Type type = real_url.isValid() ? LoadResult::TrackAvailable - : LoadResult::NoMoreTracks; + LoadResult::Type type = + real_url.isValid() ? LoadResult::TrackAvailable : LoadResult::Error; LoadResult res(url, type, real_url); res.auth_header_ = service_->client()->GetAuthHeader(); return res; diff --git a/src/internet/intergalacticfm/intergalacticfmurlhandler.cpp b/src/internet/intergalacticfm/intergalacticfmurlhandler.cpp index 99dc63e48..d87778b66 100644 --- a/src/internet/intergalacticfm/intergalacticfmurlhandler.cpp +++ b/src/internet/intergalacticfm/intergalacticfmurlhandler.cpp @@ -69,7 +69,7 @@ void IntergalacticFMUrlHandler::LoadPlaylistFinished() { if (reply->error() != QNetworkReply::NoError) { // TODO((David Sansome): Error handling qLog(Error) << reply->errorString(); - emit AsyncLoadComplete(LoadResult(original_url, LoadResult::NoMoreTracks)); + emit AsyncLoadComplete(LoadResult(original_url, LoadResult::Error)); return; } @@ -82,7 +82,7 @@ void IntergalacticFMUrlHandler::LoadPlaylistFinished() { // Failed to get playlist? if (songs.count() == 0) { qLog(Error) << "Error loading" << scheme() << "playlist"; - emit AsyncLoadComplete(LoadResult(original_url, LoadResult::NoMoreTracks)); + emit AsyncLoadComplete(LoadResult(original_url, LoadResult::Error)); return; } diff --git a/src/internet/radiobrowser/radiobrowserurlhandler.cpp b/src/internet/radiobrowser/radiobrowserurlhandler.cpp index 27300ce8d..be818206c 100644 --- a/src/internet/radiobrowser/radiobrowserurlhandler.cpp +++ b/src/internet/radiobrowser/radiobrowserurlhandler.cpp @@ -47,11 +47,11 @@ UrlHandler::LoadResult RadioBrowserUrlHandler::StartLoading(const QUrl& url) { void RadioBrowserUrlHandler::LoadStationFailed(const QUrl& original_url) { qLog(Error) << "Error loading" << original_url; - emit AsyncLoadComplete(LoadResult(original_url, LoadResult::NoMoreTracks)); + emit AsyncLoadComplete(LoadResult(original_url, LoadResult::Error)); } void RadioBrowserUrlHandler::LoadStationFinished(const QUrl& original_url, const Song& song) { emit AsyncLoadComplete( LoadResult(original_url, LoadResult::TrackAvailable, song.url())); -} \ No newline at end of file +} diff --git a/src/internet/somafm/somafmurlhandler.cpp b/src/internet/somafm/somafmurlhandler.cpp index f0459e4ca..a33448018 100644 --- a/src/internet/somafm/somafmurlhandler.cpp +++ b/src/internet/somafm/somafmurlhandler.cpp @@ -66,7 +66,7 @@ void SomaFMUrlHandler::LoadPlaylistFinished() { if (reply->error() != QNetworkReply::NoError) { // TODO((David Sansome): Error handling qLog(Error) << reply->errorString(); - emit AsyncLoadComplete(LoadResult(original_url, LoadResult::NoMoreTracks)); + emit AsyncLoadComplete(LoadResult(original_url, LoadResult::Error)); return; } @@ -79,7 +79,7 @@ void SomaFMUrlHandler::LoadPlaylistFinished() { // Failed to get playlist? if (songs.count() == 0) { qLog(Error) << "Error loading" << scheme() << "playlist"; - emit AsyncLoadComplete(LoadResult(original_url, LoadResult::NoMoreTracks)); + emit AsyncLoadComplete(LoadResult(original_url, LoadResult::Error)); return; } diff --git a/src/internet/subsonic/subsonicurlhandler.cpp b/src/internet/subsonic/subsonicurlhandler.cpp index 06932af9a..60f7809ab 100644 --- a/src/internet/subsonic/subsonicurlhandler.cpp +++ b/src/internet/subsonic/subsonicurlhandler.cpp @@ -29,7 +29,7 @@ SubsonicUrlHandler::SubsonicUrlHandler(SubsonicService* service, UrlHandler::LoadResult SubsonicUrlHandler::StartLoading(const QUrl& url) { if (service_->login_state() != SubsonicService::LoginState_Loggedin) - return LoadResult(url); + return LoadResult(url, LoadResult::Error); QUrlQuery id(url.query()); QUrl newurl = service_->BuildRequestUrl("stream");