fix(ScrollAction): unsafe non-null assertion (#157855)

This conditional most likely was meant to use `||` instead of `&&`
operator to check if `notificationContext` is null OR if no Scrollable
was found for this `notificationContext`. Using the AND operator instead
causes the null assertion to fail when notificationContext is null,
while also causing an assertion failure when notificationContext is not
null, but no Scrollable is found for it. While at it, let's also
deduplicate the Scrollable.maybeOf call.

This fix is purely speculative, I didn't encounter this error, but
rather was reading the ScrollAction's source and stumbled upon this.
~~With that said I wasn't actually able to trigger this error condition,
thus why there are no tests included with this PR, if anyone has ideas
on how this could be triggered, I'll be happy to include such test
cases.~~

fixes #158063

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This commit is contained in:
Kamil Szczęk 2024-11-22 20:07:21 +01:00 committed by GitHub
parent b13770f65a
commit 7bf73e8bdc
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 92 additions and 6 deletions

View File

@ -460,17 +460,18 @@ class ScrollAction extends ContextAction<ScrollIntent> {
return true;
}());
if (primaryScrollController.position.context.notificationContext == null
&& Scrollable.maybeOf(primaryScrollController.position.context.notificationContext!) == null) {
final BuildContext? notificationContext = primaryScrollController.position.context.notificationContext;
if (notificationContext != null) {
state = Scrollable.maybeOf(notificationContext);
}
if (state == null) {
return;
}
state = Scrollable.maybeOf(primaryScrollController.position.context.notificationContext!);
}
assert(state != null, '$ScrollAction was invoked on a context that has no scrollable parent');
assert(state!.position.hasPixels, 'Scrollable must be laid out before it can be scrolled via a ScrollAction');
assert(state.position.hasPixels, 'Scrollable must be laid out before it can be scrolled via a ScrollAction');
// Don't do anything if the user isn't allowed to scroll.
if (state!.resolvedPhysics != null && !state.resolvedPhysics!.shouldAcceptUserOffset(state.position)) {
if (state.resolvedPhysics != null && !state.resolvedPhysics!.shouldAcceptUserOffset(state.position)) {
return;
}
final double increment = getDirectionalIncrement(state, intent);

View File

@ -4,6 +4,7 @@
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';
@ -11,6 +12,21 @@ final LogicalKeyboardKey modifierKey = defaultTargetPlatform == TargetPlatform.m
? LogicalKeyboardKey.metaLeft
: LogicalKeyboardKey.controlLeft;
class _NoNotificationContextScrollable extends Scrollable {
const _NoNotificationContextScrollable({
super.controller,
required super.viewportBuilder,
});
@override
ScrollableState createState() => _NoNotificationContextScrollableState();
}
class _NoNotificationContextScrollableState extends ScrollableState {
@override
BuildContext? get notificationContext => null;
}
void main() {
group('ScrollableDetails', (){
test('copyWith / == / hashCode', () {
@ -590,4 +606,73 @@ void main() {
expect(find.text('The cape as red as blood'), findsOneWidget);
expect(find.text('The hair as yellow as corn'), findsOneWidget);
});
// Regression test for https://github.com/flutter/flutter/issues/158063.
testWidgets('Invoking a ScrollAction when notificationContext is null does not cause an exception.', (WidgetTester tester) async {
const List<LogicalKeyboardKey> keysWithModifier = <LogicalKeyboardKey>[
LogicalKeyboardKey.arrowDown, LogicalKeyboardKey.arrowUp,
];
const List<LogicalKeyboardKey> keys = <LogicalKeyboardKey>[
...keysWithModifier,
LogicalKeyboardKey.pageDown, LogicalKeyboardKey.pageUp,
];
final ScrollController controller = ScrollController();
addTearDown(controller.dispose);
await tester.pumpWidget(
MaterialApp(
theme: ThemeData(platform: TargetPlatform.fuchsia),
home: PrimaryScrollController(
controller: controller,
child: Focus(
autofocus: true,
child: _NoNotificationContextScrollable(
controller: controller,
viewportBuilder: (BuildContext context, ViewportOffset offset) => Viewport(
offset: offset,
slivers: List<Widget>.generate(
20,
(int index) => SliverToBoxAdapter(
child: SizedBox(
key: ValueKey<String>('Box $index'),
height: 50.0,
),
),
),
),
),
),
),
),
);
// Verify the initial scroll offset.
await tester.pumpAndSettle();
expect(controller.position.pixels, equals(0.0));
expect(
tester.getRect(find.byKey(const ValueKey<String>('Box 0'), skipOffstage: false)),
equals(const Rect.fromLTRB(0.0, 0.0, 800.0, 50.0)),
);
for (final LogicalKeyboardKey key in keys) {
// The default web shortcuts do not use a modifier key for ScrollActions.
if (!kIsWeb && keysWithModifier.contains(key)) {
await tester.sendKeyDownEvent(modifierKey);
}
await tester.sendKeyEvent(key);
expect(tester.takeException(), isNull);
if (!kIsWeb && keysWithModifier.contains(key)) {
await tester.sendKeyUpEvent(modifierKey);
}
// No scrollable is found, so the scroll position should not change.
await tester.pumpAndSettle();
expect(controller.position.pixels, equals(0.0));
expect(
tester.getRect(find.byKey(const ValueKey<String>('Box 0'), skipOffstage: false)),
equals(const Rect.fromLTRB(0.0, 0.0, 800.0, 50.0)),
);
}
}, variant: KeySimulatorTransitModeVariant.all());
}