From 48bdeaeff14f456abb371878307c91d79156832e Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Thu, 2 Nov 2023 02:42:36 +0900
Subject: [PATCH] Fix another potential crash in bubbles mod
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Storing `DrawableHitObject` for later use is not safe – especially
when accessing `HitObject` – as it's a pooled class. In the case here,
it's important to note that `PrepareForUse` can be called a frame or
more later in execution, which made this unsafe.

Closes https://github.com/ppy/osu/issues/24444.
---
 osu.Game.Rulesets.Osu/Mods/OsuModBubbles.cs | 77 ++++++++++-----------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModBubbles.cs b/osu.Game.Rulesets.Osu/Mods/OsuModBubbles.cs
index 9c0e43e96f..b34cc29741 100644
--- a/osu.Game.Rulesets.Osu/Mods/OsuModBubbles.cs
+++ b/osu.Game.Rulesets.Osu/Mods/OsuModBubbles.cs
@@ -2,11 +2,9 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
-using System.Diagnostics;
 using System.Linq;
 using osu.Framework.Bindables;
 using osu.Framework.Extensions.Color4Extensions;
-using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Colour;
 using osu.Framework.Graphics.Containers;
@@ -22,6 +20,7 @@
 using osu.Game.Rulesets.UI;
 using osu.Game.Scoring;
 using osuTK;
+using osuTK.Graphics;
 
 namespace osu.Game.Rulesets.Osu.Mods
 {
@@ -90,21 +89,18 @@ public void ApplyToDrawableHitObject(DrawableHitObject drawableObject)
                         break;
 
                     default:
-                        addBubble();
+                        BubbleDrawable bubble = bubblePool.Get();
+
+                        bubble.WasHit = drawable.IsHit;
+                        bubble.Position = getPosition(drawableOsuHitObject);
+                        bubble.AccentColour = drawable.AccentColour.Value;
+                        bubble.InitialSize = new Vector2(bubbleSize);
+                        bubble.FadeTime = bubbleFade;
+                        bubble.MaxSize = maxSize;
+
+                        bubbleContainer.Add(bubble);
                         break;
                 }
-
-                void addBubble()
-                {
-                    BubbleDrawable bubble = bubblePool.Get();
-
-                    bubble.DrawableOsuHitObject = drawableOsuHitObject;
-                    bubble.InitialSize = new Vector2(bubbleSize);
-                    bubble.FadeTime = bubbleFade;
-                    bubble.MaxSize = maxSize;
-
-                    bubbleContainer.Add(bubble);
-                }
             };
 
             drawableObject.OnRevertResult += (drawable, _) =>
@@ -118,18 +114,38 @@ void addBubble()
             };
         }
 
+        private Vector2 getPosition(DrawableOsuHitObject drawableObject)
+        {
+            switch (drawableObject)
+            {
+                // SliderHeads are derived from HitCircles,
+                // so we must handle them before to avoid them using the wrong positioning logic
+                case DrawableSliderHead:
+                    return drawableObject.HitObject.Position;
+
+                // Using hitobject position will cause issues with HitCircle placement due to stack leniency.
+                case DrawableHitCircle:
+                    return drawableObject.Position;
+
+                default:
+                    return drawableObject.HitObject.Position;
+            }
+        }
+
         #region Pooled Bubble drawable
 
         private partial class BubbleDrawable : PoolableDrawable
         {
-            public DrawableOsuHitObject? DrawableOsuHitObject { get; set; }
-
             public Vector2 InitialSize { get; set; }
 
             public float MaxSize { get; set; }
 
             public double FadeTime { get; set; }
 
+            public bool WasHit { get; set; }
+
+            public Color4 AccentColour { get; set; }
+
             private readonly Box colourBox;
             private readonly CircularContainer content;
 
@@ -157,15 +173,12 @@ public BubbleDrawable()
 
             protected override void PrepareForUse()
             {
-                Debug.Assert(DrawableOsuHitObject.IsNotNull());
-
-                Colour = DrawableOsuHitObject.IsHit ? Colour4.White : Colour4.Black;
+                Colour = WasHit ? Colour4.White : Colour4.Black;
                 Scale = new Vector2(1);
-                Position = getPosition(DrawableOsuHitObject);
                 Size = InitialSize;
 
                 //We want to fade to a darker colour to avoid colours such as white hiding the "ripple" effect.
-                ColourInfo colourDarker = DrawableOsuHitObject.AccentColour.Value.Darken(0.1f);
+                ColourInfo colourDarker = AccentColour.Darken(0.1f);
 
                 // The absolute length of the bubble's animation, can be used in fractions for animations of partial length
                 double duration = 1700 + Math.Pow(FadeTime, 1.07f);
@@ -178,7 +191,7 @@ protected override void PrepareForUse()
                     .ScaleTo(MaxSize * 1.5f, duration * 0.2f, Easing.OutQuint)
                     .FadeOut(duration * 0.2f, Easing.OutCirc).Expire();
 
-                if (!DrawableOsuHitObject.IsHit) return;
+                if (!WasHit) return;
 
                 content.BorderThickness = InitialSize.X / 3.5f;
                 content.BorderColour = Colour4.White;
@@ -192,24 +205,6 @@ protected override void PrepareForUse()
                        // Avoids transparency overlap issues during the bubble "pop"
                        .TransformTo(nameof(BorderThickness), 0f);
             }
-
-            private Vector2 getPosition(DrawableOsuHitObject drawableObject)
-            {
-                switch (drawableObject)
-                {
-                    // SliderHeads are derived from HitCircles,
-                    // so we must handle them before to avoid them using the wrong positioning logic
-                    case DrawableSliderHead:
-                        return drawableObject.HitObject.Position;
-
-                    // Using hitobject position will cause issues with HitCircle placement due to stack leniency.
-                    case DrawableHitCircle:
-                        return drawableObject.Position;
-
-                    default:
-                        return drawableObject.HitObject.Position;
-                }
-            }
         }
 
         #endregion