Skip to content

Commit a0de4ed

Browse files
russellwheatleyrussellwheatley
authored
fix: update core, auth and app-check logic so internal resources on method channels are properly disposed (#18268)
* test: update CI tests failing * chore: update user to handle integration tests * test: comment out auth test causing ios failures * test: remove duplicate ai test runner * ci: stop running app again if it is already booted * refactor: ensure AppCheck and Auth method channels clean up plugins if app is deleted * test: test clean up logic is working as intended * test: fix register service * test: android is failing on a couple of e2e tests * test: unskip auth test * chore: trailing comma * test: skip auth test failing on iOS
1 parent 9783a44 commit a0de4ed

19 files changed

Lines changed: 384 additions & 85 deletions

File tree

‎.github/workflows/ios.yaml‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -133,16 +133,9 @@ jobs:
133133
run: |
134134
# Uncomment following line to have simulator logs printed out for debugging purposes.
135135
# xcrun simctl spawn booted log stream --predicate 'eventMessage contains "flutter"' &
136-
# The iOS simulator sometimes fails to connect the VM Service, hanging for
137-
# 12 minutes before timing out. Use a 6-minute limit and retry once with
138-
# a simulator reboot. Normal connection takes 30s-5min.
139-
perl -e 'alarm 360; exec @ARGV' -- flutter test integration_test/e2e_test.dart -d "$SIMULATOR" --timeout 10x --dart-define=CI=true || {
140-
echo "First attempt failed or timed out. Rebooting simulator and retrying..."
141-
xcrun simctl shutdown "$SIMULATOR" || true
142-
xcrun simctl boot "$SIMULATOR"
143-
xcrun simctl bootstatus "$SIMULATOR" -b
144-
flutter test integration_test/e2e_test.dart -d "$SIMULATOR" --timeout 10x --dart-define=CI=true
145-
}
136+
# Once the integration test runner has launched, leave it running rather
137+
# than starting a second app instance from a retry.
138+
flutter test integration_test/e2e_test.dart -d "$SIMULATOR" --timeout 10x --dart-define=CI=true
146139
- name: Save Firestore Emulator Cache
147140
# Branches can read main cache but main cannot read branch cache. Avoid LRU eviction with main-only cache.
148141
if: github.ref == 'refs/heads/main'

‎packages/firebase_app_check/firebase_app_check/lib/src/firebase_app_check.dart‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,21 @@ class FirebaseAppCheck extends FirebasePluginPlatform
4343
static FirebaseAppCheck instanceFor({required FirebaseApp app}) {
4444
return _firebaseAppCheckInstances.putIfAbsent(app.name, () {
4545
final instance = FirebaseAppCheck._(app: app);
46-
app.registerService<FirebaseAppCheck>(instance);
46+
app.registerService<FirebaseAppCheck>(
47+
instance,
48+
dispose: (appCheck) => appCheck._dispose(),
49+
);
4750
return instance;
4851
});
4952
}
5053

54+
Future<void> _dispose() async {
55+
_firebaseAppCheckInstances.remove(app.name);
56+
final delegate = _delegatePackingProperty;
57+
_delegatePackingProperty = null;
58+
await delegate?.dispose();
59+
}
60+
5161
/// Activates the Firebase App Check service.
5262
///
5363
/// ## Platform Configuration

‎packages/firebase_app_check/firebase_app_check/test/firebase_app_check_test.dart‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,38 @@ void main() {
4242
expect(appCheck, isA<FirebaseAppCheck>());
4343
expect(appCheck.app.name, 'secondaryApp');
4444
});
45+
46+
test('creates a fresh instance after app delete and reinitialize',
47+
() async {
48+
const appName = 'delete-reinit-app-check';
49+
const options = FirebaseOptions(
50+
appId: '1:1234567890:ios:42424242424242',
51+
apiKey: '123',
52+
projectId: '123',
53+
messagingSenderId: '1234567890',
54+
);
55+
final app = await Firebase.initializeApp(
56+
name: appName,
57+
options: options,
58+
);
59+
final appCheck1 = FirebaseAppCheck.instanceFor(app: app);
60+
61+
expect(app.getService<FirebaseAppCheck>(), same(appCheck1));
62+
63+
await app.delete();
64+
65+
final app2 = await Firebase.initializeApp(
66+
name: appName,
67+
options: options,
68+
);
69+
addTearDown(app2.delete);
70+
71+
final appCheck2 = FirebaseAppCheck.instanceFor(app: app2);
72+
73+
expect(appCheck2, isNot(same(appCheck1)));
74+
expect(appCheck2.app, app2);
75+
expect(app2.getService<FirebaseAppCheck>(), same(appCheck2));
76+
});
4577
});
4678
});
4779
}

‎packages/firebase_app_check/firebase_app_check/test/mock.dart‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,22 @@ void setupFirebaseAppCheckMocks([Callback? customHandlers]) {
1414
TestWidgetsFlutterBinding.ensureInitialized();
1515

1616
setupFirebaseCoreMocks();
17+
TestFirebaseAppHostApi.setUp(MockFirebaseAppHostApi());
18+
}
19+
20+
class MockFirebaseAppHostApi implements TestFirebaseAppHostApi {
21+
@override
22+
Future<void> delete(String appName) async {}
23+
24+
@override
25+
Future<void> setAutomaticDataCollectionEnabled(
26+
String appName,
27+
bool enabled,
28+
) async {}
29+
30+
@override
31+
Future<void> setAutomaticResourceManagementEnabled(
32+
String appName,
33+
bool enabled,
34+
) async {}
1735
}

‎packages/firebase_app_check/firebase_app_check_platform_interface/lib/src/method_channel/method_channel_firebase_app_check.dart‎

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,25 +19,32 @@ class MethodChannelFirebaseAppCheck extends FirebaseAppCheckPlatform {
1919
MethodChannelFirebaseAppCheck({required FirebaseApp app})
2020
: super(appInstance: app) {
2121
_tokenChangesListeners[app.name] = StreamController<String?>.broadcast();
22+
_listenerRegistration = _registerTokenListener(app);
23+
}
24+
25+
Future<void> _registerTokenListener(FirebaseApp app) async {
26+
try {
27+
final channelName = await _pigeonApi.registerTokenListener(app.name);
28+
if (_isDisposed) {
29+
return;
30+
}
2231

23-
_pigeonApi.registerTokenListener(app.name).then((channelName) {
2432
final events = EventChannel(channelName);
25-
events
33+
_subscription = events
2634
.receiveGuardedBroadcastStream(onError: convertPlatformException)
27-
.listen(
28-
(arguments) {
29-
// ignore: close_sinks
30-
StreamController<String?> controller =
31-
_tokenChangesListeners[app.name]!;
35+
.listen((arguments) {
36+
// ignore: close_sinks
37+
final controller = _tokenChangesListeners[app.name];
38+
if (!_isDisposed && controller != null) {
3239
Map<dynamic, dynamic> result = arguments;
3340
controller.add(result['token'] as String?);
34-
},
35-
);
41+
}
42+
});
3643
// ignore: avoid_catches_without_on_clauses
37-
}).catchError((_) {
44+
} catch (_) {
3845
// Silently ignore errors during token listener registration.
3946
// This can happen in test environments where the host API is not set up.
40-
});
47+
}
4148
}
4249

4350
static final Map<String, StreamController<String?>> _tokenChangesListeners =
@@ -49,6 +56,9 @@ class MethodChannelFirebaseAppCheck extends FirebaseAppCheckPlatform {
4956

5057
/// The Pigeon API used for platform communication.
5158
final FirebaseAppCheckHostApi _pigeonApi = FirebaseAppCheckHostApi();
59+
late final Future<void> _listenerRegistration;
60+
StreamSubscription<dynamic>? _subscription;
61+
bool _isDisposed = false;
5262

5363
/// Returns a stub instance to allow the platform interface to access
5464
/// the class instance statically.
@@ -69,6 +79,16 @@ class MethodChannelFirebaseAppCheck extends FirebaseAppCheckPlatform {
6979
});
7080
}
7181

82+
@override
83+
Future<void> dispose() async {
84+
_isDisposed = true;
85+
await _listenerRegistration;
86+
await _subscription?.cancel();
87+
_subscription = null;
88+
await _tokenChangesListeners.remove(app.name)?.close();
89+
_methodChannelFirebaseAppCheckInstances.remove(app.name);
90+
}
91+
7292
@override
7393
MethodChannelFirebaseAppCheck setInitialValues() {
7494
return this;

‎packages/firebase_app_check/firebase_app_check_platform_interface/lib/src/platform_interface/platform_interface_firebase_app_check.dart‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,4 +147,7 @@ abstract class FirebaseAppCheckPlatform extends PlatformInterface {
147147
FirebaseAppCheckPlatform setInitialValues() {
148148
throw UnimplementedError('setInitialValues() is not implemented');
149149
}
150+
151+
/// Disposes resources tied to this platform App Check instance.
152+
Future<void> dispose() async {}
150153
}

‎packages/firebase_auth/firebase_auth/lib/src/firebase_auth.dart‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,21 @@ class FirebaseAuth extends FirebasePluginPlatform implements FirebaseService {
4646
}) {
4747
return _firebaseAuthInstances.putIfAbsent(app.name, () {
4848
final instance = FirebaseAuth._(app: app);
49-
app.registerService<FirebaseAuth>(instance);
49+
app.registerService<FirebaseAuth>(
50+
instance,
51+
dispose: (auth) => auth._dispose(),
52+
);
5053
return instance;
5154
});
5255
}
5356

57+
Future<void> _dispose() async {
58+
_firebaseAuthInstances.remove(app.name);
59+
final delegate = _delegatePackingProperty;
60+
_delegatePackingProperty = null;
61+
await delegate?.dispose();
62+
}
63+
5464
/// Returns the current [User] if they are currently signed-in, or `null` if
5565
/// not.
5666
///

‎packages/firebase_auth/firebase_auth/test/firebase_auth_test.dart‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,38 @@ void main() {
231231
});
232232
});
233233

234+
test('creates a fresh instance after app delete and reinitialize',
235+
() async {
236+
final appName = 'delete-reinit-$testCount';
237+
const options = FirebaseOptions(
238+
apiKey: 'apiKey',
239+
appId: 'appId',
240+
messagingSenderId: 'messagingSenderId',
241+
projectId: 'projectId',
242+
);
243+
final app = await Firebase.initializeApp(
244+
name: appName,
245+
options: options,
246+
);
247+
final auth1 = FirebaseAuth.instanceFor(app: app);
248+
249+
expect(app.getService<FirebaseAuth>(), same(auth1));
250+
251+
await app.delete();
252+
253+
final app2 = await Firebase.initializeApp(
254+
name: appName,
255+
options: options,
256+
);
257+
addTearDown(app2.delete);
258+
259+
final auth2 = FirebaseAuth.instanceFor(app: app2);
260+
261+
expect(auth2, isNot(same(auth1)));
262+
expect(auth2.app, app2);
263+
expect(app2.getService<FirebaseAuth>(), same(auth2));
264+
});
265+
234266
group('tenantId', () {
235267
test('set tenantId should call delegate method', () async {
236268
// Each test uses a unique FirebaseApp instance to avoid sharing state

‎packages/firebase_auth/firebase_auth/test/mock.dart‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ void setupFirebaseAuthMocks([Callback? customHandlers]) {
1212
TestWidgetsFlutterBinding.ensureInitialized();
1313

1414
setupFirebaseCoreMocks();
15+
TestFirebaseAppHostApi.setUp(MockFirebaseAppHostApi());
1516
}
1617

1718
Future<T> neverEndingFuture<T>() async {
@@ -20,3 +21,20 @@ Future<T> neverEndingFuture<T>() async {
2021
await Future.delayed(const Duration(minutes: 5));
2122
}
2223
}
24+
25+
class MockFirebaseAppHostApi implements TestFirebaseAppHostApi {
26+
@override
27+
Future<void> delete(String appName) async {}
28+
29+
@override
30+
Future<void> setAutomaticDataCollectionEnabled(
31+
String appName,
32+
bool enabled,
33+
) async {}
34+
35+
@override
36+
Future<void> setAutomaticResourceManagementEnabled(
37+
String appName,
38+
bool enabled,
39+
) async {}
40+
}

0 commit comments

Comments
 (0)