Fix possible memory corruption in video player

Fixes two bugs, one was definitely happening, and 2nd one that is causing
undefined behavior and may work only in some std implementations

- VideoPlayer would attempt to access subtitles widget after VideoPlayer
itself was destroyed in onPlaybackFinished call
- std::function was destroyed from a function that is being called by
it. Replaced with 1-method interface to avoid usage of std::function in
this scenario

client/CMakeLists.txt

@@ -347,6 +347,7 @@ set(vcmiclientcommon_HEADERS
 	widgets/CArtifactsOfHeroAltar.h
 	widgets/CArtifactsOfHeroMarket.h
 	widgets/CArtifactsOfHeroBackpack.h
+	widgets/IVideoHolder.h
 	widgets/RadialMenu.h
 	widgets/VideoWidget.h
 	widgets/markets/CAltarArtifacts.h

client/mainmenu/CHighScoreScreen.cpp

@@ -205,7 +205,7 @@ CHighScoreInputScreen::CHighScoreInputScreen(bool won, HighScoreCalculation calc
 	}
 	else
 	{
-		videoPlayer = std::make_shared<VideoWidgetOnce>(Point(0, 0), VideoPath::builtin("LOSEGAME.SMK"), true, [this](){close();});
+		videoPlayer = std::make_shared<VideoWidgetOnce>(Point(0, 0), VideoPath::builtin("LOSEGAME.SMK"), true, this);
 		CCS->musich->playMusic(AudioPath::builtin("music/UltimateLose"), false, true);
 	}
 
@@ -216,6 +216,11 @@ CHighScoreInputScreen::CHighScoreInputScreen(bool won, HighScoreCalculation calc
 	}
 }
 
+void CHighScoreInputScreen::onVideoPlaybackFinished()
+{
+	close();
+}
+
 int CHighScoreInputScreen::addEntry(std::string text) {
 	std::vector<JsonNode> baseNode = persistentStorage["highscore"][calc.isCampaign ? "campaign" : "scenario"].Vector();
 	

client/mainmenu/CHighScoreScreen.h

@@ -8,6 +8,8 @@
  *
  */
 #pragma once
+
+#include "../widgets/IVideoHolder.h"
 #include "../windows/CWindowObject.h"
 #include "../../lib/gameState/HighScore.h"
 #include "../../lib/gameState/GameStatistics.h"
@@ -69,7 +71,7 @@ public:
 	CHighScoreInput(std::string playerName, std::function<void(std::string text)> readyCB);
 };
 
-class CHighScoreInputScreen : public CWindowObject
+class CHighScoreInputScreen : public CWindowObject, public IVideoHolder
 {
 	std::vector<std::shared_ptr<CLabel>> texts;
 	std::shared_ptr<CHighScoreInput> input;
@@ -82,6 +84,8 @@ class CHighScoreInputScreen : public CWindowObject
 	bool won;
 	HighScoreCalculation calc;
 	StatisticDataSet stat;
+
+	void onVideoPlaybackFinished() override;
 public:
 	CHighScoreInputScreen(bool won, HighScoreCalculation calc, const StatisticDataSet & statistic);
 

client/widgets/IVideoHolder.h

@@ -0,0 +1,17 @@
+/*
+ * IVideoHolder.h, part of VCMI engine
+ *
+ * Authors: listed in file AUTHORS in main folder
+ *
+ * License: GNU General Public License v2.0 or later
+ * Full text of license available in license.txt file, in main folder
+ *
+ */
+#pragma once
+
+class IVideoHolder
+{
+public:
+	virtual ~IVideoHolder() = default;
+	virtual void onVideoPlaybackFinished() = 0;
+};

client/widgets/VideoWidget.cpp

@@ -10,6 +10,7 @@
 #include "StdInc.h"
 #include "VideoWidget.h"
 #include "TextControls.h"
+#include "IVideoHolder.h"
 
 #include "../CGameInfo.h"
 #include "../gui/CGuiHandler.h"
@@ -172,15 +173,17 @@ void VideoWidgetBase::tick(uint32_t msPassed)
 	{
 		videoInstance->tick(msPassed);
 
+		if(subTitle)
+			subTitle->setText(getSubTitleLine(videoInstance->timeStamp()));
+
 		if(videoInstance->videoEnded())
 		{
 			videoInstance.reset();
 			stopAudio();
 			onPlaybackFinished();
+			// WARNING: onPlaybackFinished call may destoy `this`. Make sure that this is the very last operation in this method!
 		}
 	}
-	if(subTitle && videoInstance)
-		subTitle->setText(getSubTitleLine(videoInstance->timeStamp()));
 }
 
 VideoWidget::VideoWidget(const Point & position, const VideoPath & prologue, const VideoPath & looped, bool playAudio)
@@ -200,19 +203,19 @@ void VideoWidget::onPlaybackFinished()
 	playVideo(loopedVideo);
 }
 
-VideoWidgetOnce::VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, const std::function<void()> & callback)
+VideoWidgetOnce::VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, IVideoHolder * owner)
 	: VideoWidgetBase(position, video, playAudio)
-	, callback(callback)
+	, owner(owner)
 {
 }
 
-VideoWidgetOnce::VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, float scaleFactor, const std::function<void()> & callback)
+VideoWidgetOnce::VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, float scaleFactor, IVideoHolder * owner)
 	: VideoWidgetBase(position, video, playAudio, scaleFactor)
-	, callback(callback)
+	, owner(owner)
 {
 }
 
 void VideoWidgetOnce::onPlaybackFinished()
 {
-	callback();
+	owner->onVideoPlaybackFinished();
 }

client/widgets/VideoWidget.h

@@ -14,6 +14,7 @@
 #include "../lib/filesystem/ResourcePath.h"
 #include "../lib/json/JsonNode.h"
 
+class IVideoHolder;
 class IVideoInstance;
 class CMultiLineLabel;
 
@@ -64,10 +65,10 @@ public:
 
 class VideoWidgetOnce final: public VideoWidgetBase
 {
-	std::function<void()> callback;
+	IVideoHolder * owner;
 
 	void onPlaybackFinished() final;
 public:
-	VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, const std::function<void()> & callback);
+	VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, IVideoHolder * owner);
-	VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, float scaleFactor, const std::function<void()> & callback);
+	VideoWidgetOnce(const Point & position, const VideoPath & video, bool playAudio, float scaleFactor, IVideoHolder * owner);
 };

client/windows/CSpellWindow.cpp

@@ -498,20 +498,20 @@ void CSpellWindow::turnPageLeft()
 {
 	OBJECT_CONSTRUCTION;
 	if(settings["video"]["spellbookAnimation"].Bool() && !isBigSpellbook)
-		video = std::make_shared<VideoWidgetOnce>(Point(13, 14), VideoPath::builtin("PGTRNLFT.SMK"), false, [this](){
+		video = std::make_shared<VideoWidgetOnce>(Point(13, 14), VideoPath::builtin("PGTRNLFT.SMK"), false, this);
-			video.reset();
-			redraw();
-		});
 }
 
 void CSpellWindow::turnPageRight()
 {
 	OBJECT_CONSTRUCTION;
 	if(settings["video"]["spellbookAnimation"].Bool() && !isBigSpellbook)
-		video = std::make_shared<VideoWidgetOnce>(Point(13, 14), VideoPath::builtin("PGTRNRGH.SMK"), false, [this](){
+		video = std::make_shared<VideoWidgetOnce>(Point(13, 14), VideoPath::builtin("PGTRNRGH.SMK"), false, this);
-			video.reset();
+}
-			redraw();
+
-		});
+void CSpellWindow::onVideoPlaybackFinished()
+{
+	video.reset();
+	redraw();
 }
 
 void CSpellWindow::keyPressed(EShortcut key)

client/windows/CSpellWindow.h

@@ -10,6 +10,7 @@
 #pragma once
 
 #include "CWindowObject.h"
+#include "../widgets/IVideoHolder.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -31,7 +32,7 @@ class CToggleButton;
 class VideoWidgetOnce;
 
 /// The spell window
-class CSpellWindow : public CWindowObject
+class CSpellWindow : public CWindowObject, public IVideoHolder
 {
 	class SpellArea : public CIntObject
 	{
@@ -116,6 +117,8 @@ class CSpellWindow : public CWindowObject
 	void turnPageLeft();
 	void turnPageRight();
 
+	void onVideoPlaybackFinished() override;
+
 	bool openOnBattleSpells;
 	std::function<void(SpellID)> onSpellSelect; //external processing of selected spell
 

client/windows/GUIClasses.cpp

@@ -1670,22 +1670,27 @@ VideoWindow::VideoWindow(const VideoPath & video, const ImagePath & rim, bool sh
 	if(!rim.empty())
 	{
 		setBackground(rim);
-		videoPlayer = std::make_shared<VideoWidgetOnce>(Point(80, 186), video, true, [this](){ exit(false); });
+		videoPlayer = std::make_shared<VideoWidgetOnce>(Point(80, 186), video, true, this);
 		pos = center(Rect(0, 0, 800, 600));
 	}
 	else
 	{
 		blackBackground = std::make_shared<GraphicalPrimitiveCanvas>(Rect(0, 0, GH.screenDimensions().x, GH.screenDimensions().y));
-		videoPlayer = std::make_shared<VideoWidgetOnce>(Point(0, 0), video, true, scaleFactor, [this](){ exit(false); });
+		videoPlayer = std::make_shared<VideoWidgetOnce>(Point(0, 0), video, true, scaleFactor, this);
 		pos = center(Rect(0, 0, videoPlayer->pos.w, videoPlayer->pos.h));
 		blackBackground->addBox(Point(0, 0), Point(pos.x, pos.y), Colors::BLACK);
 	}
 
 	if(backgroundAroundWindow)
 		backgroundAroundWindow->pos.moveTo(Point(0, 0));
-
 }
 
+void VideoWindow::onVideoPlaybackFinished()
+{
+	exit(false);
+}
+
+
 void VideoWindow::exit(bool skipped)
 {
 	close();

client/windows/GUIClasses.h

@@ -12,6 +12,7 @@
 #include "CWindowObject.h"
 #include "../lib/ResourceSet.h"
 #include "../widgets/Images.h"
+#include "../widgets/IVideoHolder.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -509,7 +510,7 @@ public:
 	CThievesGuildWindow(const CGObjectInstance * _owner);
 };
 
-class VideoWindow : public CWindowObject
+class VideoWindow : public CWindowObject, public IVideoHolder
 {
 	std::shared_ptr<VideoWidgetOnce> videoPlayer;
 	std::shared_ptr<CFilledTexture> backgroundAroundWindow;
@@ -517,6 +518,7 @@ class VideoWindow : public CWindowObject
 
 	std::function<void(bool)> closeCb;
 
+	void onVideoPlaybackFinished() override;
 	void exit(bool skipped);
 public:
 	VideoWindow(const VideoPath & video, const ImagePath & rim, bool showBackground, float scaleFactor, const std::function<void(bool)> & closeCb);