Bug 279094 – [macOS] Send playEffect() twice to Gamepad.actuator and it cannot stop with reset()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

When sending playEffect() to Gamepad.actuator more than once, the reset() cannot stop the gamepad from vibrating.

[Basuke Suzuki]

rdar://126589062

[Basuke Suzuki]

commit a63922a80e01fb65438cf6b77c9323e974bb9c4d
Author: Basuke Suzuki <[e-mail]>
Date:   Tue Sep 3 17:22:13 2024 -0700

    It worked. But not perfect

diff --git a/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.h b/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.h
index 0f89fd968954..ee6aa4de5601 100644
--- a/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.h
+++ b/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.h
@@ -81,6 +81,8 @@ private:
     bool m_failedToStartRightTriggerEngine { false };
     std::unique_ptr<GameControllerHapticEffect> m_currentDualRumbleEffect;
     std::unique_ptr<GameControllerHapticEffect> m_currentTriggerRumbleEffect;
+    bool m_preventDualRumbleFinishNotification { false };
+    bool m_preventTriggerRumbleFinishNotification { false };
 };
 
 } // namespace WebCore
diff --git a/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.mm b/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.mm
index 435a62e6949a..b878619b6e64 100644
--- a/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.mm
+++ b/Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.mm
@@ -85,8 +85,17 @@ void GameControllerHapticEngines::playEffect(GamepadHapticEffectType type, const
     if (!newEffect)
         return completionHandler(false);
 
-    if (currentEffect)
+    if (currentEffect) {
+        switch (type) {
+        case GamepadHapticEffectType::DualRumble:
+            m_preventDualRumbleFinishNotification = true;
+            break;
+        case GamepadHapticEffectType::TriggerRumble:
+            m_preventTriggerRumbleFinishNotification = true;
+            break;
+        }
         currentEffect->stop();
+    }
 
     currentEffect = WTFMove(newEffect);
     ensureStarted(type, [weakThis = WeakPtr { *this }, type, effect = WeakPtr { *currentEffect }, completionHandler = WTFMove(completionHandler)](bool success) mutable {
@@ -135,9 +144,20 @@ void GameControllerHapticEngines::ensureStarted(GamepadHapticEffectType effectTy
         }
         completionHandler(success);
     });
-    auto startEngine = [weakThis = WeakPtr { *this }](CHHapticEngine *engine, CompletionHandler<void(bool)>&& completionHandler, std::function<void()>&& playersFinished) mutable {
-        [engine startWithCompletionHandler:makeBlockPtr([weakThis = WTFMove(weakThis), engine, completionHandler = WTFMove(completionHandler), playersFinished](NSError* error) mutable {
-            ensureOnMainRunLoop([completionHandler = WTFMove(completionHandler), success = !error]() mutable {
+    auto startEngine = [weakThis = WeakPtr { *this }, effectType](CHHapticEngine *engine, CompletionHandler<void(bool)>&& completionHandler, std::function<void()>&& playersFinished) mutable {
+        [engine startWithCompletionHandler:makeBlockPtr([weakThis = WTFMove(weakThis), engine, effectType, completionHandler = WTFMove(completionHandler), playersFinished](NSError* error) mutable {
+            ensureOnMainRunLoop([completionHandler = WTFMove(completionHandler), effectType, weakThis = WTFMove(weakThis), success = !error]() mutable {
+                if (weakThis) {
+                    switch (effectType) {
+                    case GamepadHapticEffectType::DualRumble:
+                        weakThis->m_preventDualRumbleFinishNotification = false;
+                        break;
+                    case GamepadHapticEffectType::TriggerRumble:
+                        weakThis->m_preventTriggerRumbleFinishNotification = false;
+                        break;
+                    }
+                }
+
                 completionHandler(success);
             });
             if (error)
@@ -156,14 +176,14 @@ void GameControllerHapticEngines::ensureStarted(GamepadHapticEffectType effectTy
             if (weakThis)
                 weakThis->m_failedToStartLeftHandleEngine = !success;
         }, [weakThis = WeakPtr { *this }] {
-            if (weakThis && weakThis->m_currentDualRumbleEffect)
+            if (weakThis && weakThis->m_currentDualRumbleEffect && !weakThis->m_preventDualRumbleFinishNotification)
                 weakThis->m_currentDualRumbleEffect->leftEffectFinishedPlaying();
         });
         startEngine(m_rightHandleEngine.get(), [weakThis = WeakPtr { *this }, callbackAggregator](bool success) {
             if (weakThis)
                 weakThis->m_failedToStartRightHandleEngine = !success;
         }, [weakThis = WeakPtr { *this }] {
-            if (weakThis && weakThis->m_currentDualRumbleEffect)
+            if (weakThis && weakThis->m_currentDualRumbleEffect && !weakThis->m_preventDualRumbleFinishNotification)
                 weakThis->m_currentDualRumbleEffect->rightEffectFinishedPlaying();
         });
         break;
@@ -172,14 +192,14 @@ void GameControllerHapticEngines::ensureStarted(GamepadHapticEffectType effectTy
             if (weakThis)
                 weakThis->m_failedToStartLeftTriggerEngine = !success;
         }, [weakThis = WeakPtr { *this }] {
-            if (weakThis && weakThis->m_currentTriggerRumbleEffect)
+            if (weakThis && weakThis->m_currentTriggerRumbleEffect && !weakThis->m_preventTriggerRumbleFinishNotification)
                 weakThis->m_currentTriggerRumbleEffect->leftEffectFinishedPlaying();
         });
         startEngine(m_rightTriggerEngine.get(), [weakThis = WeakPtr { *this }, callbackAggregator](bool success) {
             if (weakThis)
                 weakThis->m_failedToStartRightTriggerEngine = !success;
         }, [weakThis = WeakPtr { *this }] {
-            if (weakThis && weakThis->m_currentTriggerRumbleEffect)
+            if (weakThis && weakThis->m_currentTriggerRumbleEffect && !weakThis->m_preventTriggerRumbleFinishNotification)
                 weakThis->m_currentTriggerRumbleEffect->rightEffectFinishedPlaying();
         });
         break;

[Basuke Suzuki]

Adding flag not to invoke handler just worked most of the time.

The root cause of the issue is requesting a completionHandler invocation to wrong effect object. Say two sequential playEffect requests is coming.

1. The effect object of request 1 is stored in m_currentDualRumbleEffect and referenced by currentEffect in playEffect() method.
2. In second playEffect(), currentEffect is stopped  before start playing second one. This is in main thread.
3. Notification for stopPlaying is called for first request {*) in CoreHaptics thread.
4. currentEffect is replaced with new effect.
5. Notification arrived in main thread and try to invoke completionHandler targeting to second request. This is wrong because the notification is for first request.

The above patch added a flag to prevent invocation of completionHandler during the playEffect setup.

To fix this correctly, the notification should be handled in Effect object.

[Basuke Suzuki]

Pull request: https://github.com/WebKit/WebKit/pull/33124

[EWS]

Committed 286251@main (467d7dd6f1ea): <https://commits.webkit.org/286251@main>

Reviewed commits have been landed. Closing PR #33124 and removing active labels.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 282731

[Basuke Suzuki]

Pull request: https://github.com/WebKit/WebKit/pull/36328

[EWS]

Committed 286286@main (0a7a483a4ef3): <https://commits.webkit.org/286286@main>

Reviewed commits have been landed. Closing PR #36328 and removing active labels.