invertase / react-native-firebase

🔥 A well-tested feature-rich modular Firebase implementation for React Native. Supports both iOS & Android platforms for all Firebase services.
https://rnfirebase.io
Other
11.64k stars 2.21k forks source link

🔥Can't modify config objects after they are in use for FIRDatabaseReferences #2838

Closed kevinEsherick closed 3 years ago

kevinEsherick commented 4 years ago

Issue

I'm getting the error displayed in title sometimes after upgrading to v6. Getting it seemingly randomly, unable to intentionally reproduce. Below are screenshots of the error and callstack. They both appear to have some information that might be helpful.

IMG_5323 IMG_5325


Project Files

iOS

Click To Expand

#### `ios/Podfile`: - [ ] I'm not using Pods - [x] I'm using Pods and my Podfile looks like: ```ruby # Uncomment the next line to define a global platform for your project platform :ios, '9.0' target 'App' do # Uncomment the next line if you're using Swift or would like to use dynamic frameworks # use_frameworks! # Pods for App pod 'React', :path => '../node_modules/react-native' pod 'yoga', :path => '../node_modules/react-native/ReactCommon/yoga' pod 'RNFBApp', :path => '../node_modules/@react-native-firebase/app' pod 'RNFBAnalytics', :path => '../node_modules/@react-native-firebase/analytics' pod 'RNFBAuth', :path => '../node_modules/@react-native-firebase/auth' pod 'RNFBDatabase', :path => '../node_modules/@react-native-firebase/database' pod 'RNFBRemoteConfig', :path => '../node_modules/@react-native-firebase/remote-config' pod 'RNFBCrashlytics', :path => '../node_modules/@react-native-firebase/crashlytics' pod 'Fabric', '~> 1.9.0' pod 'Crashlytics', '~> 3.12.0' pod 'Folly', :podspec => "../node_modules/react-native/third-party-podspecs/Folly.podspec" target 'AppTests' do inherit! :search_paths # Pods for testing end post_install do |installer| installer.pods_project.targets.each do |target| if target.name == "React" target.remove_from_project end end end end ``` #### `AppDelegate.m`: ```objc #import "AppDelegate.h" #import #import #import #import @implementation AppDelegate - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions { [FIRApp configure]; NSURL *jsCodeLocation; jsCodeLocation = [[RCTBundleURLProvider sharedSettings] jsBundleURLForBundleRoot:@"index" fallbackResource:nil]; UIView *backgroundView = [[[NSBundle mainBundle] loadNibNamed:@"LaunchScreen" owner:self options:nil] firstObject]; RCTRootView *rootView = [[RCTRootView alloc] initWithBundleURL:jsCodeLocation moduleName:@"App" initialProperties:nil launchOptions:launchOptions]; rootView.backgroundColor = [UIColor clearColor]; self.window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds]; UIViewController *rootViewController = [UIViewController new]; rootViewController.view = backgroundView; self.window.rootViewController = rootViewController; [self.window makeKeyAndVisible]; [backgroundView addSubview:rootView]; rootView.frame = backgroundView.frame; return YES; } // Required to register for notifications - (void)application:(UIApplication *)application didRegisterUserNotificationSettings:(UIUserNotificationSettings *)notificationSettings { [RCTPushNotificationManager didRegisterUserNotificationSettings:notificationSettings]; } // Required for the register event. - (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken { [RCTPushNotificationManager didRegisterForRemoteNotificationsWithDeviceToken:deviceToken]; } // Required for the notification event. You must call the completion handler after handling the remote notification. - (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)userInfo fetchCompletionHandler:(void (^)(UIBackgroundFetchResult))completionHandler { [RCTPushNotificationManager didReceiveRemoteNotification:userInfo fetchCompletionHandler:completionHandler]; } // Required for the registrationError event. - (void)application:(UIApplication *)application didFailToRegisterForRemoteNotificationsWithError:(NSError *)error { [RCTPushNotificationManager didFailToRegisterForRemoteNotificationsWithError:error]; } // Required for the localNotification event. - (void)application:(UIApplication *)application didReceiveLocalNotification:(UILocalNotification *)notification { [RCTPushNotificationManager didReceiveLocalNotification:notification]; } @end ```


Android

Click To Expand

#### Have you converted to AndroidX? - [ ] my application is an AndroidX application? - [ ] I am using `android/gradle.settings` `jetifier=true` for Android compatibility? - [ ] I am using the NPM package `jetifier` for react-native compatibility? #### `android/build.gradle`: ```groovy // N/A ``` #### `android/app/build.gradle`: ```groovy // N/A ``` #### `android/settings.gradle`: ```groovy // N/A ``` #### `MainApplication.java`: ```java // N/A ``` #### `AndroidManifest.xml`: ```xml ```


Environment

Click To Expand

**`react-native info` output:** ``` React Native Environment Info: System: OS: macOS High Sierra 10.13.6 CPU: (4) x64 Intel(R) Core(TM) i5-2435M CPU @ 2.40GHz Memory: 1002.86 MB / 8.00 GB Shell: 3.2.57 - /bin/bash Binaries: Node: 8.7.0 - /usr/local/bin/node Yarn: 1.2.1 - /usr/local/bin/yarn npm: 5.5.1 - /usr/local/bin/npm SDKs: iOS SDK: Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1 IDEs: Android Studio: 3.4 AI-183.6156.11.34.5692245 Xcode: 10.1/10B61 - /usr/bin/xcodebuild npmPackages: react: 16.8.3 => 16.8.3 react-native: 0.59.3 => 0.59.3 npmGlobalPackages: create-react-native-app: 1.0.0 react-native-app-id-fixed: 0.2.1 react-native-cli: 2.0.1 ``` - **Platform that you're experiencing the issue on**: - [ ] iOS - [ ] Android - [x] **iOS** but have not tested behavior on Android - [ ] **Android** but have not tested behavior on iOS - [ ] Both - **`react-native-firebase` version you're using that has this issue:** - `6.0.3` - **`Firebase` module(s) you're using that has the issue:** - `Realtime Database, maybe RemoteConfig` - **Are you using `TypeScript`?** - `N`


Think react-native-firebase is great? Please consider supporting all of the project maintainers and contributors by donating via our Open Collective where all contributors can submit expenses. [Learn More]

mikehardy commented 4 years ago

You have probably already looked, but it seems like some chunk of code somewhere is toggling database persistence enabled. And it's doing that after the database is in use?

https://firebase.google.com/docs/reference/ios/firebasedatabase/api/reference/Classes/FIRDatabase#persistenceenabled

"Note that this method must be called before creating your first Database reference and only needs to be called once per application"

so, probably some sort of order of operations problem / race / something in your app bootstrap?

kevinEsherick commented 4 years ago

Thanks for the help. Just spent a lot of time looking into this. I think it's a bug or documentation error in react-native-firebase but I could be wrong. It seems getDatabaseForApp calls setDatabaseConfig which calls setPersistenceEnabled. getDatabaseForApp is called in just about every database method and references the database prior to calling setDatabaseConfig, which will lead setPersistenceEnabled to be called. The only other means through which setPersistenceEnabled in invoked afaik is by directly calling firebase.database().setPersistenceEnabled(). I never call this. So maybe setDatabaseConfig needs to be called before the database is referenced in getDatabaseForApp? Or otherwise setPersistenceEnabled needs to be invoked in app setup for all users?

mikehardy commented 4 years ago

as it is optional I don't think it is necessary to actually call it ever, it should only be called by the uesr or (if react-native-firebase allows you to set it from firebase.json) immediately at startup. You can always try just commenting it out in node_modules and using patch-package to make that persistent / shareable with your team + CI and see how it goes, would be the basis for a PR

kevinEsherick commented 4 years ago

Commenting out these lines in RNFBDatabaseCommon.m and will report back how it goes.

BOOL *persistenceEnabled = (BOOL *) [preferences getBooleanValue:DATABASE_PERSISTENCE_ENABLED defaultValue:false]; [firDatabase setPersistenceEnabled:(BOOL) persistenceEnabled];

Like I said I haven't been able to repro, but it's happened frequently enough that I should be able to tell if this is a fix. Hopefully there are no side effects. In any case I think some sort of fix is basis for a PR.

EDIT: Commenting out those lines got rid of that error but has exposed another one. #2734 is now happening. The fix showed there won't work for me because it's for v5 whereas I'm on v6. If you know what the v6 equivalent of that is (if there is one) that would be really helpful. I'll be looking for it in the meantime and will report back as soon as I've found a fix. I've successfully been able to get it to reproduce—it happens on the first call to database after a fresh install of the app. I think this all ties in with database methods being called early in the app lifecycle, since this is the case in my app and has been suggested by others with similar issues. This design is important enough to my app that I'll continue looking for fixes on the firebase end for now. But this should be documented or fixed because afaik there's nothing to indicate that this is bad practice.

mikehardy commented 4 years ago

Hey @kevinEsherick my github workflow relies on new notifications so if you update a comment vs making a new comment I won't see it :-), I only see this by luck really.

It appeared to me that my comment on the new issue for you is definitely in v6, but also the exact same change (just in a different file) could be applied trivially. https://github.com/invertase/react-native-firebase/blob/master/packages/database/ios/RNFBDatabase/RNFBDatabaseTransactionModule.m#L68

The only difference I saw was the addition of the pragma lines but you'll notice they push and pop inside the move, so they shouldn't affect whether the patch works or not, if it is the right solution.

As far as patches go this one should be trivial to try anyway.

kevinEsherick commented 4 years ago

Oh thanks, good to know! I was talking about this fix: https://github.com/invertase/react-native-firebase/pull/2734/commits/84794f91635cf59bf4e1de11fa14684e424dcc00 which the OP on that issue reported as a fix, but it involves some code that is not found, at least in the exact same form, in v6. So I'm looking for the v6 equivalent of this. Got sidetracked last night and am back to working on it now. If you know what the v6 equivalent of the above fix would be, please let me know. I'm guessing you thought I was talking about your comment which links to this: https://github.com/invertase/react-native-firebase/blob/master/packages/database/ios/RNFBDatabase/RNFBDatabaseTransactionModule.m#L68 but I'm not sure where in that file you're pointing to a fix—that file seems to be identical to what I'm currently working with. If there's a fix therein, please let me know. Thanks!

kevinEsherick commented 4 years ago

Ah okay so if I'm not mistaken the link you posted on that issue is the v6 equivalent of the v5 fix that OP posted. So was the fix you posted supposed to be adding this line: FIRDatabaseReference *firDatabaseReference = [RNFBDatabaseCommon getReferenceForDatabase:firDatabase path:path]; or deleting it or am I missing something? My RNFBDatabaseTransactionModule.m is presently identical to the one you posted there, so if the patch was included there it doesn't seem to be working.

mikehardy commented 4 years ago

If you follow the PR, you'll see it moves one line of code from it's original location to a new location up about 4 lines in a larger scope

The PR was against v5, but the code I linked for v6 is almost identical, and so the v5 PR change (moving the line up) should apply to v6 as well, if someone tried it. You can be the person that tries it :-)

mikehardy commented 4 years ago

when I say "should apply" I mean, "you can edit that file and manually implement the same change", not anything fancy like "git can apply that patch" or something

kevinEsherick commented 4 years ago

Tried simply moving lines up above dispatch_async to no avail. You have to move both 67 and 68 bc firDatabase is declared in 67 then used in 68. Still got the error. Will keep toying around and report back.

kevinEsherick commented 4 years ago

For now I've done this in FRepoManager.m: if (repo == nil) { repo = [[FRepo alloc] initWithRepoInfo:repoInfo config:config database:database]; repos[repoInfo] = repo; return repo; } else { //[NSException // raise:@"RepoExists" // format:@"createRepo called for Repo that already exists."]; return repo; //nil; } I commented out the exception throw and just return the repo that exists. Since it seems to just be a race condition causing an exception to be thrown when createRepo is called one already exists, I see no problem in just returning the existing repo rather than throwing an exception, since it's the same repo as the one that the second call was attempting to create. Everything still works fine, and the exception isn't thrown of course. Does this seem safe to you?

mikehardy commented 4 years ago

your reasoning seems sound but I don't have a lot of experience with this side of the code (ios) or this module so don't trust me farther than "at least another set of eyes saw and didn't say it was obviously wrong" :-). Hopefully that's worth something

kevinEsherick commented 4 years ago

Hahaha thank you. Hesitant to close because I think there should be some sort of handling of this error on the firebase side, whether by finding a way to better handle createRepo being called when one is in existence or by documenting how to avoid causing this to happen. But this is a bit out of my depth so lmk your thoughts

dakkafex commented 4 years ago

Getting this issue too, what exactly is the current fix/hack?

mikehardy commented 4 years ago

looks like from here, this comment https://github.com/invertase/react-native-firebase/issues/2838#issuecomment-552010519 - some confirmation it works for you would be useful if you try it

kevinEsherick commented 4 years ago

It's not a fix, just prevents the app from crashing. The database write will not take place, so I'm still trying to figure out what's causing the issue in the first place. @dakkafex can you give a little more detail on when it's happening for you?

dakkafex commented 4 years ago

Im using the DB in a helper function to update my users location (based on a trigger in the component itself)

It pretty much happens the moment either of the functions is being called, even if i comment one of them out to restrict calling it more then once.

```TSX setLocation: function(geoLocation) : void { let uid = auth().currentUser.uid; let grid = Geohash.encode(geoLocation.latitude, geoLocation.longitude, 7); let dbRef = database().ref(`locationsX/${grid}/${uid}`); dbRef.set({ geo: new firestore.GeoPoint(geoLocation.latitude, geoLocation.longitude), time: Date.now(), }) .catch(e => console.log(e)) }, createMatches: function(geoLocation) : void { let uid: string = auth().currentUser.uid; let grid: string = Geohash.encode(geoLocation.latitude, geoLocation.longitude, 7); let neighbours = {...Geohash.neighbours(grid), c: grid}; let locations = []; for (const n in neighbours) { database().ref(`locationsX/${neighbours[n]}`) .once('value') .then(docs => { docs.forEach((doc) =>{ if (doc.key !== uid) { try { locations.push({ latitude: doc.val().geo._latitude, longitude: doc.val().geo._longitude, time: doc.val().time, profile: doc.key }) } catch (error) { console.log(error); } } }) }) .then(() => { locations.forEach(loc => { let distanceBewteen = Tdistance([loc.latitude, loc.longitude], [geoLocation.latitude, geoLocation.longitude]); let lessThenMin = moment().diff(loc.time, 'm') if (distanceBewteen <= 0.2 && lessThenMin <= 5) { firestore() .collection(`users/${uid}/match`) .doc(loc.profile) .set({ ID: loc.profile, L: new firestore.GeoPoint(loc.latitude, loc.longitude), T: firestore.Timestamp.fromMillis(loc.time), }) } }) }) }; } ``` excuse the poor use of TS, im just getting into using it, figure i try just making helper functions with it.
luskin commented 4 years ago

We just started running into this issue today after running a pod install on iOS and seeing either a react-native-firebase or just a plain firebase update installed. I'll dig into this and report back as soon as I can.

dakkafex commented 4 years ago

does 5.x.x have this issue too?

ghsdh3409 commented 4 years ago

Same here!

luskin commented 4 years ago

All I can report is that this error is quite inconsistent. What we have noticed is that eventually the crash goes away if you:

1) Archive and distribute release build to test flight 2) Install app from test flight 3) Open app - CRASH 4) Open app - CRASH 5) Open app - Works...

I don't have the time right now to dig into why this is the case but it's extremely odd behavior. Any insight from the maintainers please? This is holding up the release of our new app!

mikehardy commented 4 years ago

There was a reported possible workaround via the Discord support chat server yesterday, I quote:

i have forced setPersistenceEnabled(false); to false in App.js ... it seems to
have fixed the issue? (at least its not crashing anymore)

Subject to testing by you guys and reporting success these are my thoughts:

I believe it is supposed to be false by default in the underlying SDK, per my understanding of the docs, and unless you want to change it I do not believe anything should be done in native code

I do not know what is happening in the react-native-firebase module bootstrap that is attempting to toggle it on, either at the right time or otherwise, but it appears something is happening and perhaps forcing it to false early in your project bootstrap works like the reported success above?

I do not think turning persistenceEnabled to true will be successful right now but I think it might be unrelated (except in so far as it's obviously related to persistence being set or not)

luskin commented 4 years ago

For anyone else with this issue, we have downgraded to 0.60.6 and it SEEMS to be working - I'll keep you all posted. 0.61 seems to have known issues with react-native-firebase as well as react-native-code-push so until those get ironed out I would recommend downgrading like we did.

UPDATE: @mikehardy The issue re-appeared on 0.60.6. Turns out it's only an issue (at least for us) when building release. No issue in debug.

dakkafex commented 4 years ago

There was a reported possible workaround via the Discord support chat server yesterday, I quote:

i have forced setPersistenceEnabled(false); to false in App.js ... it seems to
have fixed the issue? (at least its not crashing anymore)

Subject to testing by you guys and reporting success these are my thoughts:

I believe it is supposed to be false by default in the underlying SDK, per my understanding of the docs, and unless you want to change it I do not believe anything should be done in native code

I do not know what is happening in the react-native-firebase module bootstrap that is attempting to toggle it on, either at the right time or otherwise, but it appears something is happening and perhaps forcing it to false early in your project bootstrap works like the reported success above?

I do not think turning persistenceEnabled to true will be successful right now but I think it might be unrelated (except in so far as it's obviously related to persistence being set or not)

So this did work, in debug, in release im getting the error again

luskin commented 4 years ago

Alright so we stumbled upon something interesting, if you build a release configuration via command line instead of within Xcode this issue does not occur. I have no idea why this would be the case but for us we have gotten by using this approach.

mikehardy commented 4 years ago

That is most unexpected. I always use fastlane which goes from the command line, and I use a specific set of command line options with npx react-native-clean-project in order to clean DerivedData each time prior. I have problems with reproducibility and differences between Xcode and command line myself otherwise. You might try react-native-clean-project with --clean-ios-build (IIRC) and see if that helps?

Salakar commented 4 years ago

Has anyone got a code block they can give me that reproduces this? I can take a look at debugging it then, thanks

damandeepsingh93 commented 4 years ago

Even I am getting this, but its very random. I can see it in bugnsag reports. We have encountered only 10 crashes against 1000 launches. Our project uses fastlane to build the releases.

cmcleese commented 4 years ago

Still happening for me

luskin commented 4 years ago

Same as @damandeepsingh93 - I'm seeing it randomly, about 7% of users. We have noticed that it happens primarily while the app is backgrounded

zhigang1992 commented 4 years ago

@Salakar I was able consistently reproducing the crash (on app launch, and not on reloads) with the code on the left

And the code on the right fixed it

Screen Shot 2020-01-09 at 11 06 17 AM

I think it might be a race condition of some sort.

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

damandeepsingh93 commented 4 years ago

Please fix this, I am still seeing random crashes in bugsnag

zhigang1992 commented 4 years ago

https://stackoverflow.com/questions/33787615/firebase-ios-sdk-async-and-threads/33793174#33793174

I was able to fix it by switching to the main thread

fungilation commented 4 years ago

I'm hitting this in Sentry crash logs too, and it's random also on a small fraction of total launches.

https://sentry.io/share/issue/89492a04f61840d5add8428657732ba3/

I run these in app root index.js:

firebase.database().setPersistenceEnabled(true)
firebase.database().setPersistenceCacheSizeBytes(50000000)
stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

sndp07 commented 4 years ago

https://stackoverflow.com/questions/33787615/firebase-ios-sdk-async-and-threads/33793174#33793174

I was able to fix it by switching to the main thread

@zhigang1992 May be a silly question, but could you add little details on what you did for this(switching to the main thread) ?

zhigang1992 commented 4 years ago

https://github.com/tappollo/booster/blob/6d4c5f9e27519bb2347f4d09e106170351b450b7/app/patches/%40react-native-firebase%2Bdatabase%2B6.7.1.patch

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 years ago

Closing this issue after a prolonged period of inactivity. If this is still present in the latest release, please feel free to create a new issue with up-to-date information.

iiAtlas commented 4 years ago

I see this was closed but just wanted to pass along that I am having my app crash on it to. I am using app:7.2.1 and database:7.2.2

Happy to debug further but seems you all have a good grip on this. Wanted to just pass along a +1!

NickLewry commented 4 years ago

@mikehardy (apologies for the mention just wasn't sure if this thread was still monitored) I'm getting this issue too, it does not happen in Debug only on the first load of an app built in release mode on a new device.

"@react-native-firebase/app@7.2.1" "@react-native-firebase/database@7.2.2"

iOS 13.4

error message: Can't modify config objects after they are in use for FIRDatabaseReferences. -[FIRDatabaseConfig setPersistenceEnabled:]

source of error: -[FIRDatabaseConfig setPersistenceEnabled:] + 65 (FIRDatabaseConfig.m:65)

I've tried adding setPersistenceEnabled(false) in the earliest stage of my project but it still crashes.

iiAtlas commented 4 years ago

@NickLewry likewise — I noticed this on my release build only! Distributed via TestFlight.

NickLewry commented 4 years ago

@iiAtlas so after doing more digging turns out I had a race condition where a UI component was trying to make a request before firebase had initialised fully.

I popped this quick code block before the first call my application makes to firebase which is how I know what the issue is:

await new Promise((res) => setTimeout(res, 2000));

Hope you manage to find out what your issue is and hopefully this was helpful!

Just been messing around again and I think i have a clearer reason. If the user loading the app was a new user I would then create a new user record using set and then a couple lines down I'm creating a listener on that record the listen for any changes.

I've fixed this issue by moving the create user (set method) to below the listener creation, I'm not entirely sure why this fixes it so if anyone has any ideas that would be great?

iiAtlas commented 4 years ago

@NickLewry thanks for the tip, I do have a component where I update the users attached profile -- I'll start there!

dannyyaou commented 4 years ago

I encountered this as well. "@react-native-firebase/database": "^7.2.2", only on production...

luskin commented 4 years ago

Any tips here from the maintainers? We see thousands of these crashes a day, usually while the app is in the background

mikehardy commented 4 years ago

sorry my project is firestore only, so I have no direct experience - you might make completely sure you're using the most up to date versions and SDK just to make sure further attention is based on current code / not already fixed

luskin commented 4 years ago

@mikehardy we just entirely rebuilt our app with the latest react native and latest RNFirebase. We are still seeing this crash occasionally on some devices: SIGABRT: Can't modify config objects after they are in use for FIRDatabaseReferences.

Again, this was not an update, this was an entire fresh rebuild of our app using the latest react native/rn firebase templates & installation procedures.

...
    "@react-native-firebase/app": "^8.3.0",
    "@react-native-firebase/auth": "^8.3.1",
    "@react-native-firebase/crashlytics": "^8.3.0",
    "@react-native-firebase/database": "^7.4.1",
    "@react-native-firebase/dynamic-links": "^7.4.1",
    "@react-native-firebase/messaging": "^7.6.1",
    "@react-native-firebase/perf": "^7.3.1",
    "@react-native-firebase/remote-config": "^8.1.1",
...
    "react": "16.13.1",
    "react-native": "0.63.0",
mikehardy commented 4 years ago

@luskin well that's pretty clean as an attempt, thanks very much for the feedback. All I can think of is instrumenting it more to see who's touching the config objects, perhaps a boolean gate that is toggled on after any of the methods are called that would trigger the error (anything but config methods I think?) and that is checked on any of the config methods, logging a trace if that happens vs a crash

Sort of a diagnostic PR although if there wasn't a performance hit to it, handling these cases gracefully permanently might not be a bad idea