Firebase Auth's onAuthStateChanged canceller does not work properly

[omochi]

Operating System

macOS Ventura 13.4.1（22F82）

Browser Version

Safari 16.5.1 (18615.2.9.11.7)

Firebase SDK Version

9.22.2

Firebase SDK Product:

Auth

Describe your project's tooling

TypeScript, React, Vite.

Describe the problem

In the Firebase Auth library, there is a bug where calling the canceller returned by onAuthStateChanged does not prevent the subsequent values from being emitted.
Although the events when the authentication state changes will no longer be emitted correctly, the very first event that is emitted immediately after subscribing will be delivered, even if the canceller is called before that event.
Upon investigating the cause of this bug, I found an issue with the following code:

firebase-js-sdk/packages/auth/src/core/auth/auth_impl.ts

Line 600 in fe2ac13

promise.then(() => cb(this.currentUser));

Here, the value is directly passed to the callback function without going through the stream, making the canceller of the returned stream non-functional. In a correct implementation, it should be necessary to pass the initial value via the stream.

I encountered this bug while working on a real-world project and it caused difficulties.
React has a feature called StrictMode to ensure that effect clean-up is performed correctly and it's recommended to use it. As a result, the root component renders twice quickly.
The first mount is unmounted immediately, so if the component is implemented correctly and purely, no side effects will occur from the first mount, and the application will behave as if only the second mount took place.
However, due to this bug, it's impossible to cancel the event subscription started in the first mount, making it impossible to make the component pure. As a result, an unintended situation occurs where onAuthStateChanged is triggered twice. This makes it difficult for the programmer to accurately control the behavior during application initialization, leading to instability.

Steps and code to reproduce issue

Additionally, I have created a minimal reproducible code for this bug.

https://github.com/omochi/firebase-auth-js-cancel-bug

You can easily check the bug in the repository below, so please take a look at it if you can.

[DellaBitta]

Hi @omochi,

Thanks for the report and the easy to reproduce test case. I was able to reproduce the error that you reported. I will mark this as a bug for further investigation by the Auth team. Thank you!

[omochi]

Thank you for checking the bug and getting back to me.

[prameshj]

Thanks for this report! We are trying to repro the same with our demo app https://github.com/firebase/firebase-js-sdk/tree/master/packages/auth/demo as well.

Looks like the problem might be in the line you identified -

firebase-js-sdk/packages/auth/src/core/auth/auth_impl.ts

Line 600 in fe2ac13

promise.then(() => cb(this.currentUser));

We wait for the initialization promise to complete and invoke the subscribed callback..But when we invoke the callback, we do not check if the subscription is still active, so that's probably the race condition here.

IIUC, subsequent login/logouts do not trigger the callback, right.. it is just the one time during initial login.

[prameshj]

With the demo app, we were unable to see the issue. However, if we add a delay in the promise.then() block to call the callback, after sleeping for a few seconds, then we see the callback invoked after unsubscribe is done. I believe this is the issue you are seeing too. But i am not sure what is causing the initialization promise to resolve slower (assuming that's the issue). We will try with your app as well.

[omochi]

Yes, registerStateListener does delay invoking cb if AuthImpl is not initialized, as it uses _initializationPromise to wait for initialization.
Indeed, if the invocation of cb is completed during the call to registerStateListener, it won't be a problem because it would be before the user calls the unsubscribe callback.

---

There are several reasons why initialization might be delayed, but let me explain a few of them.
_initializationPromise is constructed in _initializeWithPersistence method, first, it's wrapped in this.queue. This queue is constructed with Promise, and at the first run, the situation will be to use then to connect the process to a completed Promise.
However, invoking then on a Promise does not immediately call the callback, even if the preceding Promise has already been resolved. This can be confirmed with the following snippet.

const a = Promise.resolve();
console.log("before then");
const b = a.then(() => {
    console.log("b");
});
console.log("after then");

In this code, the output of "b" comes after "after then".

Another reason is the delay caused by Indexed DB. In the initialization process constructed within _initializeWithPersistence, initializeCurrentUser is called. If there are no authentication informations on the user's browser, this will call directlySetCurrentUser, and then PersistenceUserManager.removeCurrentUser.

When Firebase uses Indexed DB (which is the default behavior), this results in a call to IndexedDBLocalPersistence._remove, and finally calls IDBObjectStore.delete.
As this is an asynchronous method, it does not complete immediately.

---

By the way, this problem can also occur even if it's already initialized.
This is because when calling registerStateListener, it becomes a form of then on a resolved Promise if it has been initialized, which as previously mentioned, doesn't execute instantly.

---

The problem is not with the delay caused by initialization. The issue lies in that the transmission of this initial value can't be interrupted by unsubscribe. Even if there is a delay in initialization, it is possible to implement so that the event can be canceled by unsubscribe.
In such stream-style programing, all values need to be expressed as streams.
To do this, you create a new stream by combining the stream that sends events and the stream that sends a single fixed value.
Then, all you have to do is return the cancellation of the subscription to this new stream to the user.

[AngelAngelXie]

Hi, thanks so much for the detailed explanation! We investigated your demo code further and tried to trace what is happening under the hood.

As you mentioned earlier, under react StrictMode, the root component is rendered twice, resulting in the following events to occur:

1. Firebase app initializes

2. useEffect called, auth instance obtained, initialization promise queued, onAuthStateChanged1 registered -> waits on initialization promise before triggering callback.

3. useEffect called again due to react strict mode. As a result, the return block is called. This unsubscribes from the onAuthStateChanged listener 1.

4. After the return block, the main block of useEffect is called, registering yet another callback for onAuthStateChanged2 -> waits on initialization promise before triggering callback

5. Since nothing else is in the main queue, js event loop calls the callback for the 2 onAuthStateChanged registered listeners. It should have only called one, since the previous one was unsubscribed.

Because onAuthStateChanged1 has been added to the event loop queue during the first render, it cannot be removed from the queue even though we unsubscribed in the second render.

Our inability to remove the onAuthStateChanged callback during unsubscribe is causing there to be two onAuthStateChanged callback calls described in step 5.

However, we can get around this if we can check to ensure that the listener is still subscribed at the time of the callback call. Currently, we are looking into ways in which we can modify the line below to accomplish this.

firebase-js-sdk/packages/auth/src/core/auth/auth_impl.ts

Line 600 in fe2ac13

promise.then(() => cb(this.currentUser));

Please let us know if this matches with your understanding. In the meantime, we will continue to design a fix for this issue.

[omochi]

Thank you for updating me on the progress of the investigation. The detailed situation where the problem occurs is consistent with my understanding.

As I suggested in my previous comment, I believe it would be desirable to adopt the following correction policy.
We treat the process of sending the current value and the process of sending the event each as a 'stream', Furthermore, we conceive of a stream composed of these two, and we design the process as a subscription against it.
By doing so, the operation of unsubscribing from the subscription will naturally be corrected.
Specifically, the existing registerStateListener is rewritten as follows.

  private createUserStateStream(
    emitter: EmitterStream<User | null>
  ): Observable<User | null> {
    if (this._deleted) {
      return new NeverStream();
    }

    const promise = this._isInitialized
      ? Promise.resolve()
      : this._initializationPromise;
    _assert(promise, this, AuthErrorCode.INTERNAL_ERROR);

    const user = new PromiseStream(
      promise.then(() => this.currentUser)
    );

    return new ConcatStream(
      user,
      emitter
    );
  }

The functioning of the initialization Promise is maintained, and we combine the current value and the event via ConcatStream.

I have created a full implementation and submitted it as pull request (#7430), so I would be pleased if you could use it for reference.

[prameshj]

Thanks for the analysis and the pull request!

I am wondering if, as a smaller scope change, rewriting registerStateListener like this would be sufficient? ( i have not tried this out, merely thinking out aloud here)

  private registerStateListener(
    subscription: Subscription<User>,
    nextOrObserver: NextOrObserver<User>,
    error?: ErrorFn,
    completed?: CompleteFn
  ): Unsubscribe {
    if (this._deleted) {
      return () => {};
    }

+ let isUnsubscribed = false;
    const cb =
      typeof nextOrObserver === 'function'
        ? nextOrObserver
        : nextOrObserver.next.bind(nextOrObserver);

    const promise = this._isInitialized
      ? Promise.resolve()
      : this._initializationPromise;
    _assert(promise, this, AuthErrorCode.INTERNAL_ERROR);
    // The callback needs to be called asynchronously per the spec.
    // eslint-disable-next-line @typescript-eslint/no-floating-promises
    + promise.then(() => {if (isUnsubscribed) { return; }; cb(this.currentUser)});

    if (typeof nextOrObserver === 'function') {
      + Unsubscribe unsubscribe = subscription.addObserver(nextOrObserver, error, completed);
      + return {isUnsubscribed = true; unsubscribe();}
    } else {
      + Unsubscribe unsubscribe = subscription.addObserver(nextOrObserver);
      + return {isUnsubscribed = true; unsubscribe();}
    }
  }

[omochi]

Yes, I believe that would work.

I'm concerned that the ad hoc nature of the logic makes it difficult to maintain, but it's good that it can be resolved with a small change.

[prameshj]

Thanks! If possible, would you be able to try if the smaller change fixes the issue? If not, we can try on our end.

[omochi]

I built patch and submit at #7498.

[omochi]

Patch is merged and fixed the issue.