[Bug] Going offline/online after authenticating causes crash

[mikepulaski]

[REQUIRED] Please fill in the following fields:

• Pre-built SDK from the website or open-source from this repo: open source
• Firebase C++ SDK version: 8.1.0
• Problematic Firebase Component: database
• Other Firebase Components in use: auth
• Platform you are using the C++ SDK on: mac, linux
• Platform you are targeting: desktop

[REQUIRED] Please describe the issue here:

An assert failure occurs if going offline/online too quickly after being authenticated. Persistent connections do not cancel their "fetch token" async work, and if two run at the same time, the app may crash.

ASSERT: [...]/firebase-cpp-sdk/database/src/desktop/connection/persistent_connection.cc(578): connection_state_ == kDisconnected

This is problematic for my actual app, as I want to be able to reconnect as soon as the network becomes reachable. If the network becomes unreachable/reachable in rapid succession (very common), it will crash because of this race, which is 100% reproducible.

I noticed that in the ObjC SDK, a guard was added to make sure the "fetch token" routine was still valid when handling its result by using a simple counter. I'd imagine this would have a similarly simple fix.

Steps to reproduce:

Have you been able to reproduce this issue with just the Firebase C++ quickstarts ? yes
What's the issue repro rate? 100%

Relevant Code:

Add the following to desktop/testapp/src/common_main.cc after authenticating (in quickstart-cpp):

database->GoOffline();
database->GoOnline();

If some time has elapsed after authenticating (e.g., the database is connected), you can still reproduce it if you call GoOffline() / GoOnline() more than once.

database->GoOffline();
database->GoOnline();
database->GoOffline();
database->GoOnline();

Here's a forked version of quickstart-cpp with the modifications in-place: https://github.com/mikepulaski/quickstart-cpp/tree/auth-token-race

[chkuang-g]

@mikehaney24 Thanks for reporting this. I think this is a legit issue.

The assert occurred here.

firebase-cpp-sdk/database/src/desktop/connection/persistent_connection.cc

Line 578 in 3c21909

FIREBASE_DEV_ASSERT(connection_state_ == kDisconnected);

When GoOffline() is called, it would change the stated here.

firebase-cpp-sdk/database/src/desktop/connection/persistent_connection.cc

Line 490 in 3c21909

connection_state_ = kDisconnected;

I guess it is possible to have race here (though all these calls are serialized in the same thread)

Though I cannot find "guard" in ObjC SDK. This seems to the same logic in C++.
https://github.com/firebase/firebase-ios-sdk/blob/4d518509ed2988e2be4edf61c770aa4c2495061d/FirebaseDatabase/Sources/Core/FPersistentConnection.m#L515

Or Android SDK
https://github.com/firebase/firebase-android-sdk/blob/cd6d1bcb8e3a6c070cb6cc76acdbdc65871cac24/firebase-database/src/main/java/com/google/firebase/database/connection/PersistentConnectionImpl.java#L771

Could you point me to the part of the code about what you meant by the "guard"?

Thank you!
Shawn

[mikepulaski]

Hey Shawn,

[…]This seems to the same logic in C++. https://github.com/firebase/firebase-ios-sdk/blob/4d518509ed2988e2be4edf61c770aa4c2495061d/FirebaseDatabase/Sources/Core/FPersistentConnection.m#L515 <https://github.com/firebase/firebase-ios-sdk/blob/4d518509ed2988e2be4edf61c770aa4c2495061d/FirebaseDatabase/Sources/Core/FPersistentConnection.m#L515>

It’s easy to miss, but the ObjC version has the following statement, which (I believe) protects against this sort of thing: ```if (thisFetchTokenAttempt == self.currentFetchTokenAttempt) { /*…/* }``` The ObjC block captures “thisFetchTokenAttempt” and compares it to one stored as an ivar. That was the “guard” I was talking about which I think would work pretty much the same with a lambda :) Thanks for looking into this!

…

Could you point me to the part of the code about what you meant by the "guard"? Thank you! Shawn — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#594 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALQOXPXBW6Y66N3M22VKDUBFZHXANCNFSM5B3PIYYA>.

[DellaBitta]

Hi @mikepulaski,

I was able to reproduce this in our integration test as well, but calling GoOffline and Online a few times in the FirebaseDatabaseTest::SetUp method.

Code:

void FirebaseDatabaseTest::SetUp() {
  FirebaseTest::SetUp();
  InitializeDatabase();
  if(database_ != nullptr) {
    database_->GoOffline();
    database_->GoOnline();
    database_->GoOffline();
    database_->GoOnline();
    database_->GoOffline();
    database_->GoOnline();
  }
}

Output:

[ RUN      ] FirebaseDatabaseTest.TestSignIn
DEBUG: Initializing Firebase Database.
DEBUG: Attempting to initialize Firebase Database.
DEBUG: WaitForCompletion InitializeDatabase
DEBUG: Successfully initialized Firebase Database.
DEBUG: Shutdown the Database library.
ASSERT: firebase-cpp-sdk/database/src/desktop/connection/persistent_connection.cc(578): connection_state_ == kDisconnected

I'm going to mark this as a bug, thanks for the report!