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.
This commit is contained in:
parent
57b5911f13
commit
6240fd3d0a
|
@ -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
|
||||
|
||||
|
|
|
@ -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_;
|
||||
|
|
|
@ -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.
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
@ -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;
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
@ -47,7 +47,7 @@ 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,
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
@ -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");
|
||||
|
|
Loading…
Reference in New Issue