1358247 - Crash in mozilla::gfx::VRManagerChild::RecvReplyGamepadVibrateHaptic

Status: RESOLVED FIXED

(Core :: WebVR, defect, critical)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-aac62a34-787f-48b3-8085-eec200170420.
=============================================================

Only 4 crashes, and all from a single installation.

It is crashing on MOZ_CRASH("We should always have a promise.");

Any ideas, Daosheng?

Comment 1

[Daosheng Mu[:daoshengmu]]

I can't reproduce it locally via HTC Vive. I will followup this crash status.

Comment 2

[Daosheng Mu[:daoshengmu]]

This is because we have several content processes, and we remove the promise duplicately. We should be according to the PVRManager to remove promise id at the parent side. (https://dxr.mozilla.org/mozilla-central/rev/c2248f85346939d3e0b01f57276c440ccb2d16a1/gfx/vr/VRManager.cpp#485)

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1358247 - Reply VR vibrate promises to the individual channels;

Review commit: https://reviewboard.mozilla.org/r/204202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204202/

Comment 4

[:kip (Kearwood Gilbert)]

Comment on attachment 8933261 [details]
Bug 1358247 - Reply VR vibrate promises to the individual channels;

https://reviewboard.mozilla.org/r/204202/#review209948

This is looking very good, but could you please double-check if the lifetime of the VRManagerPromise will be long enough for the Runnable's to execute asyncrhonously?

::: gfx/vr/gfxVROculus.cpp:1404
(Diff revision 1)
>  
>    if (remainingTime) {
>      MOZ_ASSERT(mVibrateThread);
>  
>      RefPtr<Runnable> runnable =
> -      NewRunnableMethod<ovrSession, uint32_t, double, double, uint64_t, uint32_t>(
> +      NewRunnableMethod<ovrSession, uint32_t, double, double, uint64_t, VRManagerPromise>(

Is it possible that the passed aPromise could be deallocated before the VRControllerOculus::UpdateVibrateHaptic Runnable executes?

ie. Could the temporary in VRManagerParent::RecvVibrateHaptic be deallocated by ending the function before the Runnable executes?

Should it be copied or refcounted here to avoid this?

::: gfx/vr/gfxVROpenVR.h:141
(Diff revision 1)
>    virtual void RemoveControllers() override;
>    virtual void VibrateHaptic(uint32_t aControllerIdx,
>                               uint32_t aHapticIndex,
>                               double aIntensity,
>                               double aDuration,
> -                             uint32_t aPromiseID) override;
> +                             const VRManagerPromise& aPromiseID) override;

Nit: aPromiseID => aPromise

Comment 5

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8933261 [details]
Bug 1358247 - Reply VR vibrate promises to the individual channels;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204202/diff/1-2/

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8933261 [details]
Bug 1358247 - Reply VR vibrate promises to the individual channels;

https://reviewboard.mozilla.org/r/204202/#review209948

> Is it possible that the passed aPromise could be deallocated before the VRControllerOculus::UpdateVibrateHaptic Runnable executes?
> 
> ie. Could the temporary in VRManagerParent::RecvVibrateHaptic be deallocated by ending the function before the Runnable executes?
> 
> Should it be copied or refcounted here to avoid this?

I have confirmed it is a copy already, but we still can use StoreCopyPassByConstLRef<T> to wrap it to make it at the compiling time checking.

Comment 7

[:kip (Kearwood Gilbert)]

Comment on attachment 8933261 [details]
Bug 1358247 - Reply VR vibrate promises to the individual channels;

https://reviewboard.mozilla.org/r/204202/#review210304

Daosheng has confirmed that there will be no lifetime issues with the asynchronous runnables due to object copies.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c3ea505d841
Reply VR vibrate promises to the individual channels; r=kip

Comment 9

[Bogdan Tara[:bogdan_tara]]

https://hg.mozilla.org/mozilla-central/rev/8c3ea505d841

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for uplift to Beta or can it ride the 59 train?

Comment 11

[Daosheng Mu[:daoshengmu]]

I am thinking to uplift it to Beta, but I would like to wait for a few days to confirm it does not happen again,

Comment 12

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8933261 [details]
Bug 1358247 - Reply VR vibrate promises to the individual channels;

Approval Request Comment
[Feature/Bug causing the regression]: Receiving promise from the wrong IPC channels, and it hits MOZ_CRASH().
[User impact if declined]: It would cause crashes when users use VR controller vibration across multiple content processes.
[Is this code covered by automated tests?]: nope. It needs a specific hardware to test it manually.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: nope. I have tested by myself.
[List of other uplifts needed for the feature/fix]: nope.
[Is the change risky?]: nope.
[Why is the change risky/not risky?]: it only affects the users who wanna use VR vibration, and it fixes the crash.
[String changes made/needed]:

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8933261 [details]
Bug 1358247 - Reply VR vibrate promises to the individual channels;

very low volume crash, I'd rather let this ride 59