Skip to content

Commit

Permalink
Fix reentrancy with WidgetBindingObserver callbacks (flutter#131774)
Browse files Browse the repository at this point in the history 
Part of flutter#131678

Fixes up callsites for WidgetsBindingObserver related callbacks.
  • Loading branch information
@dnfield
dnfield authored Aug 2, 2023
1 parent aff8ef1 commit b3f99ff
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 9 deletions.
21 changes: 12 additions & 9 deletions packages/flutter/lib/src/widgets/binding.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
@override
Future<AppExitResponse> handleRequestAppExit() async {
bool didCancel = false;
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
if ((await observer.didRequestAppExit()) == AppExitResponse.cancel) {
didCancel = true;
// Don't early return. For the case where someone is just using the
Expand All @@ -637,31 +637,31 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
@override
void handleMetricsChanged() {
super.handleMetricsChanged();
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didChangeMetrics();
}
}

@override
void handleTextScaleFactorChanged() {
super.handleTextScaleFactorChanged();
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didChangeTextScaleFactor();
}
}

@override
void handlePlatformBrightnessChanged() {
super.handlePlatformBrightnessChanged();
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didChangePlatformBrightness();
}
}

@override
void handleAccessibilityFeaturesChanged() {
super.handleAccessibilityFeaturesChanged();
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didChangeAccessibilityFeatures();
}
}
Expand All @@ -673,6 +673,7 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
/// See [dart:ui.PlatformDispatcher.onLocaleChanged].
@protected
@mustCallSuper
@visibleForTesting
void handleLocaleChanged() {
dispatchLocalesChanged(platformDispatcher.locales);
}
Expand All @@ -686,7 +687,7 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
@protected
@mustCallSuper
void dispatchLocalesChanged(List<Locale>? locales) {
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didChangeLocales(locales);
}
}
Expand All @@ -700,7 +701,7 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
@protected
@mustCallSuper
void dispatchAccessibilityFeaturesChanged() {
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didChangeAccessibilityFeatures();
}
}
Expand All @@ -720,6 +721,7 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
/// This method exposes the `popRoute` notification from
/// [SystemChannels.navigation].
@protected
@visibleForTesting
Future<void> handlePopRoute() async {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
if (await observer.didPopRoute()) {
Expand All @@ -741,6 +743,7 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
/// [SystemChannels.navigation].
@protected
@mustCallSuper
@visibleForTesting
Future<void> handlePushRoute(String route) async {
final RouteInformation routeInformation = RouteInformation(uri: Uri.parse(route));
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
Expand Down Expand Up @@ -777,15 +780,15 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB
@override
void handleAppLifecycleStateChanged(AppLifecycleState state) {
super.handleAppLifecycleStateChanged(state);
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didChangeAppLifecycleState(state);
}
}

@override
void handleMemoryPressure() {
super.handleMemoryPressure();
for (final WidgetsBindingObserver observer in _observers) {
for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
observer.didHaveMemoryPressure();
}
}
Expand Down
108 changes: 108 additions & 0 deletions packages/flutter/test/widgets/binding_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:ui';

import 'package:flutter/foundation.dart';
import 'package:flutter/services.dart';
import 'package:flutter/widgets.dart';
Expand Down Expand Up @@ -45,6 +47,94 @@ class PushRouteInformationObserver with WidgetsBindingObserver {
}
}

// Implements to make sure all methods get coverage.
class RentrantObserver implements WidgetsBindingObserver {
RentrantObserver() {
WidgetsBinding.instance.addObserver(this);
}

bool active = true;

int removeSelf() {
active = false;
int count = 0;
while (WidgetsBinding.instance.removeObserver(this)) {
count += 1;
}
return count;
}

@override
void didChangeAccessibilityFeatures() {
assert(active);
WidgetsBinding.instance.addObserver(this);
}

@override
void didChangeAppLifecycleState(AppLifecycleState state) {
assert(active);
WidgetsBinding.instance.addObserver(this);
}

@override
void didChangeLocales(List<Locale>? locales) {
assert(active);
WidgetsBinding.instance.addObserver(this);
}

@override
void didChangeMetrics() {
assert(active);
WidgetsBinding.instance.addObserver(this);
}

@override
void didChangePlatformBrightness() {
assert(active);
WidgetsBinding.instance.addObserver(this);
}

@override
void didChangeTextScaleFactor() {
assert(active);
WidgetsBinding.instance.addObserver(this);
}

@override
void didHaveMemoryPressure() {
assert(active);
WidgetsBinding.instance.addObserver(this);
}

@override
Future<bool> didPopRoute() {
assert(active);
WidgetsBinding.instance.addObserver(this);
return Future<bool>.value(true);
}

@override
Future<bool> didPushRoute(String route) {
assert(active);
WidgetsBinding.instance.addObserver(this);
return Future<bool>.value(true);
}

@override
Future<bool> didPushRouteInformation(RouteInformation routeInformation) {
assert(active);
WidgetsBinding.instance.addObserver(this);
return Future<bool>.value(true);
}

@override
Future<AppExitResponse> didRequestAppExit() {
assert(active);
WidgetsBinding.instance.addObserver(this);
return Future<AppExitResponse>.value(AppExitResponse.exit);
}
}

void main() {
Future<void> setAppLifeCycleState(AppLifecycleState state) async {
final ByteData? message =
Expand All @@ -53,6 +143,23 @@ void main() {
.handlePlatformMessage('flutter/lifecycle', message, (_) { });
}

testWidgets('Rentrant observer callbacks do not result in exceptions', (WidgetTester tester) async {
final RentrantObserver observer = RentrantObserver();
WidgetsBinding.instance.handleAccessibilityFeaturesChanged();
WidgetsBinding.instance.handleAppLifecycleStateChanged(AppLifecycleState.resumed);
WidgetsBinding.instance.handleLocaleChanged();
WidgetsBinding.instance.handleMetricsChanged();
WidgetsBinding.instance.handlePlatformBrightnessChanged();
WidgetsBinding.instance.handleTextScaleFactorChanged();
WidgetsBinding.instance.handleMemoryPressure();
WidgetsBinding.instance.handlePopRoute();
WidgetsBinding.instance.handlePushRoute('/');
WidgetsBinding.instance.handleRequestAppExit();
await tester.idle();
expect(observer.removeSelf(), greaterThan(1));
expect(observer.removeSelf(), 0);
});

testWidgets('didHaveMemoryPressure callback', (WidgetTester tester) async {
final MemoryPressureObserver observer = MemoryPressureObserver();
WidgetsBinding.instance.addObserver(observer);
Expand Down Expand Up @@ -118,6 +225,7 @@ void main() {

observer.accumulatedStates.clear();
await expectLater(() async => setAppLifeCycleState(AppLifecycleState.detached), throwsAssertionError);
WidgetsBinding.instance.removeObserver(observer);
});

testWidgets('didPushRoute callback', (WidgetTester tester) async {
Expand Down

0 comments on commit b3f99ff

Please sign in to comment.