From 11cf04eed171f3dac856929f917febc62b8d9636 Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Wed, 11 Nov 2020 13:39:42 +0900
Subject: [PATCH] Fix frames potentially getting added to spectator replay in
 wrong format

The way spectator currently works, the `Spectator` screen is responsible
for adding new frames to the replay, even when it has a child
(`SpectatorPlayer`) present.

There was a possibility that a new play had already started, and on
returning to the Spectator screen (to initialise the new play) there
would be a brief period where the Player instance is still reading from
the replay, the `userBeganPlaying` call had not yet finished
initialising the new target replay, and `userSentFrames` is run
(asynchronously), writing frames to the previous replay using the
incorrect ruleset instance).

To make this work, it doesn't `Schedule` frame addition to the replay
(making things a bit unsafe). Changing this itself isn't such a simple
one to do, so I instead opted to fix this via locking.

Closes https://github.com/ppy/osu/issues/10777.
---
 osu.Game/Screens/Play/Spectator.cs | 80 ++++++++++++++++++------------
 1 file changed, 48 insertions(+), 32 deletions(-)

diff --git a/osu.Game/Screens/Play/Spectator.cs b/osu.Game/Screens/Play/Spectator.cs
index 0f593db277..6f51771c12 100644
--- a/osu.Game/Screens/Play/Spectator.cs
+++ b/osu.Game/Screens/Play/Spectator.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using JetBrains.Annotations;
 using osu.Framework.Allocation;
@@ -61,7 +62,9 @@ namespace osu.Game.Screens.Play
         [Resolved]
         private RulesetStore rulesets { get; set; }
 
-        private Replay replay;
+        private Score score;
+
+        private readonly object scoreLock = new object();
 
         private Container beatmapPanelContainer;
 
@@ -198,23 +201,32 @@ namespace osu.Game.Screens.Play
 
         private void userSentFrames(int userId, FrameDataBundle data)
         {
+            // this is not scheduled as it handles propagation of frames even when in a child screen (at which point we are not alive).
+            // probably not the safest way to handle this.
+
             if (userId != targetUser.Id)
                 return;
 
-            // this should never happen as the server sends the user's state on watching,
-            // but is here as a safety measure.
-            if (replay == null)
-                return;
-
-            foreach (var frame in data.Frames)
+            lock (scoreLock)
             {
-                IConvertibleReplayFrame convertibleFrame = rulesetInstance.CreateConvertibleReplayFrame();
-                convertibleFrame.FromLegacy(frame, beatmap.Value.Beatmap);
+                // this should never happen as the server sends the user's state on watching,
+                // but is here as a safety measure.
+                if (score == null)
+                    return;
 
-                var convertedFrame = (ReplayFrame)convertibleFrame;
-                convertedFrame.Time = frame.Time;
+                // rulesetInstance should be guaranteed to be in sync with the score via scoreLock.
+                Debug.Assert(rulesetInstance != null && rulesetInstance.RulesetInfo.Equals(score.ScoreInfo.Ruleset));
 
-                replay.Frames.Add(convertedFrame);
+                foreach (var frame in data.Frames)
+                {
+                    IConvertibleReplayFrame convertibleFrame = rulesetInstance.CreateConvertibleReplayFrame();
+                    convertibleFrame.FromLegacy(frame, beatmap.Value.Beatmap);
+
+                    var convertedFrame = (ReplayFrame)convertibleFrame;
+                    convertedFrame.Time = frame.Time;
+
+                    score.Replay.Frames.Add(convertedFrame);
+                }
             }
         }
 
@@ -247,10 +259,13 @@ namespace osu.Game.Screens.Play
             if (userId != targetUser.Id)
                 return;
 
-            if (replay != null)
+            lock (scoreLock)
             {
-                replay.HasReceivedAllFrames = true;
-                replay = null;
+                if (score != null)
+                {
+                    score.Replay.HasReceivedAllFrames = true;
+                    score = null;
+                }
             }
 
             Schedule(clearDisplay);
@@ -283,27 +298,28 @@ namespace osu.Game.Screens.Play
                 return;
             }
 
-            replay ??= new Replay { HasReceivedAllFrames = false };
-
-            var scoreInfo = new ScoreInfo
+            lock (scoreLock)
             {
-                Beatmap = resolvedBeatmap,
-                User = targetUser,
-                Mods = state.Mods.Select(m => m.ToMod(resolvedRuleset)).ToArray(),
-                Ruleset = resolvedRuleset.RulesetInfo,
-            };
+                score = new Score
+                {
+                    ScoreInfo = new ScoreInfo
+                    {
+                        Beatmap = resolvedBeatmap,
+                        User = targetUser,
+                        Mods = state.Mods.Select(m => m.ToMod(resolvedRuleset)).ToArray(),
+                        Ruleset = resolvedRuleset.RulesetInfo,
+                    },
+                    Replay = new Replay { HasReceivedAllFrames = false },
+                };
 
-            ruleset.Value = resolvedRuleset.RulesetInfo;
-            rulesetInstance = resolvedRuleset;
+                ruleset.Value = resolvedRuleset.RulesetInfo;
+                rulesetInstance = resolvedRuleset;
 
-            beatmap.Value = beatmaps.GetWorkingBeatmap(resolvedBeatmap);
-            watchButton.Enabled.Value = true;
+                beatmap.Value = beatmaps.GetWorkingBeatmap(resolvedBeatmap);
+                watchButton.Enabled.Value = true;
 
-            this.Push(new SpectatorPlayerLoader(new Score
-            {
-                ScoreInfo = scoreInfo,
-                Replay = replay,
-            }));
+                this.Push(new SpectatorPlayerLoader(score));
+            }
         }
 
         private void showBeatmapPanel(SpectatorState state)