Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reflection-related iOS crashes #29992

Merged
merged 4 commits into from
Sep 26, 2024
Merged

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Sep 25, 2024

This is one half of the fixes required to get iOS to run.

The crashes can be reproed on device or simulator by running in Release configuration (with <MtouchUseLlvm>false</MtouchUseLlvm> to reduce compile time, but also fails without it), and trying to do certain actions:

  • Opening the beatmap listing.
  • Downloading a beatmap (e.g. via first run overlay)
  • Finishing a play and/or going to the results screen.
  • Turning on a mod with settings (e.g. DoubleTime, ModAccuracyChallenge, ModDifficultyAdjust).
  • Transferring a mod with non-default settings between rulesets.

All these cases are fixed with this change. Additional to the above, I've also tested the following extra scenarios:

  • Creating a skin.
  • Changing skins (including to a custom one).
  • Editing a beatmap.
  • Copying mods from the leaderboard.
  • Entering multiplayer, entering gameplay.
  • Downloading a score + playing the replay.

The stack trace always ends up in some sort of reflection
#00x000000010f25d4ac in mono_interp_exec_method at /Users/runner/work/1/s/src/mono/mono/mini/interp/interp.c:4116
#10x000000010f25a800 in interp_entry_from_trampoline at /Users/runner/work/1/s/src/mono/mono/mini/interp/interp.c:3088
#20x000000010edba548 in wrapper_other_object___interp_lmf_mono_interp_entry_from_trampoline_intptr_intptr ()
#30x000000010ee02b78 in native_to_interp_trampoline ()
#40x000000010edfe30c in gsharedvt_out_trampoline ()
#50x000000010ed36448 in System_Reflection_RuntimePropertyInfo_GetterAdapterFrame_T_GSHAREDVT_R_GSHAREDVT_System_Reflection_RuntimePropertyInfo_Getter_2_T_GSHAREDVT_R_GSHAREDVT_object ()
#60x000000010eafb5a4 in System_Reflection_RuntimePropertyInfo_GetValue_object_object__ ()
#70x000000010eb0f2f4 in System_Reflection_PropertyInfo_GetValue_object ()
#80x000000010ba279ec in Realms_RealmResultsVisitor_TryExtractConstantValue_System_Linq_Expressions_Expression_object_ at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\Linq\RealmResultsVisitor.cs:582
#90x000000010ba28198 in Realms_RealmResultsVisitor_VisitBinary_System_Linq_Expressions_BinaryExpression at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\Linq\RealmResultsVisitor.cs:623
#100x000000010d963040 in System_Linq_Expressions_BinaryExpression_Accept_System_Linq_Expressions_ExpressionVisitor at /<unknown>:1
#110x000000010d987090 in System_Linq_Expressions_ExpressionVisitor_Visit_System_Linq_Expressions_Expression at /<unknown>:1
#120x000000010ba27588 in Realms_RealmResultsVisitor_VisitCombination_System_Linq_Expressions_BinaryExpression_System_Action_1_Realms_QueryHandle at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\Linq\RealmResultsVisitor.cs:535
#130x000000010ba27bec in Realms_RealmResultsVisitor_VisitBinary_System_Linq_Expressions_BinaryExpression at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\Linq\RealmResultsVisitor.cs:596
#140x000000010d963040 in System_Linq_Expressions_BinaryExpression_Accept_System_Linq_Expressions_ExpressionVisitor at /<unknown>:1
#150x000000010d987090 in System_Linq_Expressions_ExpressionVisitor_Visit_System_Linq_Expressions_Expression at /<unknown>:1
#160x000000010ba241a0 in Realms_RealmResultsVisitor_VisitMethodCall_System_Linq_Expressions_MethodCallExpression at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\Linq\RealmResultsVisitor.cs:205
#170x000000010d98f4d0 in System_Linq_Expressions_MethodCallExpression_Accept_System_Linq_Expressions_ExpressionVisitor at /<unknown>:1
#180x000000010d987090 in System_Linq_Expressions_ExpressionVisitor_Visit_System_Linq_Expressions_Expression at /<unknown>:1
#190x000000010ba22748 in Realms_RealmResults_1_T_REF_GetOrCreateHandle at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\Linq\RealmResults.cs:75
#200x000000010e8d9f40 in System_Lazy_1_T_REF_ViaFactory_System_Threading_LazyThreadSafetyMode ()
#210x000000010e8da15c in System_Lazy_1_T_REF_ExecutionAndPublication_System_LazyHelper_bool ()
#220x000000010e8da5d8 in System_Lazy_1_T_REF_CreateValue ()
#230x000000010e8da770 in System_Lazy_1_T_REF_get_Value ()
#240x000000010ba77798 in Realms_NotificationCallbacks_1__c__DisplayClass3_0_T_REF__Addb__0 at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\DatabaseTypes\RealmCollectionBase.cs:630
#250x000000010ba325c0 in Realms_Realm_ExecuteOutsideTransaction_System_Action at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\Realm.cs:1341
#260x000000010b9fc0c0 in Realms_NotificationCallbacks_1_T_REF_Add_Realms_NotificationCallbackDelegate_1_T_REF_bool at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\DatabaseTypes\RealmCollectionBase.cs:624
#270x000000010b9f8858 in Realms_RealmCollectionBase_1_T_REF_SubscribeForNotificationsImpl_Realms_NotificationCallbackDelegate_1_T_REF_bool at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\DatabaseTypes\RealmCollectionBase.cs:180
#280x000000010b9f8750 in Realms_RealmCollectionBase_1_T_REF_SubscribeForNotifications_Realms_NotificationCallbackDelegate_1_T_REF at /D:\a\realm-dotnet\realm-dotnet\Realm\Realm\DatabaseTypes\RealmCollectionBase.cs:175
#290x000000010d040578 in osu_Game_Database_RealmObjectExtensions_QueryAsyncWithNotifications_T_REF_Realms_IRealmCollection_1_T_REF_Realms_NotificationCallbackDelegate_1_T_REF at /Users/smgi/Repos/osu/osu.Game/Database/RealmObjectExtensions.cs:296
#300x000000010d040658 in osu_Game_Database_RealmObjectExtensions_QueryAsyncWithNotifications_T_REF_System_Linq_IQueryable_1_T_REF_Realms_NotificationCallbackDelegate_1_T_REF at /Users/smgi/Repos/osu/osu.Game/Database/RealmObjectExtensions.cs:339

This would be a .NET/Mono regression, but I haven't isolated/reported it yet. The bindable-related commit lends perhaps a hint as to why this is happening - in the case of IBindable<>, the interface can be implemented multiple times on leaf objects so the runtime is unsure which property to resolve, even if there is only one matching implementation in the leaf type.

I gather similar is happening with IHasOnlineID<> leading to the Realm issue, though in that case (and because it's the first issue I resolved) I've taken a sledgehammer to it by avoiding any inner references.

The bindable change can continue existing going into the future, because, while it's not trying to cover for every possible use-case ever, it's also adding a bit of safety for truly undefined scenarios.

I will be reporting the issue later, time-provided.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dear lord.

so long as it works i guess (i can't test, obviously)

osu.Game/Utils/BindableValueAccessor.cs Outdated Show resolved Hide resolved
osu.Game/Utils/BindableValueAccessor.cs Outdated Show resolved Hide resolved
@smoogipoo
Copy link
Contributor Author

One sec - I found another case.

@smoogipoo
Copy link
Contributor Author

False alarm... My files became out of date during some more iOS testing.

@peppy
Copy link
Member

peppy commented Sep 26, 2024

For every new usage of realm, we will need to keep in mind that custom handling will be necessary to allow iOS to work, yeah? This seems absolutely fucked and bound to fail sooner or later (but I guess we just have to live with this?)

I'd also want the changes in this PR do have loud inline comments in each case. Because the chance of it getting changed without realising it will break iOS is high (and we have no automated coverage of this).

@peppy peppy self-requested a review September 26, 2024 06:36
@bdach
Copy link
Collaborator

bdach commented Sep 26, 2024

This seems absolutely fucked and bound to fail sooner or later (but I guess we just have to live with this?)

Honestly I see it as not that different from every single mono-related explosion after we used a new language feature in an unrelated change. See ppy/osu-framework#6162, ppy/osu-framework#5571, etc.

@peppy
Copy link
Member

peppy commented Sep 26, 2024

Honestly I see it as not that different from every single mono-related explosion after we used a new language feature in an unrelated change.

Yeah I get that. I'm just disgruntled each time. There's not much we can do without having runtime tests running on iOS (probably not worth it yet). I think deploying working to app store and relying on testflight testing is best.

@smoogipoo
Copy link
Contributor Author

This seems absolutely fucked and bound to fail sooner or later

I agree. I will be trying to isolate and report this to .NET, and will revert at least the Realm-related changes if it gets fixed. It's definitely a bug.

@peppy peppy merged commit 78c1426 into ppy:master Sep 26, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants