refactor: make ISceneTipsProvider synchronous and release tips unconditionally

Both implementations resolve synchronously (the localization provider serves a
cached table loaded in InitializeAsync, and the feature-flag decorator only
transforms that result), so the async signature suggested a suspension point
that cannot happen. Making Get() sync and fetching in OnBeforeViewShow means
tips is populated on every path that reaches OnViewClose, so the release there
needs no emptiness guard (review feedback on the previous guard).

Author: alejandro-jimenez-dcl

Explorer/Assets/DCL/SceneLoadingScreens/ISceneTipsProvider.cs

@@ -1,13 +1,9 @@
-using Cysharp.Threading.Tasks;
-using System.Threading;
-using UnityEngine;
-
 namespace DCL.SceneLoadingScreens
 {
     public interface ISceneTipsProvider
     {
-        // TODO: in the future we may require the parcel coordinate to provide specific scene tips
-        // UniTask<SceneTips> Get(Vector2Int parcelCoord, CancellationToken ct);
-        UniTask<SceneTips> GetAsync(CancellationToken ct);
+        // TODO: in the future we may require the parcel coordinate to provide specific scene tips,
+        // which would make this async again: UniTask<SceneTips> GetAsync(Vector2Int parcelCoord, CancellationToken ct)
+        SceneTips Get();
     }
 }

Explorer/Assets/DCL/SceneLoadingScreens/SceneLoadingScreenController.cs

@@ -104,6 +104,13 @@ protected override void OnBeforeViewShow()
             BlockUnwantedInputs();
             SetLoadProgress(0);
             viewInstance!.ClearTips();
+
+            // Fetched synchronously before the view shows, so `tips` is populated on every path
+            // that can reach OnViewClose and the release there needs no emptiness guard.
+            tips = sceneTipsProvider.Get();
+
+            if (tips.Random)
+                tips.Tips.Shuffle();
         }
 
         protected override void OnViewShow()
@@ -137,19 +144,12 @@ protected override void OnViewClose()
             audioMixerVolumesController.UnmuteGroup(AudioMixerExposedParam.Chat_Volume);
 
             viewInstance!.ClearTips();
-
-            // Tips is null until the first load completes: a close racing that load must not
-            // release a default instance, and a later close must not release the same tips twice.
-            if (tips.Tips != null)
-            {
-                tips.Release();
-                tips = default;
-            }
+            tips.Release();
         }
 
         protected override async UniTask WaitForCloseIntentAsync(CancellationToken ct)
         {
-            await LoadTipsAsync(ct);
+            await LoadTipsAsync();
 
             ShowTip(currentTip.Value);
 
@@ -193,13 +193,8 @@ private async UniTask WaitTimeThresholdAsync(float progressProportion, Cancellat
             }
         }
 
-        private async UniTask LoadTipsAsync(CancellationToken ct)
+        private async UniTask LoadTipsAsync()
         {
-            tips = await sceneTipsProvider.GetAsync(ct);
-
-            if (tips.Random)
-                tips.Tips.Shuffle();
-
             using var scope = ListPool<UniTask<SceneTips.LoadedTip>>.Get(out var tasks);
             foreach (SceneTips.Tip tip in tips.Tips) tasks.Add(tip.LoadAsync());
             var loaded = await UniTask.WhenAll(tasks!);

Explorer/Assets/DCL/SceneLoadingScreens/Tests/SceneLoadingScreenControllerInputBlockShould.cs

@@ -15,7 +15,6 @@
 using System.Threading;
 using System.Threading.Tasks;
 using UnityEditor;
-using UnityEngine;
 using UnityEngine.Audio;
 using UnityEngine.TestTools;
 using Object = UnityEngine.Object;
@@ -53,11 +52,11 @@ public class SceneLoadingScreenControllerInputBlockShould
         // suppression is still active, instead of leaking into an unrelated later test.
         private const int FLUSH_FRAME_COUNT = 30;
 
-        private World world;
+        private World? world;
         private SingleInstanceEntity inputMapEntity;
-        private IInputBlock inputBlock;
-        private SceneLoadingScreenView viewInstance;
-        private AudioMixerVolumesController audioMixerVolumesController;
+        private IInputBlock? inputBlock;
+        private SceneLoadingScreenView? viewInstance;
+        private AudioMixerVolumesController? audioMixerVolumesController;
         private bool originalIgnoreFailingMessages;
 
         [OneTimeSetUp]
@@ -119,7 +118,7 @@ public void TearDown()
             if (viewInstance != null)
                 Object.DestroyImmediate(viewInstance.gameObject);
 
-            world.Dispose();
+            world!.Dispose();
 
             // Reset the static field so later tests in the same run aren't left with a stale in-memory
             // prefs instance (mirrors the reset half of the same established pattern).
@@ -134,12 +133,7 @@ public async Task ReleaseInputBlockWhenCloseIntentIsCancelledAsync()
             // suppression - the flag must be raised inside the test body itself.
             LogAssert.ignoreFailingMessages = true;
 
-            ISceneTipsProvider tipsProvider = Substitute.For<ISceneTipsProvider>();
-
-            tipsProvider.GetAsync(Arg.Any<CancellationToken>())
-                        .Returns(UniTask.FromResult(new SceneTips(TimeSpan.Zero, false, new List<SceneTips.Tip>())));
-
-            SceneLoadingScreenController controller = CreateController(tipsProvider);
+            SceneLoadingScreenController controller = CreateController(EmptyTipsProvider());
 
             using var cts = new CancellationTokenSource();
             cts.Cancel();
@@ -168,50 +162,30 @@ public async Task ReleaseInputBlockWhenCloseIntentIsCancelledAsync()
         }
 
         [Test]
-        public async Task ReleaseInputBlockWhenCloseRacesTheInitialTipsLoadAsync()
+        public async Task ReleaseInputBlockWhenClosedWhileSceneIsStillLoadingAsync()
         {
             // The framework resets LogAssert state at test start, wiping any fixture/SetUp-scoped
             // suppression - the flag must be raised inside the test body itself.
             LogAssert.ignoreFailingMessages = true;
 
-            ISceneTipsProvider tipsProvider = Substitute.For<ISceneTipsProvider>();
-
-            // The tips load never resolves during this test, so `tips` stays at its default value
-            // (Tips == null) for the whole run - exactly the close-races-the-initial-load scenario from
-            // review.md ("earliest-cancel variant... cold addressables make the tips window largest on
-            // first show") that made the first patch attempt's placement of the release - after
-            // tips.Release() - still leak, because tips.Release() throws on a default SceneTips before
-            // reaching it.
-            tipsProvider.GetAsync(Arg.Any<CancellationToken>()).Returns(UniTask.Never<SceneTips>(CancellationToken.None));
-
-            SceneLoadingScreenController controller = CreateController(tipsProvider);
+            SceneLoadingScreenController controller = CreateController(EmptyTipsProvider());
 
-            // Deliberately not awaited: LaunchViewLifeCycleAsync runs synchronously through
-            // OnBeforeViewShow/OnViewShow and suspends inside LoadTipsAsync (awaiting a promise that
-            // never completes), so by the time control returns here the block has already been
-            // acquired and WaitForCloseIntentAsync is still in flight - matching the real
-            // MVCManager.ShowOverlayAsync race where the teardown's finally can call HideViewAsync while
-            // the orphaned lifecycle task is still suspended.
-            UniTask launch = controller.LaunchViewLifeCycleAsync(new CanvasOrdering(CanvasOrdering.SortingLayer.Overlay, 0), CompletedParams(), CancellationToken.None);
+            // A load report that never completes keeps WaitForCloseIntentAsync suspended waiting on it,
+            // so the fade-out (the unpatched code's only release) never runs. Deliberately not awaited:
+            // this matches the real MVCManager.ShowOverlayAsync race where the teardown's finally calls
+            // HideViewAsync while the orphaned lifecycle task is still suspended (teleport superseded
+            // mid-load - leak path 2 from report.md).
+            AsyncLoadProcessReport pendingReport = AsyncLoadProcessReport.Create(CancellationToken.None);
+            UniTask launch = controller.LaunchViewLifeCycleAsync(new CanvasOrdering(CanvasOrdering.SortingLayer.Overlay, 0), new SceneLoadingScreenController.Params(pendingReport), CancellationToken.None);
 
             Assert.That(ActiveKinds(), Is.EqualTo(ALL_KINDS & ~BLOCKED_BY_LOADING_SCREEN),
                 "input should be blocked right after showing the loading screen");
 
-            try
-            {
-                await ((IController)controller).HideViewAsync(CancellationToken.None);
-            }
-            catch (NullReferenceException)
-            {
-                // Pre-existing, separate defect (review.md finding 1): unpatched OnViewClose() calls
-                // tips.Release() on a still-default `tips` and throws. That defect is not what is under
-                // test here - what matters is whether the input block was released before that
-                // statement could run at all, which is asserted below regardless of this exception.
-            }
+            await ((IController)controller).HideViewAsync(CancellationToken.None);
 
             Assert.That(ActiveKinds(), Is.EqualTo(ALL_KINDS),
-                "BLOCK_USER_INPUT must be released even when OnViewClose races the initial tips load - " +
-                "unpatched, this leaks +1 on the refcount forever (#9502)");
+                "BLOCK_USER_INPUT must be released even when the loading screen closes while the scene " +
+                "is still loading - unpatched, this leaks +1 on the refcount forever (#9502)");
 
             launch.Forget();
 
@@ -220,8 +194,15 @@ public async Task ReleaseInputBlockWhenCloseRacesTheInitialTipsLoadAsync()
             await FlushDeferredViewLogsAsync();
         }
 
+        private static ISceneTipsProvider EmptyTipsProvider()
+        {
+            ISceneTipsProvider tipsProvider = Substitute.For<ISceneTipsProvider>();
+            tipsProvider.Get().Returns(new SceneTips(TimeSpan.Zero, false, new List<SceneTips.Tip>()));
+            return tipsProvider;
+        }
+
         private SceneLoadingScreenController CreateController(ISceneTipsProvider tipsProvider) =>
-            new (() => viewInstance, tipsProvider, TimeSpan.Zero, audioMixerVolumesController, inputBlock);
+            new (() => viewInstance!, tipsProvider, TimeSpan.Zero, audioMixerVolumesController!, inputBlock!);
 
         private static SceneLoadingScreenController.Params CompletedParams()
         {
@@ -231,7 +212,7 @@ private static SceneLoadingScreenController.Params CompletedParams()
         }
 
         private InputMapComponent.Kind ActiveKinds() =>
-            inputMapEntity.GetInputMapComponent(world).Active;
+            inputMapEntity.GetInputMapComponent(world!).Active;
 
         private static InputMapComponent.Kind AllKinds()
         {

Explorer/Assets/DCL/SceneLoadingScreens/TipsFromFeatureFlagDecorator.cs

@@ -1,11 +1,9 @@
-using Cysharp.Threading.Tasks;
 using DCL.FeatureFlags;
 using DCL.PerformanceAndDiagnostics.Analytics;
 using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Runtime.Serialization;
-using System.Threading;
 
 namespace DCL.SceneLoadingScreens
 {
@@ -30,7 +28,7 @@ public TipsFromFeatureFlagDecorator(ISceneTipsProvider legacyTips)
             this.legacyTips = legacyTips;
         }
 
-        public async UniTask<SceneTips> GetAsync(CancellationToken ct)
+        public SceneTips Get()
         {
             if (!featureFlagChecked)
             {
@@ -42,7 +40,7 @@ public async UniTask<SceneTips> GetAsync(CancellationToken ct)
                 featureFlagChecked = true;
             }
 
-            SceneTips originTips = await legacyTips.GetAsync(ct);
+            SceneTips originTips = legacyTips.Get();
 
             if (audienceTipsParseSuccess)
             {

Explorer/Assets/DCL/SceneLoadingScreens/UnityLocalizationSceneTipsProvider.cs

@@ -46,7 +46,7 @@ public async UniTask InitializeAsync(CancellationToken ct)
             fallbackTips = Get(tipsTable, imagesTable, ct);
         }
 
-        public async UniTask<SceneTips> GetAsync(CancellationToken ct) =>
+        public SceneTips Get() =>
 
             // TODO: we will need specific scene tips in the future, but its disabled at the moment
             /*StringTable tipsTable = await tipsDatabase.GetTableAsync($"LoadingSceneTips-{parcelCoord.x},{parcelCoord.y}").Task