Fix potential null exception of OverlayEntry in Slider #124514

liumcse · 2023-04-10T14:18:08Z

When Slider is disposed while user is interacting with the Slider, null pointer exception will be thrown. This PR fixes it by saving overlayEntry to a local variable when calling showValueIndicator() instead of using ! to force converting potentially null value to non-null value.

Also using ? instead of ! In dispose() method.

List which issues are fixed by this PR. You must list at least one issue.

Internal bug: b/273666179, b/192329942

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

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

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

flutter-dashboard · 2023-04-10T14:18:12Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

liumcse · 2023-04-10T22:00:44Z

Ready for review

chunhtai

I can't think of a way the showValueIndicator can be null, I think there may be something more going on there

chunhtai · 2023-04-14T20:35:43Z

packages/flutter/lib/src/material/slider.dart

      curve: Curves.fastOutSlowIn,
    )..addStatusListener((AnimationStatus status) {
      if (status == AnimationStatus.dismissed && _state.overlayEntry != null) {
-        _state.overlayEntry!.remove();


The original code seems fine to me, I think if this become null somehow. Some thing may be broken and we would want to know

chunhtai · 2023-04-14T20:40:19Z

packages/flutter/lib/src/material/slider.dart


  void showValueIndicator() {
    if (overlayEntry == null) {
-      overlayEntry = OverlayEntry(


I can't see how it is possible that overlayEntry become null before you insert.

chunhtai · 2023-04-14T20:40:45Z

packages/flutter/test/material/slider_test.dart

    expect(SchedulerBinding.instance.transientCallbackCount, equals(0));
  });

+  testWidgets('Should not throw exception when dragged after Slider is disposed', (WidgetTester tester) async {


This test pass even without the change, is this testing a real issue?

@liumcse
It seems that the test passes without the fix on the master.

TahaTesser · 2023-04-21T21:43:23Z

Thanks for updating the PR. I'll take a look on Monday.

TahaTesser · 2023-04-24T12:57:41Z

@liumcse

When Slider is disposed while user is interacting with the Slider, null pointer exception will be thrown.

Is this reproducible in a code sample?

Here I created a code that disposes Slider with a timer while dragging the thumb. I couldn't reproduce a null pointer exception error.

Run the code sample and drag the slider until the page disposes. Check the logs there is no error.

code sample

import 'dart:async';

import 'package:flutter/material.dart';

/// Flutter code sample for [Slider].

void main() => runApp(const SliderApp());

class SliderApp extends StatelessWidget {
  const SliderApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      // showSemanticsDebugger: true,
      // showPerformanceOverlay: true,
      theme: ThemeData(
        colorSchemeSeed: const Color(0xff6750a4),
        useMaterial3: true,
      ),
      home: const Home(),
    );
  }
}

class Home extends StatelessWidget {
  const Home({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: ElevatedButton(
        onPressed: () {
          Navigator.push(context,
              MaterialPageRoute<Widget>(builder: (_) => const SliderExample()));
        },
        child: Text('Click me'),
      ),
    );
  }
}

class SliderExample extends StatefulWidget {
  const SliderExample({super.key});

  @override
  State<SliderExample> createState() => _SliderExampleState();
}

class _SliderExampleState extends State<SliderExample> {
  double _currentSliderValue = 20;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Slider(
        value: _currentSliderValue,
        max: 100,
        divisions: 5,
        onChangeStart: (double value) {
          Timer(const Duration(seconds: 2), () {
            Navigator.pop(context);
          });
        },
        label: _currentSliderValue.round().toString(),
        onChanged: (double value) {
          setState(() {
            _currentSliderValue = value;
          });
        },
      ),
    );
  }
}

TahaTesser · 2023-05-12T12:36:50Z

cc: @chunhtai
In order to address this we'd need the internal bug to be tracked in a Flutter issue with more details.
We can probably close this in the meantime since the fix and test aren't clear.

chunhtai · 2023-05-12T16:06:05Z

I agree. @liumcse I think we need a repro before we can fix this issue. Otherwise, it would likely to be a workaround that would bite us back in the future. I will be closing this PR, feel free to reopen if you have time to come back to this pr

This is a follow up to the following pull requests: - #124514 I was finally able to reproduce this bug and found out why it was happening. Consider this code: ```dart GestureDetector( behavior: HitTestBehavior.translucent, // Note: Make sure onTap is not null to ensure events // are captured by `GestureDetector` onTap: () {}, child: _shouldShowSlider ? Slider(value: _value, onChanged: _handleSlide) : const SizedBox.shrink(). ) ``` Runtime exception happens when: 1. User taps and holds the Slider (drag callback captured by `GestureDetector`) 2. `_shouldShowSlider` changes to false, Slider disappears and unmounts, and unregisters `_handleSlide`. But the callback is still registered by `GestureDetector` 3. Users moves finger as if Slider were still there 4. Drag callback is invoked, `_SliderState.showValueIndicator` is called 5. Exception - Slider is already disposed This pull request fixes it by adding a mounted check inside `_SliderState.showValueIndicator` to ensure the Slider is actually mounted at the time of invoking drag event callback. I've added a unit test that will fail without this change. The error stack trace is: ``` The following assertion was thrown while handling a gesture: This widget has been unmounted, so the State no longer has a context (and should be considered defunct). Consider canceling any active work during "dispose" or using the "mounted" getter to determine if the State is still active. When the exception was thrown, this was the stack: #0 State.context.<anonymous closure> (package:flutter/src/widgets/framework.dart:950:9) #1 State.context (package:flutter/src/widgets/framework.dart:956:6) #2 _SliderState.showValueIndicator (package:flutter/src/material/slider.dart:968:18) #3 _RenderSlider._startInteraction (package:flutter/src/material/slider.dart:1487:12) #4 _RenderSlider._handleDragStart (package:flutter/src/material/slider.dart:1541:5) #5 DragGestureRecognizer._checkStart.<anonymous closure> (package:flutter/src/gestures/monodrag.dart:531:53) #6 GestureRecognizer.invokeCallback (package:flutter/src/gestures/recognizer.dart:275:24) #7 DragGestureRecognizer._checkStart (package:flutter/src/gestures/monodrag.dart:531:7) #8 DragGestureRecognizer._checkDrag (package:flutter/src/gestures/monodrag.dart:498:5) #9 DragGestureRecognizer.acceptGesture (package:flutter/src/gestures/monodrag.dart:431:7) #10 _CombiningGestureArenaMember.acceptGesture (package:flutter/src/gestures/team.dart:45:14) #11 GestureArenaManager._resolveInFavorOf (package:flutter/src/gestures/arena.dart:281:12) #12 GestureArenaManager._resolve (package:flutter/src/gestures/arena.dart:239:9) #13 GestureArenaEntry.resolve (package:flutter/src/gestures/arena.dart:53:12) #14 _CombiningGestureArenaMember._resolve (package:flutter/src/gestures/team.dart:85:15) #15 _CombiningGestureArenaEntry.resolve (package:flutter/src/gestures/team.dart:19:15) #16 OneSequenceGestureRecognizer.resolve (package:flutter/src/gestures/recognizer.dart:375:13) #17 DragGestureRecognizer.handleEvent (package:flutter/src/gestures/monodrag.dart:414:13) #18 PointerRouter._dispatch (package:flutter/src/gestures/pointer_router.dart:98:12) #19 PointerRouter._dispatchEventToRoutes.<anonymous closure> (package:flutter/src/gestures/pointer_router.dart:143:9) #20 _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:625:13) #21 PointerRouter._dispatchEventToRoutes (package:flutter/src/gestures/pointer_router.dart:141:18) #22 PointerRouter.route (package:flutter/src/gestures/pointer_router.dart:127:7) #23 GestureBinding.handleEvent (package:flutter/src/gestures/binding.dart:488:19) #24 GestureBinding.dispatchEvent (package:flutter/src/gestures/binding.dart:468:22) #25 RendererBinding.dispatchEvent (package:flutter/src/rendering/binding.dart:439:11) #26 GestureBinding._handlePointerEventImmediately (package:flutter/src/gestures/binding.dart:413:7) #27 GestureBinding.handlePointerEvent (package:flutter/src/gestures/binding.dart:376:5) #28 GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:323:7) #29 GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:292:9) #30 _invoke1 (dart:ui/hooks.dart:186:13) #31 PlatformDispatcher._dispatchPointerDataPacket (dart:ui/platform_dispatcher.dart:433:7) #32 _dispatchPointerDataPacket (dart:ui/hooks.dart:119:31) Handler: "onStart" Recognizer: HorizontalDragGestureRecognizer#a5df2 ``` *List which issues are fixed by this PR. You must list at least one issue.* Internal bug: b/273666179, b/192329942 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

Fix potential null exception of OverlayEntry

5da8ab2

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 10, 2023

liumcse marked this pull request as draft April 10, 2023 14:18

Add test

b656b0b

liumcse marked this pull request as ready for review April 10, 2023 15:52

liumcse changed the title ~~[WIP] Fix potential null exception of OverlayEntry~~ Fix potential null exception of OverlayEntry in Slider Apr 10, 2023

Merge branch 'master' into slider-overlay

900a9b5

liumcse added 4 commits April 12, 2023 00:35

Merge branch 'master' into slider-overlay

9976948

Merge branch 'master' into slider-overlay

d73a185

Merge branch 'master' into slider-overlay

87757d6

Merge branch 'master' into slider-overlay

26be1bc

chunhtai reviewed Apr 14, 2023

View reviewed changes

Merge branch 'flutter:master' into slider-overlay

9aa4253

HansMuller requested a review from TahaTesser April 21, 2023 16:01

chunhtai closed this May 12, 2023

liumcse mentioned this pull request Aug 6, 2023

Slider should check mounted before start interaction #132010

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix potential null exception of OverlayEntry in Slider #124514

Fix potential null exception of OverlayEntry in Slider #124514

Uh oh!

liumcse commented Apr 10, 2023 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Apr 10, 2023

Uh oh!

liumcse commented Apr 10, 2023

Uh oh!

chunhtai left a comment

Uh oh!

chunhtai Apr 14, 2023

Uh oh!

chunhtai Apr 14, 2023

Uh oh!

chunhtai Apr 14, 2023

Uh oh!

TahaTesser Apr 24, 2023

Uh oh!

TahaTesser commented Apr 21, 2023

Uh oh!

TahaTesser commented Apr 24, 2023

Uh oh!

TahaTesser commented May 12, 2023

Uh oh!

chunhtai commented May 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix potential null exception of OverlayEntry in Slider #124514

Fix potential null exception of OverlayEntry in Slider #124514

Uh oh!

Conversation

liumcse commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Apr 10, 2023

Uh oh!

liumcse commented Apr 10, 2023

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

TahaTesser Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Apr 21, 2023

Uh oh!

TahaTesser commented Apr 24, 2023

Uh oh!

TahaTesser commented May 12, 2023

Uh oh!

chunhtai commented May 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

liumcse commented Apr 10, 2023 •

edited

Loading