From 59fc766c6fdfd03d0983fc95ce8b76793a300dd5 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Tue, 5 Aug 2025 17:31:15 -0400 Subject: [PATCH] [web] Fix potential race condition in ClickDebouncer (#173294) Based on Gemini's comment: https://github.com/flutter/flutter/pull/173072#discussion_r2246216031 --- .../lib/src/engine/pointer_binding.dart | 6 +++ .../test/engine/pointer_binding_test.dart | 38 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart index 295f0a09e23..51dbb19484d 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -350,6 +350,12 @@ class ClickDebouncer { /// /// This method is called asynchronously from [_maybeStartDebouncing]. void _doStartDebouncing(DomEvent event, List data) { + // It's possible that debouncing was canceled between the pointerdown event and the execution + // of this method. + if (!isDebouncing) { + return; + } + _state = ( target: event.target!, // The 200ms duration was chosen empirically by testing tapping, mouse diff --git a/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart b/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart index b8860c748d0..023f724e9e8 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/pointer_binding_test.dart @@ -2651,6 +2651,44 @@ void _testClickDebouncer({required PointerBinding Function() getBinding}) { expect(semanticsActions, isEmpty); }); + testWithSemantics('Does not start debouncing if reset before scheduled execution', () async { + expect(EnginePlatformDispatcher.instance.semanticsEnabled, isTrue); + expect(PointerBinding.clickDebouncer.isDebouncing, isFalse); + expect(PointerBinding.clickDebouncer.debugState, isNull); + + final DomElement testElement = createDomElement('flt-semantics'); + testElement.setAttribute('flt-tappable', ''); + view.dom.semanticsHost.appendChild(testElement); + + // 1. Trigger _maybeStartDebouncing, which sets _isDebouncing = true and schedules _doStartDebouncing. + testElement.dispatchEvent(context.primaryDown()); + + // At this point, _isDebouncing is true, but _doStartDebouncing (which sets _state and creates the Timer) + // has not yet executed because it was scheduled with Timer.run(). + expect(PointerBinding.clickDebouncer.isDebouncing, isTrue); + expect(PointerBinding.clickDebouncer.debugState, isNull); // _state is still null + + // 2. Simulate a scenario where reset() is called before _doStartDebouncing gets a chance to run. + // This could happen due to a hot restart or other lifecycle events. + PointerBinding.clickDebouncer.reset(); + + // After reset(), _isDebouncing should be false and _state should still be null. + expect(PointerBinding.clickDebouncer.isDebouncing, isFalse); + expect(PointerBinding.clickDebouncer.debugState, isNull); + + // 3. Allow the scheduled _doStartDebouncing to run. With the fix, it should now check + // `!isDebouncing` and return early. + await nextEventLoop(); + + // Verify that _doStartDebouncing did not proceed to set _state or create a Timer. + expect(PointerBinding.clickDebouncer.isDebouncing, isFalse); + expect(PointerBinding.clickDebouncer.debugState, isNull); + + // Ensure no events were sent to the framework as debouncing was effectively cancelled. + expect(pointerPackets, isEmpty); + expect(semanticsActions, isEmpty); + }); + testWithSemantics('Starts debouncing after event loop', () async { expect(EnginePlatformDispatcher.instance.semanticsEnabled, isTrue); expect(PointerBinding.clickDebouncer.isDebouncing, isFalse);