Merge pull request #300 from firebase/bugfix/race_crash

Fixed race condition when removing listeners.

Author: alexames

database/integration_test/src/integration_test.cc

@@ -112,7 +112,6 @@ TEST(TimestampIsNear, Matcher) {
  EXPECT_THAT(firebase::Variant::EmptyString(), Not(TimestampIsNear(0)));
 }
 
-
 class FirebaseDatabaseTest : public FirebaseTest {
  public:
  FirebaseDatabaseTest();
@@ -151,7 +150,7 @@ class FirebaseDatabaseTest : public FirebaseTest {
 
  static firebase::App* shared_app_;
  static firebase::auth::Auth* shared_auth_;
- 
+
  static bool first_time_;
 
  bool initialized_;
@@ -169,15 +168,13 @@ class FirebaseDatabaseTest : public FirebaseTest {
 // - TearDown: Shut down Database.
 // - Once, after all tests are finished:
 // - TearDownTestSuite: Sign out. Shut down Auth and App.
- 
+
 firebase::App* FirebaseDatabaseTest::shared_app_;
 firebase::auth::Auth* FirebaseDatabaseTest::shared_auth_;
 
 bool FirebaseDatabaseTest::first_time_ = true;
 
-void FirebaseDatabaseTest::SetUpTestSuite() {
- InitializeAppAndAuth();
-}
+void FirebaseDatabaseTest::SetUpTestSuite() { InitializeAppAndAuth(); }
 
 void FirebaseDatabaseTest::InitializeAppAndAuth() {
  LogDebug("Initialize Firebase App.");
@@ -197,14 +194,14 @@ void FirebaseDatabaseTest::InitializeAppAndAuth() {
 
  // Initialize Firebase Auth.
  ::firebase::ModuleInitializer initializer;
- initializer.Initialize(
- shared_app_, &shared_auth_, [](::firebase::App* app, void* target) {
- LogDebug("Attempting to initialize Firebase Auth.");
- ::firebase::InitResult result;
- *reinterpret_cast<firebase::auth::Auth**>(target) =
- ::firebase::auth::Auth::GetAuth(app, &result);
- return result;
- });
+ initializer.Initialize(shared_app_, &shared_auth_,
+   [](::firebase::App* app, void* target) {
+  LogDebug("Attempting to initialize Firebase Auth.");
+  ::firebase::InitResult result;
+  *reinterpret_cast<firebase::auth::Auth**>(target) =
+  ::firebase::auth::Auth::GetAuth(app, &result);
+  return result;
+  });
 
  WaitForCompletion(initializer.InitializeLastResult(), "InitializeAuth");
 
@@ -219,9 +216,7 @@ void FirebaseDatabaseTest::InitializeAppAndAuth() {
  SignIn();
 }
 
-void FirebaseDatabaseTest::TearDownTestSuite() {
- TerminateAppAndAuth();
-}
+void FirebaseDatabaseTest::TearDownTestSuite() { TerminateAppAndAuth(); }
 
 void FirebaseDatabaseTest::TerminateAppAndAuth() {
  if (shared_auth_) {
@@ -283,7 +278,7 @@ void FirebaseDatabaseTest::InitializeDatabase() {
  LogDebug("Attempting to initialize Firebase Database.");
  ::firebase::InitResult result;
  *reinterpret_cast<firebase::database::Database**>(target) =
- firebase::database::Database::GetInstance(app, &result);
+  firebase::database::Database::GetInstance(app, &result);
  return result;
  });
 
@@ -345,9 +340,9 @@ void FirebaseDatabaseTest::SignOut() {
  }
  if (shared_auth_->current_user()->is_anonymous()) {
  // If signed in anonymously, delete the anonymous user.
- WaitForCompletion(shared_auth_->current_user()->Delete(), "DeleteAnonymousUser");
- }
- else {
+ WaitForCompletion(shared_auth_->current_user()->Delete(),
+  "DeleteAnonymousUser");
+ } else {
  // If not signed in anonymously (e.g. if the tests were modified to sign in
  // as an actual user), just sign out normally.
  shared_auth_->SignOut();
@@ -771,7 +766,7 @@ TEST_F(FirebaseDatabaseTest, TestQueryFiltering) {
  UnorderedElementsAre(Key("fig")));
  }
 }
-#endif  // !defined(ANDROID)
+#endif // !defined(ANDROID)
 
 // A simple ValueListener that logs the values it sees.
 class LoggingValueListener : public firebase::database::ValueListener {
@@ -804,6 +799,72 @@ class LoggingValueListener : public firebase::database::ValueListener {
  bool got_error_;
 };
 
+TEST_F(FirebaseDatabaseTest, TestAddAndRemoveListenerRace) {
+ const char* test_name = test_info_->name();
+
+ SignIn();
+
+ firebase::database::DatabaseReference ref = CreateWorkingPath();
+ WaitForCompletion(ref.Child(test_name).SetValue(0), "SetValue");
+
+ const int kTestIterations = 100;
+
+ // Ensure adding, removing and deleting a listener in rapid succession is safe
+ // from race conditions.
+ for (int i = 0; i < kTestIterations; i++) {
+ LoggingValueListener* listener = new LoggingValueListener();
+ ref.Child(test_name).AddValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ delete listener;
+ }
+
+ // Ensure adding, removing and deleting the same listener twice in rapid
+ // succession is safe from race conditions.
+ for (int i = 0; i < kTestIterations; i++) {
+ LoggingValueListener* listener = new LoggingValueListener();
+ ref.Child(test_name).AddValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ ref.Child(test_name).AddValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ delete listener;
+ }
+
+ // Ensure adding, removing and deleting the same listener twice in rapid
+ // succession is safe from race conditions.
+ for (int i = 0; i < kTestIterations; i++) {
+ LoggingValueListener* listener = new LoggingValueListener();
+ ref.Child(test_name).AddValueListener(listener);
+ ref.Child(test_name).AddValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ delete listener;
+ }
+
+ // Ensure removing a listener too many times is benign.
+ for (int i = 0; i < kTestIterations; i++) {
+ LoggingValueListener* listener = new LoggingValueListener();
+ ref.Child(test_name).AddValueListener(listener);
+ ref.Child(test_name).AddValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ ref.Child(test_name).RemoveValueListener(listener);
+ delete listener;
+ }
+
+ // Ensure adding, removing and deleting difference listeners in rapid
+ // succession is safe from race conditions.
+ for (int i = 0; i < kTestIterations; i++) {
+ LoggingValueListener* listener1 = new LoggingValueListener();
+ LoggingValueListener* listener2 = new LoggingValueListener();
+ ref.Child(test_name).AddValueListener(listener1);
+ ref.Child(test_name).AddValueListener(listener2);
+ ref.Child(test_name).RemoveValueListener(listener1);
+ ref.Child(test_name).RemoveValueListener(listener2);
+ delete listener1;
+ delete listener2;
+ }
+}
+
 TEST_F(FirebaseDatabaseTest, TestValueListener) {
  const char* test_name = test_info_->name();
 

database/src/desktop/core/event_registration.cc

@@ -20,6 +20,34 @@ namespace internal {
 
 EventRegistration::~EventRegistration() {}
 
+void EventRegistration::SafelyFireEvent(const Event& event) {
+ // Ensure that the listener has not already been removed.
+ //
+ // The main thread may remove listeners at any time, so we should not call any
+ // callbacks on a listener that has been removed. This does not protect
+ // callbacks that are currently running when the listener is removed: those
+ // must handled by a mutex from within the callback.
+ if (status_ == kRemoved) {
+ return;
+ }
+
+ FireEvent(event);
+}
+
+void EventRegistration::SafelyFireCancelEvent(Error error) {
+ // Ensure that the listener has not already been removed.
+ //
+ // The main thread may remove listeners at any time, so we should not call any
+ // callbacks on a listener that has been removed. This does not protect
+ // callbacks that are currently running when the listener is removed: those
+ // must handled by a mutex from within the callback.
+ if (status_ == kRemoved) {
+ return;
+ }
+
+ FireCancelEvent(error);
+}
+
 } // namespace internal
 } // namespace database
 } // namespace firebase

database/src/desktop/core/event_registration.h

@@ -35,7 +35,7 @@ struct Event;
 class EventRegistration {
  public:
  explicit EventRegistration(const QuerySpec& query_spec)
- : query_spec_(query_spec) {}
+ : status_(kActive), query_spec_(query_spec) {}
 
  virtual ~EventRegistration();
 
@@ -49,10 +49,10 @@ class EventRegistration {
 
  // Execute the event. Generally this means consuming the Event data and using
  // it to trigger a Listener.
- virtual void FireEvent(const Event& event) = 0;
+ void SafelyFireEvent(const Event& event);
 
  // Cancel the event, passing along the given error code.
- virtual void FireCancelEvent(Error error) = 0;
+ void SafelyFireCancelEvent(Error error);
 
  // Returns true if this EventRegistration contains the given listener.
  // Notes: This takes a void* because ValueListener and ChildListener do not
@@ -67,7 +67,29 @@ class EventRegistration {
  is_user_initiated_ = is_user_initiated;
  }
 
+ // Indicates whether this EventRegistration has been removed from the
+ // database. A removed EventRegistration will not fire any incoming Events.
+ enum Status {
+ kRemoved,
+ kActive,
+ };
+
+ // Returns the current status of this EventRegistration, indicating whether
+ // this registration has been removed from the database or not.
+ Status status() { return status_; }
+
+ // Set the status of this EventRegistration. When an EventRegistration is
+ // marked as kRemoved, it will no longer fire events.
+ void set_status(Status status) { status_ = status; }
+
+ protected:
+ virtual void FireEvent(const Event& event) = 0;
+
+ virtual void FireCancelEvent(Error error) = 0;
+
  private:
+ Status status_;
+
  QuerySpec query_spec_;
 
  bool is_user_initiated_;

database/src/desktop/core/repo.cc

@@ -527,9 +527,9 @@ void Repo::DeferredInitialization() {
 void Repo::PostEvents(const std::vector<Event>& events) {
  for (const Event& event : events) {
  if (event.type != kEventTypeError) {
- event.event_registration->FireEvent(event);
+ event.event_registration->SafelyFireEvent(event);
  } else {
- event.event_registration->FireCancelEvent(event.error);
+ event.event_registration->SafelyFireCancelEvent(event.error);
  }
  }
 }

database/src/desktop/database_desktop.cc

@@ -52,19 +52,15 @@ SingleValueListener::SingleValueListener(DatabaseInternal* database,
  future_(future),
  handle_(handle) {}
 
-SingleValueListener::~SingleValueListener() {
- database_->RemoveSingleValueListener(this);
-}
+SingleValueListener::~SingleValueListener() {}
 
 void SingleValueListener::OnValueChanged(const DataSnapshot& snapshot) {
  future_->CompleteWithResult<DataSnapshot>(handle_, kErrorNone, "", snapshot);
- delete this;
 }
 
 void SingleValueListener::OnCancelled(const Error& error_code,
  const char* error_message) {
  future_->Complete(handle_, error_code, error_message);
- delete this;
 }
 
 DatabaseInternal::DatabaseInternal(App* app)
@@ -94,16 +90,6 @@ DatabaseInternal::~DatabaseInternal() {
  // If initialization failed, there is nothing to clean up.
  if (app_ == nullptr) return;
 
- // If there are any pending listeners, delete their pointers.
- {
- MutexLock lock(listener_mutex_);
- for (SingleValueListener** listener_holder : single_value_listeners_) {
- delete *listener_holder;
- *listener_holder = nullptr;
- }
- single_value_listeners_.clear();
- }
-
  app_->function_registry()->CallFunction(
  ::firebase::internal::FnAuthStopTokenListener, app_, nullptr, nullptr);
 }
@@ -224,6 +210,12 @@ bool DatabaseInternal::RegisterValueListener(
 
 bool DatabaseInternal::UnregisterValueListener(const internal::QuerySpec& spec,
  ValueListener* listener) {
+ EventRegistration* registration =
+ ActiveEventRegistration(spec, (void*)listener);
+ if (registration) {
+ registration->set_status(EventRegistration::kRemoved);
+ }
+
  MutexLock lock(listener_mutex_);
  if (value_listeners_by_query_.Unregister(spec, listener)) {
  auto found = cleanup_value_listener_lookup_.find(listener);
@@ -284,6 +276,32 @@ void DatabaseInternal::UnregisterAllChildListeners(
  }
 }
 
+void DatabaseInternal::AddEventRegistration(
+ const QuerySpec& query_spec, void* listener_ptr,
+ EventRegistration* event_registration) {
+ event_registration_lookup_[query_spec][listener_ptr].push_back(
+ event_registration);
+}
+
+EventRegistration* DatabaseInternal::ActiveEventRegistration(
+ const QuerySpec& query_spec, void* listener_ptr) {
+ auto* listener_registration_map =
+ MapGet(&event_registration_lookup_, query_spec);
+ if (!listener_registration_map) {
+ return nullptr;
+ }
+ auto* registration_vector = MapGet(listener_registration_map, listener_ptr);
+ if (!registration_vector) {
+ return nullptr;
+ }
+ for (auto* registration : *registration_vector) {
+ if (registration->status() == EventRegistration::kActive) {
+ return registration;
+ }
+ }
+ return nullptr;
+}
+
 void DatabaseInternal::EnsureRepo() {
  MutexLock lock(repo_mutex_);
  if (!repo_) {