fix(crashlytics,ios): Explicitly set crash collection setting to opt in or out for iOS #4236

williamhaley · 2020-09-10T02:46:43Z

Description

PR spawned from discussion here #4227

By my reckoning, I think there may be some behavior that is maaaaybe not working as expected, and I also think there is some behavior that is definitely not working as I'd expect it.

Sorry if this is verbose, but some of this is very new for me so I wanted to be explicit. Also, apologies if I'm way off base on any of this

1. `FirebaseCrashlyticsCollectionEnabled` gets turned off by default by react-native-firebase

By default, Crashlytics automatically collects crash reports for all your app's users. [1]

App developers using Crashlytics can go out of their way to set FirebaseCrashlyticsCollectionEnabled to false to turn off automatic crash collection in their apps. Why would a dev do that? Maybe they want to give users control over their data. They can make an "Opt-in" view in the app that allows users to opt-in to uploading crash reports.

react-native-firebase does the opposite. It disables collecting crash reports by default.

Note here, the default for react-native-firebase is to always set FirebaseCrashlyticsCollectionEnabled to No/false unless _CRASHLYTICS_AUTO_DISABLE_ENABLED is set to true, and _CRASHLYTICS_AUTO_DISABLE_ENABLED is derived from crashlytics_disable_auto_disabler. [2]

  if [[ $_CRASHLYTICS_AUTO_DISABLE_ENABLED == "true" ]]; then
    echo "Disabled Crashlytics auto disabler." # do nothing
  else
    _PLIST_ENTRY_KEYS+=("FirebaseCrashlyticsCollectionEnabled")
    _PLIST_ENTRY_TYPES+=("bool")
    _PLIST_ENTRY_VALUES+=("NO")
  fi

If react-native-firebase devs don't set crashlytics_disable_auto_disabler to true in firebase.json, then the FirebaseCrashlyticsCollectionEnabled automatically gets set to false!

Unexpected, but also, not actually a problem because it's still entirely possible for the app to send crash reports. It's just not automatic by default.

2. On iOS react-native-firebase apps never actually set the collection setting on the Crashlytics instance

I think this is actually a bug/missing bit of behavior.

react-native-firebase turns off automatic crash collection by default (see above), which is fine actually. it's fine because react-native-firebase can still explicitly enable collecting crash reports by calling setCrashlyticsCollectionEnabled:true. Calling that essentially overrides whatever was done with FirebaseCrashlyticsCollectionEnabled.

We call setCrashlyticsCollectionEnabled on the Java side, but not on the iOS side.

On both the iOS and Java side, we have isCrashlyticsCollectionEnabled(). We use that method to infer if the app wants to enable or disable crashlytics using one of several mechanisms (including the crashlytics_auto_collection_enabled key in firebase.json). Most importantly, we use the result of that to actually set the setting on our crashlytics instance in Java.

react-native-firebase/packages/crashlytics/android/src/main/java/io/invertase/firebase/crashlytics/ReactNativeFirebaseCrashlyticsInitProvider.java

Line 54 in 55cd752

    
           FirebaseCrashlytics.getInstance().setCrashlyticsCollectionEnabled(ReactNativeFirebaseCrashlyticsInitProvider.isCrashlyticsCollectionEnabled());

iOS has a mirror version of the isCrashlyticsCollectionEnabled() method. However, on the iOS side we never actually use the result of that method to set the setting on our Crashlytics instance.

react-native-firebase/packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsInitProvider.m

Line 54 in 55cd752

+ (void)configureWithApp:(FIRApp *)app {

I verified that, by default, isCrashlyticsCollectionEnabled will return true if the developer does nothing. But because iOS never sets the setting, we don't opt-in to collecting crashes!

I feel like this is a bug, and we should use the change in this PR to do the same thing on iOS as in Java.

Related issues

Fixes discussion #4227

Release Summary

Update crash collection behavior on iOS

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
- Yes
My change supports the following platforms;
- Android
- iOS
My change includes tests;
- e2e tests added or updated in packages/\*\*/e2e
- jest tests added or updated in packages/\*\*/__tests__
I have updated TypeScript types that are affected by my change.
This is a breaking change;
- Yes
- No

Test Plan

For the first issue I mentioned, build an app with react-native-firebase. Check out the builds logs in the report navigator in Xcode (filter on [RNFB] Core Configuration and expand all transcripts). Notice that the default behavior (with an empty {} firebase.json`) is to disable automatic collection of crashes.

info: -> RNFB build script started
info: 1) Locating firebase.json file:
info:      (1 of 2) Searching in '/Users/will/dev/THEAPP' for a firebase.json file.
info:      firebase.json found at /Users/will/dev/THEAPP/ios/firebase.json
info: 2) Injecting Info.plist entries: 
    ->  0) firebase_json_raw string e30=
    ->  1) FirebaseCrashlyticsCollectionEnabled bool NO
info:      setting plist entry 'firebase_json_raw' of type 'string' in file '/Users/will/Library/Developer/Xcode/DerivedData/THEAPP-domvlkryyhzgidhjsghaoaweccvo/Build/Products/Debug-iphoneos/THEAPP.app/Info.plist'
info:      setting plist entry 'FirebaseCrashlyticsCollectionEnabled' of type 'bool' in file '/Users/will/Library/Developer/Xcode/DerivedData/THEAPP-domvlkryyhzgidhjsghaoaweccvo/Build/Products/Debug-iphoneos/THEAPP.app/Info.plist'
info:      setting plist entry 'firebase_json_raw' of type 'string' in file '/Users/will/Library/Developer/Xcode/DerivedData/THEAPP-domvlkryyhzgidhjsghaoaweccvo/Build/Products/Debug-iphoneos/THEAPP.app.dSYM/Contents/Info.plist'
info:      setting plist entry 'FirebaseCrashlyticsCollectionEnabled' of type 'bool' in file '/Users/will/Library/Developer/Xcode/DerivedData/THEAPP-domvlkryyhzgidhjsghaoaweccvo/Build/Products/Debug-iphoneos/THEAPP.app.dSYM/Contents/Info.plist'
info: <- RNFB build script finished

For the second issue, enable -FIRDebugEnabled for the build to see Crashlytics logs.

Add something like this to your AppDelegate.m. Before my changes, you should see that setting crashlytics_auto_collection_enabled to true or false in firebase.json doesn't actually change the result here. With my changes, you should see that the firebase.json crashlytics_auto_collection_enabled setting actually impacts whether or not Crashlytics collects crash reports.

  [FIRApp configure];

  NSLog(@"Is collection enabled? %d", [FIRCrashlytics crashlytics].isCrashlyticsCollectionEnabled);

On my side, I couldn't get the crashlytics JS libraries to resolve properly from my in-development ../react-native-firebase package (I don't contribute to packages often), so I just threw errors in some Swift bridged code I had. I built on the simulator, killed the app to detach the debugger, then launched it and ran code that triggered a bunch of fatalError() in the native code. crashlytics_auto_collection_enabled was false, then after several crashes I switched it to true, re-launched, and saw my exceptions show up in Crashlytics.

Something that kinda sucks is the log looks a little confusing. The AppDelegate check is accurate, but since configure was already called beforehand, it has an earlier log message saying, Automatic data collection is disabled., which is technically accurate, but then immediately negated. Subsequent launch debug logs would look more consistent.

2020-09-09 21:06:54.477375-0500 THEAPP[33868:5230341] 6.30.0 - [Firebase/Crashlytics][I-CLS000000] Automatic data collection is disabled.
2020-09-09 21:06:54.481802-0500 THEAPP[33868:5230341] 6.30.0 - [GULReachability][I-REA902003] Monitoring the network status
2020-09-09 21:06:54.481848-0500 THEAPP[33868:5230206] Is collection enabled? 1
2020-09-09 21:06:54.482806-0500 THEAPP[33868:5230341] 6.30.0 - [Firebase/Crashlytics][I-CLS000000] [Crashlytics:Crash] 8 unsent reports are available. Checking for upload permission.
2020-09-09 21:06:54.482924-0500 THEAPP[33868:5230341] 6.30.0 - [Firebase/Crashlytics][I-CLS000000] [Crashlytics:Crash] Notifying that unsent reports are available.
2020-09-09 21:06:54.483255-0500 THEAPP[33868:5230341] 6.30.0 - [Firebase/Crashlytics][I-CLS000000] [Crashlytics:Crash] Waiting for send/deleteUnsentReports to be called.

Without my changes, I could only change enable crash collection with [[FIRCrashlytics crashlytics] setCrashlyticsCollectionEnabled:false]; (or true) in native code. Changing firebase.json and re-building didn't work.

{
  "react-native": {
    "crashlytics_auto_collection_enabled": true
  }
}

Searching for unsent in the logs allowed me to see that reports were collecting on device and that I could then try changing the collection status to test changes.

5 unsent reports are available. Checking for upload permission.

Important testing note! Please keep in mind that setCrashlyticsCollectionEnabled: writes changes to disk. it is not ephermeral. If that method is called, the setting used will stick until either the app is deleted or the setting is explicitly changed.

🔥

CLAassistant · 2020-09-10T02:46:48Z

All committers have signed the CLA.

vercel · 2020-09-10T02:46:53Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/ezdtvl94z
✅ Preview: https://react-native-firebase-git-fork-williamhaley-master.invertase.vercel.app

PR spawned from discussion here #4227

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsInitProvider.m

mikehardy · 2020-09-10T03:17:22Z

I read the text but haven't followed the links and checked the code yet, if it matches the text and I see what I'm expecting to see though, this could be what we've needed for a while! Either way I'm extremely thankful you've gotten the ball rolling here with a real PR for it and I'll look at it more tomorrow morning

mikehardy · 2020-09-10T16:47:51Z

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsInitProvider.m

-  if ([self isCrashlyticsCollectionEnabled]) {
-    [FIRApp registerInternalLibrary:self withName:@"react-native-firebase-crashlytics" withVersion:@"6.0.0"];
-  }
+  [FIRApp registerInternalLibrary:self withName:@"react-native-firebase-crashlytics" withVersion:@"6.0.0"];


There is a separate issue as well - the version number should be dynamic and track with the package but it is not. That has nothing to do with this PR though, I will peel that out to a separate issue

mikehardy · 2020-09-10T17:17:52Z

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsInitProvider.m

+ * At this point "configure" has already been called on the FIRApp instance. This behavior is meant to mirror
+ * what is done in ReactNativeFirebaseCrashlyticsInitProvider.java
+ *
+ * This pattern may not be supported long term https://github.com/firebase/firebase-ios-sdk/issues/2933


Very interesting callout here! I just scanned that issue and the resulting PRs

It looks like dynamic links has a very visible example of what the change means - that configureWithApp should go away, and that 'componentsToRegister' becomes the new natural home for the logic, with a new flag to initialize quickly

https://github.com/firebase/firebase-ios-sdk/pull/4137/files/71f589926b74e69ea76d0a25ae24e58efee6836b#diff-61367186bfcedbaaa5b0554cd03f8821R126-R162

What I don't know though is where to put that flag that says we want init eagerly (for the default app since crashlytics does not do multi-app) - e.g. this https://github.com/firebase/firebase-ios-sdk/pull/4137/files/71f589926b74e69ea76d0a25ae24e58efee6836b#diff-f33c561b2a3430b947e6da076758c6d9R1887

I think this might be good enough (it will certainly work now) but we need a separate issue to track this as they will appear set to purge it for v7

mikehardy

This stuff is very hard to test! I can see the expected log out put though and I believe this is all correct. Will be good to get more feedback from other affected people

…e#4236) * Explicitly set crash collection setting PR spawned from discussion here invertase#4227

vercel bot temporarily deployed to Preview September 10, 2020 02:46 Inactive

Explicitly set crash collection setting

93f6bdc

PR spawned from discussion here #4227

vercel bot temporarily deployed to Preview September 10, 2020 02:47 Inactive

williamhaley commented Sep 10, 2020

View reviewed changes

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsInitProvider.m Show resolved Hide resolved

williamhaley commented Sep 10, 2020

View reviewed changes

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsInitProvider.m Show resolved Hide resolved

Revert change

ec12548

vercel bot deployed to Preview September 10, 2020 02:51 View deployment

mikehardy reviewed Sep 10, 2020

View reviewed changes

mikehardy mentioned this pull request Sep 10, 2020

Crashlytics library registration version number should be dynamic #4240

Closed

mikehardy reviewed Sep 10, 2020

View reviewed changes

mikehardy mentioned this pull request Sep 10, 2020

Remove use of configureWithApp from iOS init sequence #4241

Closed

mikehardy approved these changes Sep 10, 2020

View reviewed changes

mikehardy merged commit cda4c10 into invertase:master Sep 10, 2020

mikehardy mentioned this pull request Oct 3, 2020

🔥 Crashlytics reports error even if crashlytics_debug_enabled is false #4348

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(crashlytics,ios): Explicitly set crash collection setting to opt in or out for iOS #4236

fix(crashlytics,ios): Explicitly set crash collection setting to opt in or out for iOS #4236

williamhaley commented Sep 10, 2020

CLAassistant commented Sep 10, 2020 •

edited

Loading

vercel bot commented Sep 10, 2020 •

edited

Loading

mikehardy commented Sep 10, 2020

mikehardy Sep 10, 2020

mikehardy Sep 10, 2020

mikehardy left a comment

fix(crashlytics,ios): Explicitly set crash collection setting to opt in or out for iOS #4236

fix(crashlytics,ios): Explicitly set crash collection setting to opt in or out for iOS #4236

Conversation

williamhaley commented Sep 10, 2020

Description

1. FirebaseCrashlyticsCollectionEnabled gets turned off by default by react-native-firebase

2. On iOS react-native-firebase apps never actually set the collection setting on the Crashlytics instance

Related issues

Release Summary

Checklist

Test Plan

CLAassistant commented Sep 10, 2020 • edited Loading

vercel bot commented Sep 10, 2020 • edited Loading

mikehardy commented Sep 10, 2020

mikehardy Sep 10, 2020

Choose a reason for hiding this comment

mikehardy Sep 10, 2020

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

1. `FirebaseCrashlyticsCollectionEnabled` gets turned off by default by react-native-firebase

CLAassistant commented Sep 10, 2020 •

edited

Loading

vercel bot commented Sep 10, 2020 •

edited

Loading