From 471eea750af41ec00b34fe4a22f928b2524d87e8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 29 Dec 2021 21:17:44 +0900 Subject: [PATCH 1/5] Fix calling `SkinEditorOverlay.Show` before the overlay is loaded causing an exception As seen at https://github.com/ppy/osu/runs/4652969942?check_suite_focus=true. --- osu.Game/Skinning/Editor/SkinEditorOverlay.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs index 340c6ed931..6699e0f658 100644 --- a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs +++ b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs @@ -73,9 +73,13 @@ namespace osu.Game.Skinning.Editor skinEditor = new SkinEditor(target); skinEditor.State.BindValueChanged(editorVisibilityChanged); - Debug.Assert(skinEditor != null); - - LoadComponentAsync(skinEditor, AddInternal); + // Schedule ensures that if `Show` is called before this overlay is loaded, + // it will not throw (LoadComponentAsync requires the load target to be in a loaded state). + Schedule(() => + { + Debug.Assert(skinEditor != null); + LoadComponentAsync(skinEditor, AddInternal); + }); } else skinEditor.Show(); From b1a444180fda72619ae33667411d28989d674cda Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 29 Dec 2021 21:46:34 +0900 Subject: [PATCH 2/5] Fix `Show` then `Reset` potentially resulting in incorrect load target --- osu.Game/Skinning/Editor/SkinEditorOverlay.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs index 6699e0f658..ebf0a1214e 100644 --- a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs +++ b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs @@ -70,16 +70,18 @@ namespace osu.Game.Skinning.Editor // base call intentionally omitted. if (skinEditor == null) { - skinEditor = new SkinEditor(target); - skinEditor.State.BindValueChanged(editorVisibilityChanged); + var editor = new SkinEditor(target); + editor.State.BindValueChanged(editorVisibilityChanged); // Schedule ensures that if `Show` is called before this overlay is loaded, // it will not throw (LoadComponentAsync requires the load target to be in a loaded state). Schedule(() => { - Debug.Assert(skinEditor != null); - LoadComponentAsync(skinEditor, AddInternal); + Debug.Assert(editor != null); + LoadComponentAsync(editor, AddInternal); }); + + skinEditor = editor; } else skinEditor.Show(); From ef49f2ed0e4835498d3d6b9b92431a59977d0700 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 30 Dec 2021 16:02:08 +0900 Subject: [PATCH 3/5] Add extra extra safety against attempting to load a previously expired editor --- osu.Game/Skinning/Editor/SkinEditorOverlay.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs index ebf0a1214e..87da67dae0 100644 --- a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs +++ b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs @@ -73,15 +73,23 @@ namespace osu.Game.Skinning.Editor var editor = new SkinEditor(target); editor.State.BindValueChanged(editorVisibilityChanged); + skinEditor = editor; + // Schedule ensures that if `Show` is called before this overlay is loaded, // it will not throw (LoadComponentAsync requires the load target to be in a loaded state). Schedule(() => { - Debug.Assert(editor != null); - LoadComponentAsync(editor, AddInternal); - }); + if (editor != skinEditor) + return; - skinEditor = editor; + LoadComponentAsync(editor, _ => + { + if (editor != skinEditor) + return; + + AddInternal(editor); + }); + }); } else skinEditor.Show(); From 089b756f932b1b59a4b66380553a9d0254d8a121 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 30 Dec 2021 16:03:16 +0900 Subject: [PATCH 4/5] Invert logic to make reading easier --- osu.Game/Skinning/Editor/SkinEditorOverlay.cs | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs index 87da67dae0..86854ab6ff 100644 --- a/osu.Game/Skinning/Editor/SkinEditorOverlay.cs +++ b/osu.Game/Skinning/Editor/SkinEditorOverlay.cs @@ -1,7 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System.Diagnostics; using JetBrains.Annotations; using osu.Framework.Bindables; using osu.Framework.Graphics; @@ -67,32 +66,34 @@ namespace osu.Game.Skinning.Editor public override void Show() { - // base call intentionally omitted. - if (skinEditor == null) + // base call intentionally omitted as we have custom behaviour. + + if (skinEditor != null) { - var editor = new SkinEditor(target); - editor.State.BindValueChanged(editorVisibilityChanged); + skinEditor.Show(); + return; + } - skinEditor = editor; + var editor = new SkinEditor(target); + editor.State.BindValueChanged(editorVisibilityChanged); - // Schedule ensures that if `Show` is called before this overlay is loaded, - // it will not throw (LoadComponentAsync requires the load target to be in a loaded state). - Schedule(() => + skinEditor = editor; + + // Schedule ensures that if `Show` is called before this overlay is loaded, + // it will not throw (LoadComponentAsync requires the load target to be in a loaded state). + Schedule(() => + { + if (editor != skinEditor) + return; + + LoadComponentAsync(editor, _ => { if (editor != skinEditor) return; - LoadComponentAsync(editor, _ => - { - if (editor != skinEditor) - return; - - AddInternal(editor); - }); + AddInternal(editor); }); - } - else - skinEditor.Show(); + }); } private void editorVisibilityChanged(ValueChangedEvent visibility) From 7cdba2f7c3a915f8f8d8c9996d8689c1ecb4d399 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 3 Jan 2022 21:50:00 +0100 Subject: [PATCH 5/5] Add test coverage of score submission if player is exited during import --- .../TestScenePlayerScoreSubmission.cs | 44 ++++++++++++++++--- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerScoreSubmission.cs b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerScoreSubmission.cs index 324a132120..25808d307d 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerScoreSubmission.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerScoreSubmission.cs @@ -3,6 +3,7 @@ using System; using System.Linq; +using System.Threading; using System.Threading.Tasks; using NUnit.Framework; using osu.Framework.Screens; @@ -36,7 +37,9 @@ namespace osu.Game.Tests.Visual.Gameplay protected override bool HasCustomSteps => true; - protected override TestPlayer CreatePlayer(Ruleset ruleset) => new NonImportingPlayer(false); + protected override TestPlayer CreatePlayer(Ruleset ruleset) => new FakeImportingPlayer(false); + + protected new FakeImportingPlayer Player => (FakeImportingPlayer)base.Player; protected override Ruleset CreatePlayerRuleset() => createCustomRuleset?.Invoke() ?? new OsuRuleset(); @@ -207,6 +210,25 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("ensure failing submission", () => Player.SubmittedScore?.ScoreInfo.Passed == false); } + [Test] + public void TestSubmissionOnExitDuringImport() + { + prepareTokenResponse(true); + + createPlayerTest(); + AddStep("block imports", () => Player.AllowImportCompletion.Wait()); + + AddUntilStep("wait for token request", () => Player.TokenCreationRequested); + + addFakeHit(); + + AddUntilStep("wait for import to start", () => Player.ScoreImportStarted); + + AddStep("exit", () => Player.Exit()); + AddStep("allow import to proceed", () => Player.AllowImportCompletion.Release(1)); + AddAssert("ensure submission", () => Player.SubmittedScore != null && Player.ImportedScore != null); + } + [Test] public void TestNoSubmissionOnLocalBeatmap() { @@ -288,15 +310,26 @@ namespace osu.Game.Tests.Visual.Gameplay }); } - private class NonImportingPlayer : TestPlayer + protected class FakeImportingPlayer : TestPlayer { - public NonImportingPlayer(bool allowPause = true, bool showResults = true, bool pauseOnFocusLost = false) + public bool ScoreImportStarted { get; set; } + public SemaphoreSlim AllowImportCompletion { get; } + public Score ImportedScore { get; private set; } + + public FakeImportingPlayer(bool allowPause = true, bool showResults = true, bool pauseOnFocusLost = false) : base(allowPause, showResults, pauseOnFocusLost) { + AllowImportCompletion = new SemaphoreSlim(1); } - protected override Task ImportScore(Score score) + protected override async Task ImportScore(Score score) { + ScoreImportStarted = true; + + await AllowImportCompletion.WaitAsync().ConfigureAwait(false); + + ImportedScore = score; + // It was discovered that Score members could sometimes be half-populated. // In particular, the RulesetID property could be set to 0 even on non-osu! maps. // We want to test that the state of that property is consistent in this test. @@ -311,8 +344,7 @@ namespace osu.Game.Tests.Visual.Gameplay // In the above instance, if a ScoreInfo with Ruleset = {mania} and RulesetID = 0 is attached to an EF context, // RulesetID WILL BE SILENTLY SET TO THE CORRECT VALUE of 3. // - // For the above reasons, importing is disabled in this test. - return Task.CompletedTask; + // For the above reasons, actual importing is disabled in this test. } } }