[Firebase/Performance] GULObjectSwizzler crashes on objc_disposeClassPair.

[asafkorem]

[REQUIRED] Step 1: Describe your environment

• Xcode version: 13.1
• Firebase SDK version: 8.10
• Installation method: Any
• Firebase Component: Firebase
• Target platform(s): iOS

[REQUIRED] Step 2: Describe the problem

Steps to reproduce:

When using Firebase Performance along with Detox Sync (a framework used by Detox), there is a double ISA Swizzling by the components. This causes a crash when NSZombiesEnabled=NO.

• Firebase Performance does ISA Swizzling (using GULObjectSwizzler).
• Detox Sync does ISA Swizzling, with a dynamic subclass of the generated class, using DTXSwizzlingHelper.
• GULObjectSwizzler calls to dispose its own generated subclasses using objc_disposeClassPair().
• Crash with the error message: objc_disposeClassPair: class 'fir_<some uuid>_<class name>' still has subclasses, including 'fir_<some uuid>_<class name>(__detox_sync_<class name>)'!

---

This issue seems to be related: google/GoogleUtilities#15.
Also, another explanation of a similar issue, with zombie objects: #8321 (comment) (instead Zombie objects, we have DetoxSync objects).

---

This branch of DetoxSync reproduces the issue: https://github.com/asafkorem/DetoxSync/tree/feature/reproduce-detox-sync-crash: build and run ExampleApp/TimersTest.xcodeproj with NSZombiesEnabled=NO.

[google-oss-bot]

I found a few problems with this issue:

• I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
• This issue does not seem to follow the issue template. Make sure you provide all the required information.

[mikehardy]

@paulb777 just to note this is of intense interest in react-native-firebase repo, our test harness is based entirely on Detox, and last time I did a survey of available e2e test tools, Detox still won, even with the horrific ios e2e crash frequency we're taking now. Would ❤️ to see some sort of fix or remediation or something here :-)

[d4vidi]

Thanks @mikehardy, and sorry about the inconvenience around this thus far. Looks like we're really getting somewhere now.

[paulb777]

@asafkorem @mikehardy Thanks for the detailed report and background. We'll take a look. It's one of our priorities to overhaul how Firebase uses swizzling. However, a resolution may still be months out.

In the meantime, is omitting FirebasePerformance when Detox is used a viable workaround?

[mikehardy]

It is possible but is equivalent to saying omit performance from e2e testing for us

[asafkorem]

Thanks for the response @paulb777, as @mikehardy mentioned it is possible but we'd rather avoid that, I think it would be also reasonable as a workaround to set some compiler flag that will only omit the deallocation part that triggers this crash, and this flag can be used when building the app for Detox tests.

[mikehardy]

@asafkorem is there any reason we can't ride-along with NSZombiesEnabled=1 behavior - that is, make GULObjectSwizzler think we are running the zombies instrument, and thus avoid de-allocation.

Stated differently, perhaps the "NSZombiesEnabled" check could more properly be thought of as a "don't dispose generated classes" check, just with a too-specific name, but it would serve us as well 🤔 . Have you tried it?

[asafkorem]

@mikehardy yes, I thought about it too and already tried this approach.
I wasn't able to set this environment variable using xcodebuild command :/

[asafkorem]

If anyone knows how to set this variable using xcodebuild (NSZombiesEnabled), it will be a nice workaroud for the meanwhile indeed.

[mikehardy]

you have to pass environment variables very very carefully to get them through the simctl layer, examine this - it would probably work? I will try later (but maybe a few days) if you don't

SIMCTL_CHILD_NSZombiesEnabled=1

from

https://github.com/invertase/react-native-firebase/blob/b9af132fd6c8b5fe0b917716f6638906ab2e1157/package.json#L42

This is tested+working way that I inject my AppCheck debug key in (and yes, this a key you'd normally keep private, we accept that it is public in this case as it is on a project configured as a no-cost thing for testing / demonstration purposes only)

[asafkorem]

Oh, this makes more sense now.. thanks @mikehardy. I'll try this soon

[ncooke3]

cc: @visumickey

[visumickey]

Firebase performance relies a lot on Swizzling to measure performance out of the box. We are working on figuring out solutions outside of swizzling (or keep its dependency really low) to measure the performance. This is indeed tricky. So, we are definitely a few months out for a clean solution here. Until then the Zombie flags seems to be an agreeable solution (if that works). We will make sure to explore that option and add that to our FAQs as well.

[mikehardy]

It appears to be working for me, if not completely, then "well". The first run I did I took a SIGSEGV in DetoxSync again, but then I was 10/10 on further runs (an incredible result, I was at maybe 1-2/10 success prior). I am using the special method of passing env vars through simctl as mentioned above. I'm about to make a commit to react-native-firebase that will have a github cross link so others may examine in case it's useful. Thanks for this trick, at least preliminary results show it appears workable to me

[shamilovtim]

I'm slightly concerned about the SIGSEGV. Was that just a freak accident? At least with the swizzling crash there was some transparency into what it was. With SIGSEGVs I dislike that they're usually a disaster to debug

[mikehardy]

I'll need to wait to collect quite a few more results I think, before I want to spend anyone's time in this repo, possibly chasing ghosts.

[mikehardy]

Hmm, sadly I think statistics is a thing and I simply had an improbably long run of good results. With quite a few more runs throughout the day / evening, I still definitely have a problem with Detox-current on firebase-ios-sdk-current on iOS-simulator-current on both local macOS x86-64 VMs and up on github actions macOS runners 🤷 - really seemed like an improvement, and I have a hunch statistically it still might be, but there's no silver bullet with the NSZombiesEnabled=1, at least the way I'm setting it https://github.com/invertase/react-native-firebase/runs/4511769043?check_suite_focus=true 🤷

[shamilovtim]

Yeah can confirm still seeing Signal 11 with SIMCTL_CHILD_NSZombiesEnabled=1. Is there an easy way of confirming that this property is being injected into the simulator build?

[asafkorem]

Thanks @mikehardy! this workaround works for us, NSZombieEnabled=YES after adding this.

@shamilovtim Note that you should use SIMCTL_CHILD_NSZombieEnabled (..Zombie.. and not ..Zombies..).
I checked this on our test-app where I reproduced the issues with firebase-perfomance and it solves the crashes I encountered with.

[mikehardy]

Really sorry I took everyone down the wrong path with my plural-zombies typo! Thanks @asafkorem for seeing that and clearing it up. Excited to have this mitigated, my CI is running now in react-native-firebase based off your PR there Asaf

[shamilovtim]

@asafkorem Nice catch!

[asafkorem]

@paulb777 @ncooke3 @visumickey
Although passing the NSZombieEnabled flag does solves this problem as a workaround, I would have preferred to have a dedicated flag for disabling this class-disposing from firebase-performance and not to ride on the zombie-enabled flag, which might cause other side-effects, like ignoring other memory issues while running detox tests (which might be ignored when zombie-enabled, like this one) or unnecessary memory consumption, especially if a clean solution may only arrive in months. 🙏🏼

[asafkorem]

.. I would have preferred to have a dedicated flag for disabling this class-disposing from firebase-performance and not to ride on the zombie-enabled flag, which might cause other side-effects ..

I've opened a PR for this: google/GoogleUtilities#66

[paulb777]

Thanks @asafkorem! GoogleUtilities 7.7.0 is now published to CocoaPods and SPM with your PR that disables class disposing when the GULGeneratedClassDisposeDisabled environment variable is set.

[asafkorem]

Thanks @paulb777!
We'll replace the zombie flag with this one on Detox tests execution.
FYI @mikehardy.

[mikehardy]

Sample size of 10 runs which - for this bug, in the react-native-firebase harness - is strongly statistically significant already. Looks great, thanks for the ping @asafkorem

[asafkorem]

Although enabling this flag is a bit workaround, it's a pretty decent one. In my opinion, we can definitely live with that, considering the fact that this crash happens when running the apps on Detox tests environment, and this kind of memory consumption (not disposing the generated classes) should not particularly worry us in this case.

I opened this issue in the context of this bug, and I think we can close it now since it was resolved. Thanks guys!

[paulb777]

Thanks for the fixing PR @asafkorem !