From cb2c79ad80066b9e84bc2035cfe4660ee6c53bc4 Mon Sep 17 00:00:00 2001 From: russellwheatley Date: Wed, 5 Jun 2024 10:59:07 +0100 Subject: [PATCH 1/4] fix(auth, web): stream handlers are properly cleaned up --- .../lib/firebase_auth_web.dart | 156 +++++++++++++----- .../lib/src/interop/auth.dart | 18 +- 2 files changed, 124 insertions(+), 50 deletions(-) diff --git a/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart b/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart index 0e3eced01aa0..f8edbaafa610 100644 --- a/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart +++ b/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart @@ -24,6 +24,8 @@ import 'src/firebase_auth_web_user_credential.dart'; import 'src/interop/auth.dart' as auth_interop; import 'src/interop/multi_factor.dart' as multi_factor; +enum StateListener { authStateChange, userStateChange, idTokenChange } + /// The web delegate implementation for [FirebaseAuth]. class FirebaseAuthWeb extends FirebaseAuthPlatform { /// Stub initializer to allow the [registerWith] to create an instance without @@ -37,50 +39,9 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform { /// The entry point for the [FirebaseAuthWeb] class. FirebaseAuthWeb({required FirebaseApp app}) : super(appInstance: app) { // Create a app instance broadcast stream for both delegate listener events - _userChangesListeners[app.name] = - StreamController.broadcast(); - _authStateChangesListeners[app.name] = - StreamController.broadcast(); - _idTokenChangesListeners[app.name] = - StreamController.broadcast(); - - // TODO(rrousselGit): close StreamSubscription - delegate.onAuthStateChanged.map((auth_interop.User? webUser) { - if (!_initialized.isCompleted) { - _initialized.complete(); - } - - if (webUser == null) { - return null; - } else { - return UserWeb( - this, - MultiFactorWeb(this, multi_factor.multiFactor(webUser)), - webUser, - _webAuth, - ); - } - }).listen((UserWeb? webUser) { - _authStateChangesListeners[app.name]!.add(webUser); - }); - - // TODO(rrousselGit): close StreamSubscription - // Also triggers `userChanged` events - delegate.onIdTokenChanged.map((auth_interop.User? webUser) { - if (webUser == null) { - return null; - } else { - return UserWeb( - this, - MultiFactorWeb(this, multi_factor.multiFactor(webUser)), - webUser, - _webAuth, - ); - } - }).listen((UserWeb? webUser) { - _idTokenChangesListeners[app.name]!.add(webUser); - _userChangesListeners[app.name]!.add(webUser); - }); + _createStreamListener(app.name, StateListener.authStateChange); + _createStreamListener(app.name, StateListener.idTokenChange); + _createStreamListener(app.name, StateListener.userStateChange); } /// Called by PluginRegistry to register this plugin for Flutter Web @@ -138,6 +99,104 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform { return FirebaseAuthWeb._(); } + bool _cancelUserStream = false; + bool _cancelIdTokenStream = false; + + void _createStreamListener(String appName, StateListener stateListener) { + if (StateListener.authStateChange == stateListener) { + _authStateChangesListeners[appName] = + StreamController.broadcast( + onCancel: () { + _authStateChangesListeners[appName]!.close(); + _authStateChangesListeners.remove(appName); + delegate.authStateController?.close(); + }, + ); + delegate.onAuthStateChanged.map((auth_interop.User? webUser) { + if (!_initialized.isCompleted) { + _initialized.complete(); + } + + if (webUser == null) { + return null; + } else { + return UserWeb( + this, + MultiFactorWeb(this, multi_factor.multiFactor(webUser)), + webUser, + _webAuth, + ); + } + }).listen((UserWeb? webUser) { + _authStateChangesListeners[app.name]!.add(webUser); + }); + } + if (StateListener.idTokenChange == stateListener) { + _cancelIdTokenStream = false; + _idTokenChangesListeners[appName] = + StreamController.broadcast( + onCancel: () { + if (_userChangesListeners[appName] == null) { + // We cannot remove if there is a userChanges listener as we use this stream for it + _idTokenChangesListeners[appName]!.close(); + _idTokenChangesListeners.remove(appName); + delegate.idTokenController?.close(); + } else { + // We need to do this because if idTokenListener and userChanges are being listened to + // We need to cancel both at the same time otherwise neither will be closed & removed + _cancelIdTokenStream = true; + } + + if (_cancelUserStream) { + _userChangesListeners[appName]!.close(); + _userChangesListeners.remove(appName); + } + }, + ); + + // Also triggers `userChanged` events + delegate.onIdTokenChanged.map((auth_interop.User? webUser) { + if (webUser == null) { + return null; + } else { + return UserWeb( + this, + MultiFactorWeb(this, multi_factor.multiFactor(webUser)), + webUser, + _webAuth, + ); + } + }).listen((UserWeb? webUser) { + _idTokenChangesListeners[app.name]!.add(webUser); + _userChangesListeners[app.name]!.add(webUser); + }); + } + + if (StateListener.userStateChange == stateListener) { + _cancelUserStream = false; + _userChangesListeners[appName] = + StreamController.broadcast( + onCancel: () { + if (_idTokenChangesListeners[appName] == null) { + _userChangesListeners[appName]!.close(); + _userChangesListeners.remove(appName); + // There is no delegate for userChanges as we use idTokenChanges + } else { + _cancelUserStream = true; + } + + if (_cancelIdTokenStream) { + // We need to do this because if idTokenListener and userChanges are being listened to + // We need to cancel both at the same time otherwise neither will be closed & removed + _idTokenChangesListeners[appName]!.close(); + _idTokenChangesListeners.remove(appName); + delegate.idTokenController?.close(); + } + }, + ); + } + } + /// instance of Auth from the web plugin auth_interop.Auth? _webAuth; @@ -254,6 +313,9 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform { Stream authStateChanges() async* { await _initialized.future; yield currentUser; + if (_authStateChangesListeners[app.name] == null) { + _createStreamListener(app.name, StateListener.authStateChange); + } yield* _authStateChangesListeners[app.name]!.stream; } @@ -261,6 +323,9 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform { Stream idTokenChanges() async* { await _initialized.future; yield currentUser; + if (_idTokenChangesListeners[app.name] == null) { + _createStreamListener(app.name, StateListener.idTokenChange); + } yield* _idTokenChangesListeners[app.name]!.stream; } @@ -268,6 +333,9 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform { Stream userChanges() async* { await _initialized.future; yield currentUser; + if (_userChangesListeners[app.name] == null) { + _createStreamListener(app.name, StateListener.userStateChange); + } yield* _userChangesListeners[app.name]!.stream; } diff --git a/packages/firebase_auth/firebase_auth_web/lib/src/interop/auth.dart b/packages/firebase_auth/firebase_auth_web/lib/src/interop/auth.dart index 5acfc406fdac..3c0c2c45fad0 100644 --- a/packages/firebase_auth/firebase_auth_web/lib/src/interop/auth.dart +++ b/packages/firebase_auth/firebase_auth_web/lib/src/interop/auth.dart @@ -388,7 +388,9 @@ class Auth extends JsObjectWrapper { JSFunction? _onAuthUnsubscribe; - // TODO(rrousselGit): fix memory leak – the controller isn't closed even in onCancel + StreamController? get authStateController => _changeController; + StreamController? get idTokenController => _idTokenChangedController; + // ignore: close_sinks StreamController? _changeController; @@ -412,9 +414,11 @@ class Auth extends JsObjectWrapper { jsObject.onAuthStateChanged(nextWrapper.toJS, errorWrapper.toJS); } - void stopListen() { - (_onAuthUnsubscribe!.dartify()! as Function)(); + Future stopListen() async { + await (_onAuthUnsubscribe!.callAsFunction() as JSPromise?) + ?.toDart; _onAuthUnsubscribe = null; + _changeController = null; } _changeController = StreamController.broadcast( @@ -430,7 +434,6 @@ class Auth extends JsObjectWrapper { JSFunction? _onIdTokenChangedUnsubscribe; - // TODO(rrousselGit): fix memory leak – the controller isn't closed even in onCancel // ignore: close_sinks StreamController? _idTokenChangedController; @@ -454,9 +457,12 @@ class Auth extends JsObjectWrapper { jsObject.onIdTokenChanged(nextWrapper.toJS, errorWrapper.toJS); } - void stopListen() { - (_onIdTokenChangedUnsubscribe!.dartify()! as Function)(); + Future stopListen() async { + await (_onIdTokenChangedUnsubscribe!.callAsFunction() + as JSPromise?) + ?.toDart; _onIdTokenChangedUnsubscribe = null; + _idTokenChangedController = null; } _idTokenChangedController = StreamController.broadcast( From 43538531c6bd4ff6a673e3694a1ab927da85354d Mon Sep 17 00:00:00 2001 From: russellwheatley Date: Wed, 5 Jun 2024 10:59:43 +0100 Subject: [PATCH 2/4] test: all user stream handlers are opened/closed --- .../firebase_auth_instance_e2e_test.dart | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart b/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart index d64f4db04d37..2810029f0dca 100644 --- a/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart +++ b/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart @@ -196,6 +196,42 @@ void main() { skip: !kIsWeb && Platform.isWindows, ); + group('test all stream listeners', () { + Matcher containsExactlyThreeUsers() => predicate( + (list) => list.where((element) => element is User).length == 3, + 'a list containing exactly 3 User instances', + ); + test('create, cancel and reopen all user event stream handlers', () async { + final auth = FirebaseAuth.instance; + final events = []; + final streamHandler = events.add; + + StreamSubscription userChanges = + auth.userChanges().listen(streamHandler); + + StreamSubscription authStateChanges = + auth.authStateChanges().listen(streamHandler); + + StreamSubscription idTokenChanges = + auth.idTokenChanges().listen(streamHandler); + + await userChanges.cancel(); + await authStateChanges.cancel(); + await idTokenChanges.cancel(); + + userChanges = auth.userChanges().listen(streamHandler); + authStateChanges = auth.authStateChanges().listen(streamHandler); + idTokenChanges = auth.idTokenChanges().listen(streamHandler); + + await auth.signInWithEmailAndPassword( + email: testEmail, + password: testPassword, + ); + + expect(events, containsExactlyThreeUsers()); + }); + }); + group('currentUser', () { test('should return currentUser', () async { await ensureSignedIn(testEmail); From 1d72658577f5b452d75fc524fd292dfbf849ea27 Mon Sep 17 00:00:00 2001 From: russellwheatley Date: Wed, 5 Jun 2024 11:15:54 +0100 Subject: [PATCH 3/4] chore: analyse exception --- .../firebase_auth/firebase_auth_instance_e2e_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart b/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart index 2810029f0dca..425d63c148fd 100644 --- a/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart +++ b/tests/integration_test/firebase_auth/firebase_auth_instance_e2e_test.dart @@ -198,7 +198,7 @@ void main() { group('test all stream listeners', () { Matcher containsExactlyThreeUsers() => predicate( - (list) => list.where((element) => element is User).length == 3, + (list) => list.whereType().length == 3, 'a list containing exactly 3 User instances', ); test('create, cancel and reopen all user event stream handlers', () async { From b5f09984b071d263d09244df19addfd68af5a0c6 Mon Sep 17 00:00:00 2001 From: russellwheatley Date: Wed, 5 Jun 2024 14:19:21 +0100 Subject: [PATCH 4/4] chore: change to switch statement --- .../lib/firebase_auth_web.dart | 167 +++++++++--------- 1 file changed, 84 insertions(+), 83 deletions(-) diff --git a/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart b/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart index f8edbaafa610..02c00a5fbe8a 100644 --- a/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart +++ b/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart @@ -103,97 +103,98 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform { bool _cancelIdTokenStream = false; void _createStreamListener(String appName, StateListener stateListener) { - if (StateListener.authStateChange == stateListener) { - _authStateChangesListeners[appName] = - StreamController.broadcast( - onCancel: () { - _authStateChangesListeners[appName]!.close(); - _authStateChangesListeners.remove(appName); - delegate.authStateController?.close(); - }, - ); - delegate.onAuthStateChanged.map((auth_interop.User? webUser) { - if (!_initialized.isCompleted) { - _initialized.complete(); - } - - if (webUser == null) { - return null; - } else { - return UserWeb( - this, - MultiFactorWeb(this, multi_factor.multiFactor(webUser)), - webUser, - _webAuth, - ); - } - }).listen((UserWeb? webUser) { - _authStateChangesListeners[app.name]!.add(webUser); - }); - } - if (StateListener.idTokenChange == stateListener) { - _cancelIdTokenStream = false; - _idTokenChangesListeners[appName] = - StreamController.broadcast( - onCancel: () { - if (_userChangesListeners[appName] == null) { - // We cannot remove if there is a userChanges listener as we use this stream for it - _idTokenChangesListeners[appName]!.close(); - _idTokenChangesListeners.remove(appName); - delegate.idTokenController?.close(); - } else { - // We need to do this because if idTokenListener and userChanges are being listened to - // We need to cancel both at the same time otherwise neither will be closed & removed - _cancelIdTokenStream = true; + switch (stateListener) { + case StateListener.authStateChange: + _authStateChangesListeners[appName] = + StreamController.broadcast( + onCancel: () { + _authStateChangesListeners[appName]!.close(); + _authStateChangesListeners.remove(appName); + delegate.authStateController?.close(); + }, + ); + delegate.onAuthStateChanged.map((auth_interop.User? webUser) { + if (!_initialized.isCompleted) { + _initialized.complete(); } - if (_cancelUserStream) { - _userChangesListeners[appName]!.close(); - _userChangesListeners.remove(appName); + if (webUser == null) { + return null; + } else { + return UserWeb( + this, + MultiFactorWeb(this, multi_factor.multiFactor(webUser)), + webUser, + _webAuth, + ); } - }, - ); + }).listen((UserWeb? webUser) { + _authStateChangesListeners[app.name]!.add(webUser); + }); + break; + case StateListener.idTokenChange: + _cancelIdTokenStream = false; + _idTokenChangesListeners[appName] = + StreamController.broadcast( + onCancel: () { + if (_userChangesListeners[appName] == null) { + // We cannot remove if there is a userChanges listener as we use this stream for it + _idTokenChangesListeners[appName]!.close(); + _idTokenChangesListeners.remove(appName); + delegate.idTokenController?.close(); + } else { + // We need to do this because if idTokenListener and userChanges are being listened to + // We need to cancel both at the same time otherwise neither will be closed & removed + _cancelIdTokenStream = true; + } - // Also triggers `userChanged` events - delegate.onIdTokenChanged.map((auth_interop.User? webUser) { - if (webUser == null) { - return null; - } else { - return UserWeb( - this, - MultiFactorWeb(this, multi_factor.multiFactor(webUser)), - webUser, - _webAuth, - ); - } - }).listen((UserWeb? webUser) { - _idTokenChangesListeners[app.name]!.add(webUser); - _userChangesListeners[app.name]!.add(webUser); - }); - } + if (_cancelUserStream) { + _userChangesListeners[appName]!.close(); + _userChangesListeners.remove(appName); + } + }, + ); - if (StateListener.userStateChange == stateListener) { - _cancelUserStream = false; - _userChangesListeners[appName] = - StreamController.broadcast( - onCancel: () { - if (_idTokenChangesListeners[appName] == null) { - _userChangesListeners[appName]!.close(); - _userChangesListeners.remove(appName); - // There is no delegate for userChanges as we use idTokenChanges + // Also triggers `userChanged` events + delegate.onIdTokenChanged.map((auth_interop.User? webUser) { + if (webUser == null) { + return null; } else { - _cancelUserStream = true; + return UserWeb( + this, + MultiFactorWeb(this, multi_factor.multiFactor(webUser)), + webUser, + _webAuth, + ); } + }).listen((UserWeb? webUser) { + _idTokenChangesListeners[app.name]!.add(webUser); + _userChangesListeners[app.name]!.add(webUser); + }); + break; + case StateListener.userStateChange: + _cancelUserStream = false; + _userChangesListeners[appName] = + StreamController.broadcast( + onCancel: () { + if (_idTokenChangesListeners[appName] == null) { + _userChangesListeners[appName]!.close(); + _userChangesListeners.remove(appName); + // There is no delegate for userChanges as we use idTokenChanges + } else { + _cancelUserStream = true; + } - if (_cancelIdTokenStream) { - // We need to do this because if idTokenListener and userChanges are being listened to - // We need to cancel both at the same time otherwise neither will be closed & removed - _idTokenChangesListeners[appName]!.close(); - _idTokenChangesListeners.remove(appName); - delegate.idTokenController?.close(); - } - }, - ); + if (_cancelIdTokenStream) { + // We need to do this because if idTokenListener and userChanges are being listened to + // We need to cancel both at the same time otherwise neither will be closed & removed + _idTokenChangesListeners[appName]!.close(); + _idTokenChangesListeners.remove(appName); + delegate.idTokenController?.close(); + } + }, + ); + break; } }