Closed Bug 1358247 Opened 3 years ago Closed 2 years ago

Crash in mozilla::gfx::VRManagerChild::RecvReplyGamepadVibrateHaptic

Categories

(Core :: WebVR, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- disabled
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: mccr8, Assigned: daoshengmu)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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?
Flags: needinfo?(dmu)
I can't reproduce it locally via HTC Vive. I will followup this crash status.
Flags: needinfo?(dmu)
Assignee: nobody → dmu
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 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
Attachment #8933261 - Flags: review?(kgilbert) → review-
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 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.
Attachment #8933261 - Flags: review?(kgilbert) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c3ea505d841
Reply VR vibrate promises to the individual channels; r=kip
https://hg.mozilla.org/mozilla-central/rev/8c3ea505d841
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Should we consider this for uplift to Beta or can it ride the 59 train?
Flags: needinfo?(dmu)
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,
Flags: needinfo?(dmu)
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]:
Attachment #8933261 - Flags: approval-mozilla-beta?
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
Attachment #8933261 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
See Also: → 1424805
Duplicate of this bug: 1424805
You need to log in before you can comment on or make changes to this bug.