search

shaps80 / SwiftUIBackports

A collection of SwiftUI backports for iOS, macOS, tvOS and watchOS
MIT License
931 stars 59 forks source link

Fix endless recursion by avoiding to link _delegates more than once #63

Closed roland-schmitz-ocu closed 9 months ago

roland-schmitz-ocu commented 10 months ago

In the overrides to willMove(toParent parent: UIViewController?) in Detents.swift and also InteractiveDismiss.swift the controller injects itself to the chain of delegates. Apparently the function is called more than once for the same controller if the device orientation changes. When a sheet is used with both .backport.presentationDetents([.medium]) and .backport.interactiveDismissDisabled(true) then multiple willMove calls will result in a cycle in the linked delegate which will then result in an endless recursion.

To avoid the recursion and the crash we just check if _delegate has not been set before.

Resolves: #62

shaps80 commented 10 months ago

I appreciate your PR here, but I think Representable.Controller.responds(to:) is actually where you'll need to look for a fix. Your delegate fix I don't think is right. It likely resolves the recursion but I'm certain is invalid code and would introduce other issues when combining modifiers, not to mention likely break some SwiftUI built-in stuff as well.

From your stack trace it looks more obvious to me that the recursion is occurring inside the Representable.Controller.responds(to:) call which makes a lot more sense.

Could also be limited to a newer iOS version? What did you test with? Xcode and iOS versions please?

roland-schmitz-ocu commented 10 months ago

Thanks for having a look in this PR. My initial fix was not quite correct and I changed the logic now.

I still think that the issue can only be fixed in the willMove(toParent) overrides in Detents.swift and InteractiveDismiss.swift as the current implementation creates a cycle in the controller.delegate chain when you use the sample code from the bug report. It happens only if you use multiple backported modifiers (presentationDetents and .interactiveDismissDisabled) and change the orientation of the device. When the orientation changes the willMove(toParent) is called a second time. The fix avoids linking the delegates if this has already been done before.

As stated in the bug report the crash happens on iOS 16 when run with Xcode 14.3.1.

shaps80 commented 10 months ago

Thanks for this, I missed the Issue when I initially reviewed the PR, so thanks for adding that and the versions etc 👍

I haven't had a chance to create a sample project myself, but I reviewed the stack track and while I agree the delegate dance is where the issue is, I think the trace better indicates that this is related to the responds(to:) method, but I could be wrong.

Leave this with me and over the weekend I'll try and find some time to review it myself. I just want to be careful that its the right fix and that we don't break anything for existing users 👍

Btw, once thing that made me think your direction might not be the right one, is that you had invalid code but I assume you tested and it "worked" for you in some instance. That to me indicates that it's not a reliable fix and/or the bug is a race condition and doesn't present reliably.

Again, I'll try and debug this myself, cause I need 100% reproducible results in order to trust the fix 👍

I deeply appreciate your contribution here thank you!

roland-schmitz-ocu commented 10 months ago

Added a third commit to not only address issue #62 but also issue #61. I Tested it on iOS 15, iOS 16 and iOS 17 and it always solves the issues.

@shaps80 please consider merging it as it solves the issues #62 and #61.