🔥 [🐛] RTDB ref().child().off() fails to remove listeners

[laurentS]

trafficstars

Issue

        const a = firebase.database().ref('/v2/projects');
        const b = firebase.database().ref('v2/projects');
        const c = firebase.database().ref().child('/v2/projects');
        const d = firebase.database().ref().child('v2/projects');
        console.log(a.path, b.path, c.path, d.path);
       // outputs v2/projects v2/projects /v2/projects /v2/projects (note the leading slashes)

This leads to at least one problem: if I do:

firebase.database().ref().child('v2/projects').on('value', ...)
firebase.database().ref().child('v2/projects').off() // this should remove any listener on this path

the listener actually remains active after calling off(), and the attached callback is still triggered on changes in said path.

The root cause seems to be in app/lib/common/path.js where:

export function pathChild(path, childPath) {
    const canonicalChildPath = pathPieces(childPath).join('/');
// when called from ref().child(), path === '/' so path.length === 1
// and the return value from the function includes a leading slash
    if (path.length === 0) {
        return canonicalChildPath;
    }
    return `${path}/${canonicalChildPath}`;
}

this in turn means that DatabaseSyncTree.getRegistrationsByPath(this.path) in off() does not match the existing references, and therefore they are not removed.

A possible fix could be as follows in app/lib/common/path.js where:

export function pathChild(path, childPath) {
    const canonicalChildPath = pathPieces(childPath).join('/');
    if (path.length === 0 || path === '/') {  // <-- change here
        return canonicalChildPath;
    }
    return `${path}/${canonicalChildPath}`;
}

Note: this works for my use case, but I have not tested it beyond this (it's way too late now :sweat_smile: ) I've observed the issue on both iOS and android, though I've only tested in detail on android. Given that it's a pure js issue, I don't think the platform matters.

---

Project Files

Javascript

Click To Expand

package.json:

// I think these are the relevant bits
    "@react-native-firebase/app": "^12.3.0",
    "@react-native-firebase/database": "^12.3.0",

firebase.json for react-native-firebase v6:

# N/A

iOS

Click To Expand

ios/Podfile:

• [ ] I'm not using Pods
• [x] I'm using Pods and my Podfile looks like:

# N/A

AppDelegate.m:

// N/A

---

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:

// N/A

android/app/build.gradle:

// N/A

android/settings.gradle:

// N/A

MainApplication.java:

// N/A

AndroidManifest.xml:

<!-- N/A -->

---

Environment

Click To Expand

react-native info output:

System:
    OS: Linux 5.10 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
    CPU: (4) x64 Intel(R) Core(TM) i5-6200U CPU @ 2.30GHz
    Memory: 3.47 GB / 31.26 GB
    Shell: 5.1.8 - /bin/bash
  Binaries:
    Node: 12.16.2 - ~/.nvm/versions/node/v12.16.2/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 6.14.4 - ~/.nvm/versions/node/v12.16.2/bin/npm
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
  IDEs:
    Android Studio: Not Found
  Languages:
    Java: 1.8.0_292 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: ^5.0.0 => 5.0.1
    react: 17.0.1 => 17.0.1
    react-native: 0.64.2 => 0.64.2
  npmGlobalPackages:
    *react-native*: Not Found

• Platform that you're experiencing the issue on:
  • [ ] iOS
  • [ ] Android
  • [ ] iOS but have not tested behavior on Android
  • [ ] Android but have not tested behavior on iOS
  • [x] Both
• react-native-firebase version you're using that has this issue:
  • 12.3.0
• Firebase module(s) you're using that has the issue:
  • RTDB
• Are you using TypeScript?
  • No

---

• 👉 Check out React Native Firebase and Invertase on Twitter for updates on the library.

[mikehardy]

Very interesting! I went to check how this could be slipping through our test harness and, lamentably, found the reason:

https://github.com/invertase/react-native-firebase/blob/0259802768a7d84d7920caa2a53c0ab1c76f8e51/packages/database/e2e/query/off.e2e.js#L1

However, this is a lot of similar testing done directed at the on() call here https://github.com/invertase/react-native-firebase/blob/master/packages/database/e2e/query/on.e2e.js - and with a copy/paste and a couple quick edits there could be a couple tests that focus on .off() that should expose this issue clearly, and then validate the fix

If you haven't set up the tests structure here before it's actually not that bad - and then any PR will generate a patch-set plus be fully validated + easy to merge + release https://github.com/invertase/react-native-firebase/blob/master/tests/README.md

[stale[bot]]

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.

[laurentS]

I still mean to fix this, just need to make time for it.

[github-actions[bot]]

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 attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.