[Bugfix] Crashes when reading the body in Release configurations

[ArnaudWurmel]

Fixes #96

Explanation

When the _value is set to a new value, the closure's parameter value is freed from memory. When this freed value was returned to the SelectorEventLoop, the app crashed.

To avoid this, I suggest to keep a strong reference to the value object, that way it won't be deleted.

[Goos]

Hey @ArnaudWurmel – thanks for looking into this, and sorry for not responding sooner.

Have you verified that this solves the crash you're seeing? I would have assumed that the value would live until the end of the closure as it being passed in as an argument should increment its retain count.

[ArnaudWurmel]

Hey @Goos,

We were using this branch temporally and it allowed us to read the body without having crashes. However, we shortly faced another crash when we launches multiple uitests on iOS 14 (the crash occurs after ~15 tests).

We didn't explore much the second issue because it could come from our end since all Embassy tests passes.

I would have assumed that the value would live until the end of the closure as it being passed in as an argument should increment its retain count

I thought the same, when I was trying to debug this, I displayed the value at each step and after the assignment of the _value property, we couldn't access to the closure's parameter value anymore. So I come with this fix but the issue could be deeper than that.

[Goos]

@ArnaudWurmel I just merged another fix related causing a BAD_ACCESS of the body, would you mind testing the latest master and seeing if it fixed your crash?

[ArnaudWurmel]

I still do have the same issue, try to run the unit tests of Embassy (on xcode 12) in Release mode (Edit scheme > Embassy-iOS > Test > Info > Build Configuration = Release) and enable testability in release (Target Embassy-iOS > Build Settings > Enabled Testability > Release = Yes)

[dubadub]

@Goos are there any code issues we need to fix to merge this PR?

Re: cooklang/cookcli#33.

[geofstro]

I don’t know if this is related; but I’m getting a crash under Monterey with the latest version of xCode 13.2.1 It happens at the fatalError line in “Transport” following “default:" } catch let OSError.ioError(number, message) { switch number { case EAGAIN: break // Apparently on macOS EPROTOTYPE can be returned when the socket is not // fully shutdown (as an EPIPE would indicate). Here we treat them // essentially the same since we just tear the transport down anyway. // http://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ case EPROTOTYPE: fallthrough case EPIPE: closedByPeer() default: fatalError("Failed to send, errno=\(number), message=\(message)") } } catch { fatalError("Failed to send") }

…

On 3 Jan 2022, at 19:32, Alexey Dubovskoy ***@***.***> wrote: @Goos <https://github.com/Goos> are there any code issues we need to fix to merge this PR? Re: cooklang/cookcli#33 <cooklang/cookcli#33>. — Reply to this email directly, view it on GitHub <#97 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVM4KMX27WLB2CIYRVXGX3UUHTU5ANCNFSM43OSSSSQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.

[Goos]

Sorry for the silence on this one folks. I haven't had time to dig any deeper into this issue since last time, but given that it's a small change and seems to work for the people trying it out, I'm going to go ahead and merge this.