Skip to content

Conversation

@liumcse
Copy link
Contributor

@liumcse liumcse commented Aug 6, 2023

This is a follow up to the following pull requests:

I was finally able to reproduce this bug and found out why it was happening. Consider this code:

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.

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Aug 6, 2023
@liumcse liumcse force-pushed the slider/check-mount-before-interact branch from 96adc54 to 82c8a87 Compare August 6, 2023 13:32
@TahaTesser TahaTesser self-requested a review August 7, 2023 07:42
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TahaTesser TahaTesser requested review from Piinks and chunhtai and removed request for Piinks August 7, 2023 09:33
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter_LGTM

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 7, 2023
@auto-submit auto-submit bot merged commit 06ca902 into flutter:master Aug 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 8, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 8, 2023
flutter/flutter@ad0aa8d...436df69

2023-08-08 [email protected] Roll Flutter Engine from 9c83d90b01bd to 146c4c9487fc (6 revisions) (flutter/flutter#132112)
2023-08-08 [email protected] Roll Flutter Engine from c27109291e22 to 9c83d90b01bd (5 revisions) (flutter/flutter#132108)
2023-08-08 [email protected] Roll Flutter Engine from be085f6699b6 to c27109291e22 (3 revisions) (flutter/flutter#132086)
2023-08-08 [email protected] Revert "Replace TextField.canRequestFocus with TextField.focusNode.canRequestFocus" (flutter/flutter#132104)
2023-08-08 [email protected] [Impeller] add drawVertices and drawAtlas benchmarks. (flutter/flutter#132080)
2023-08-07 [email protected] Adds more documentations around ignoreSemantics deprecations. (flutter/flutter#131287)
2023-08-07 [email protected] [web] New HtmlElementView.fromTagName constructor (flutter/flutter#130513)
2023-08-07 [email protected] Move mock canvas to flutter_test (flutter/flutter#131631)
2023-08-07 [email protected] Add static_path_tessellation macrobenchmark (flutter/flutter#131837)
2023-08-07 [email protected] [web] Remove usage of `ui.webOnlyInitializePlatform()` (flutter/flutter#131344)
2023-08-07 [email protected] Roll Flutter Engine from 39a575f65d50 to be085f6699b6 (1 revision) (flutter/flutter#132069)
2023-08-07 [email protected] Android context menu theming and visual update (flutter/flutter#131816)
2023-08-07 [email protected] Roll Flutter Engine from 39ce1c097bce to 39a575f65d50 (2 revisions) (flutter/flutter#132064)
2023-08-07 [email protected] CupertinoContextMenu improvement (flutter/flutter#131030)
2023-08-07 [email protected] Roll Flutter Engine from 5b47c0577060 to 39ce1c097bce (3 revisions) (flutter/flutter#132057)
2023-08-07 [email protected] Slider should check `mounted` before start interaction (flutter/flutter#132010)
2023-08-07 [email protected] Roll Packages from ce53da1 to d7ee75a (7 revisions) (flutter/flutter#132058)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants