1371228 - VRLayerParent should be destroyed when SteamVR runtime is terminated

Status: RESOLVED FIXED

(Core :: WebVR, defect)

Description

[Daosheng Mu[:daoshengmu]]

As Bug 1321275 comment 16 mentioned, the crash happens on SteamVR runtime is turned off, but we still try to submit frames via VRLayers. We should think about make VRLayerParent notify VRLayerChild the VR presentation has been terminated and stop doing IPC transition.

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

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

Comment 2

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

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

Comment 3

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #2)
> Comment on attachment 8875672 [details]
> Bug 1371228 - (wip) handling SteamVR process quit events while sumbitting
> frames;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/147098/diff/1-2/

This version is much better. I check the connection state is true, otherwise, it will help us switch back to the non-present mode, but it can't try to reload the OpenVR runtime. Sometimes, it still has crash that seems because we use async IPC instead of sync IPC to getting mIsConnected from the frame data.

Comment 4

[Daosheng Mu[:daoshengmu]]

Besides, when the SteamVR process is quit, it also happens creating PTextureChild fail at https://dxr.mozilla.org/mozilla-central/rev/da66c4a05fda49d457d9411a7092fed87cf9e53a/gfx/layers/client/TextureClient.cpp#981

Comment 5

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

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

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

https://reviewboard.mozilla.org/r/147098/#review156596

::: gfx/vr/ipc/VRLayerChild.cpp:94
(Diff revision 3)
>  }
>  
> -mozilla::ipc::IPCResult
> -VRLayerChild::Recv__delete__()
> +void
> +VRLayerChild::ActorDestroy(ActorDestroyReason aWhy)
>  {
>    mIPCOpen = false;

I move mIPCOpen assignment back to ActorDestroy(). It should be more normal case.

Comment 7

[:kip (Kearwood Gilbert)]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

https://reviewboard.mozilla.org/r/147098/#review156844

This looks good to me.

If we are still having crashes due to the async IPC, please file a separate bug to handle that case.

Comment 8

[:kip (Kearwood Gilbert)]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

https://reviewboard.mozilla.org/r/147098/#review156848

::: gfx/vr/gfxVROpenVR.cpp:176
(Diff revision 3)
>  {
>    mVRSystem->ResetSeatedZeroPose();
>    UpdateStageParameters();
>  }
>  
> +bool

We could probably add this in VRDisplayHost, but that can be done later

Comment 9

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147098/diff/3-4/

Comment 10

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #9)
> Comment on attachment 8875672 [details]
> Bug 1371228 - Handling SteamVR process quit events to avoid crashes;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/147098/diff/3-4/

Follow the comment 8, move GetIsConnected() to VRDisplayHost.

Comment 11

[Daosheng Mu[:daoshengmu]]

This fix is not risky and should be uplifted to FF 55 for solving crash bugs.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9d121ad0389
Handling SteamVR process quit events to avoid crashes; r=kip

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9d121ad0389

Comment 14

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

Approval Request Comment
[Feature/Bug causing the regression]: This is going to solve crashes.
[User impact if declined]: Crashes will continue to happen.
[Is this code covered by automated tests?]: it relies on hardware. I have tested manually a lot of times.  
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: I have done it 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 fixes crashes, it brings more stable.
[String changes made/needed]: nope.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8875672 [details]
Bug 1371228 - Handling SteamVR process quit events to avoid crashes;

vr stability fix, beta55+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8678aff96ccb