AbstractHapticGamepad implementation may crash
Reported by
artyo...@gmail.com,
Dec 27
|
||||
Issue descriptionSteps to reproduce the problem: Unfortunately, I can't repro it with original code base, due to lack of haptic support for VR controllers (see details below). What is the expected behavior? Shouldn't crash. What went wrong? The current implementation of AbstractHapticGamepad implies that it may crash in several cases. If you look into device/gamepad/abstract_haptic_gamepad.cc, methods StartVibration and PlayDualRumbleEffect: these methods post delayed tasks with using Unretained(this). Now imaging that the duration of the effect is some significant value (2 seconds) and that AbstractHapticGamepad instance dies during these two seconds: that will cause an crash when the task got finally executed with dead 'this'. The reason why the AbstractHapticGamepad can be destroyed is simple: the corresponding gamepad data fetcher may got destroyed, for example, when it is representing a VR controller and when user exits VR. AFAIK, you don't have haptics implemented for VR gamepads (yet?), but this is why I hit that issue: during implementation of haptic support for mobile Oculus controllers. In this case, gamepad data fetcher / AbstractHapticGamepad instance got created when a user enters VR and got destroyed when user exits VR. The solution is to use a weak ptr factory (GetWeakPtr()) here instead of Uretained(this). Another crash may occur in third_party/blink/renderer/modules/gamepad/gamepad_haptic_actuator.cc, OnPlayEffectCompleted: again, this method may be executed when GetExecutionContext() is alread null, for example, during the navigation to another page, and therefore it is crashing on GetExecutionContext()->GetTaskRunner(TaskType::kMiscPlatformAPI) call. Just add a nullptr check there. Did this work before? N/A Does this work in other browsers? Yes Chrome version: 73 Channel: n/a OS Version: Flash Version:
,
Dec 28
@ artyom17: Does this issue happen with any particular app? Please provide 16 digit crash id from chrome://crashes. This would help in further triaging of the issue. Could someone from Blink>GamepadAPI team please help in further triaging. Thanks!
,
Jan 3
,
Jan 4
Thanks for reporting and for the clear descriptions, I've uploaded a CL to address both of these: https://chromium-review.googlesource.com/c/chromium/src/+/1396398 > mobile Oculus controllers Are you using the Samsung Gear VR controllers? https://www.amazon.com/Samsung-Gear-VR-Controller-pack/dp/B078XS5MJ5 > the corresponding gamepad data fetcher may got destroyed, for example, when it is representing a VR controller and when user exits VR. Ah, so you're removing the VR data fetcher when the user exits VR? The other fetchers stay registered until the browser closes which is probably why we haven't seen similar crashes for other devices. I'm not sure that it makes sense to hide VR-associated gamepads when the user is not in VR. If the Oculus mobile API supports it, it would be better if we could provide VR gamepad input even when the headset is off. > AFAIK, you don't have haptics implemented for VR gamepads (yet?) No, I've avoided VR gamepads for the moment since WebXR was going to support them. It looks like WebXR is still planning to handle VR gamepad button/axis/pose inputs but I don't see any indication that haptics will be supported in v1. Regardless of what WebXR does, I think it would be good to support VR gamepads through the Gamepad API if possible. Patches welcome!
,
Jan 7
I am talking about Oculus Touch controllers for upcoming Oculus Quest, those have haptics (unlike the controller for Oculus Go). I am removing the data fetcher simply because it makes no sense to have it without active VR session (at least for Oculus Mobile SDK). AFAIK, you do the same for the Daydream controller, don't you? Or, VrShell class is not destroyed when exiting from VR session for you? As to WebXR: it is currently not clear what will be the level of support for input, and I am almost certain that the first version of WebXR spec won't have any haptics support. At least we, in Oculus, going to continue to support Gamepad API Extensions till WebXR spec matches Gamepad API in terms of functionality. Thanks for uploading the CL with the fix, BTW!
,
Jan 9
> I am talking about Oculus Touch controllers for upcoming Oculus Quest, those have haptics (unlike the controller for Oculus Go). Interesting! Touch has LRAs which don't behave like traditional "dual-rumble" style rumble motors. Do you think we should consider adding a new effect type for Touch, or is it already well-supported by dual-rumble effects? > I am removing the data fetcher simply because it makes no sense to have it without active VR session (at least for Oculus Mobile SDK). That seems reasonable, especially for an all-in-one device like Quest that isn't usable without the headset. For desktop and other mobile platforms I think we should still try to support VR controllers without a headset where possible. If I understand correctly, Touch controllers can't be used without the headset so it may not be possible to support them without an active VR session. > AFAIK, you do the same for the Daydream controller, don't you? Or, VrShell class is not destroyed when exiting from VR session for you? I don't have any VR hardware on my desk to test with, but it does look like GvrGamepadDataFetcher is meant to be unregistered when VrShell is destroyed. https://cs.chromium.org/chromium/src/device/vr/android/gvr/gvr_gamepad_data_provider.h?l=36 IMO this is a mistake. The Daydream controller is a BLE device and doesn't rely on a headset, so it should be usable through the Gamepad API without a VR session.
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b63e01b1a909e277794821eb13ac585da1cfdaa commit 4b63e01b1a909e277794821eb13ac585da1cfdaa Author: Matt Reynolds <mattreynolds@google.com> Date: Thu Jan 10 19:03:15 2019 Fix gamepad haptics crashes Fixes two crash bugs related to gamepad haptics discovered while implementing haptics for VR devices. In //device/gamepad, AbstractHapticGamepad posts delayed tasks that may complete after the object has been destroyed. To fix, the unretained references are replaced with weak pointers. Tasks that complete after the object is destroyed are ignored. This may allow some vibration to continue after a device is unexpectedly removed, but for most devices it will not last longer than the maximum effect duration of 5 seconds. In the renderer, GamepadHapticActuator posts a delayed task to stop vibration after Promise callbacks are complete. This may crash if the execution context is already null. To fix, stop vibration synchronously when the context is null since there cannot be any pending callbacks. BUG= 918050 Change-Id: I17d80b2f914506d30c48acce156be4819cf1526a Reviewed-on: https://chromium-review.googlesource.com/c/1396398 Commit-Queue: Matt Reynolds <mattreynolds@chromium.org> Reviewed-by: Ovidio Henriquez <odejesush@chromium.org> Cr-Commit-Position: refs/heads/master@{#621678} [modify] https://crrev.com/4b63e01b1a909e277794821eb13ac585da1cfdaa/device/gamepad/abstract_haptic_gamepad.cc [modify] https://crrev.com/4b63e01b1a909e277794821eb13ac585da1cfdaa/device/gamepad/abstract_haptic_gamepad.h [modify] https://crrev.com/4b63e01b1a909e277794821eb13ac585da1cfdaa/third_party/blink/renderer/modules/gamepad/gamepad_haptic_actuator.cc
,
Jan 10
|
||||
►
Sign in to add a comment |
||||
Comment 1 by chelamcherla@chromium.org
, Dec 28