Closed Bug 1366941 Opened 7 years ago Closed 7 years ago

Crash in VRLayerChild::SubmitFrame when the textureClient size is the same with CanvasClient

Categories

(Core :: WebVR, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

Attachments

(2 files)

Crash log:

[GFX1]: Attempt to remove a texture from a CompositableForwarder.

xul.dll!mozilla::gfx::Log<1,mozilla::gfx::CriticalLogger>::WriteLog(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & aString) Line 520	C++
xul.dll!mozilla::gfx::Log<1,mozilla::gfx::CriticalLogger>::Flush() Line 283	C++
 	xul.dll!mozilla::gfx::Log<1,mozilla::gfx::CriticalLogger>::~Log<1,mozilla::gfx::CriticalLogger>() Line 274	C++
 	xul.dll!mozilla::layers::TextureClient::InitIPDLActor(mozilla::layers::KnowsCompositor * aForwarder) Line 953	C++
 	xul.dll!mozilla::gfx::VRLayerChild::SubmitFrame() Line 73	C++
Reproduce steps:
1. Apply this patch
2. Go to about:config, and make(dom.vr.puppet.enabled,true),(dom.vr.require-gesture,false),(dom.vr.test.enabled,true)
3. Try to mark and unmark "setStatus('onAnimationFrame done.');" at testcrash.html, we can find it will happen crash when "setStatus('onAnimationFrame done.');" is marked.

I have confirmed HTC Vive has the same phenomenon.
(In reply to Daosheng Mu[:daoshengmu] from comment #1)
> Created attachment 8870264 [details] [diff] [review]
> 0001-Reproduce-test-case.patch
> 
> Reproduce steps:
> 1. Apply this patch
> 2. Go to about:config, and
> make(dom.vr.puppet.enabled,true),(dom.vr.require-gesture,false),(dom.vr.test.
> enabled,true)
> 3. Try to mark and unmark "setStatus('onAnimationFrame done.');" at
> testcrash.html, we can find it will happen crash when
> "setStatus('onAnimationFrame done.');" is marked.
> 
> I have confirmed HTC Vive has the same phenomenon.

Please run testcrash.html with a local HTTP server at Nightly Debug version.
Kip, could you help me confirm this crash is unnormal? I am going to investigate the root cause.
Flags: needinfo?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #3)
> Kip, could you help me confirm this crash is unnormal? I am going to
> investigate the root cause.
Copying from our Slack conversation: If there was just a canvas element and no other elements, WebVR should still be able to create a VR layer with it and submit frames.  We should ensure that this case works without crashing.
Flags: needinfo?(kgilbert)
For the use case of WebVR, we need to share the TextureForwarder from CanvasClient and will create a new PTextureChild to VRManagerChild (https://dxr.mozilla.org/mozilla-central/rev/f81bcc23d37d7bec48f08b19a9327e93c54d37b5/gfx/vr/ipc/VRLayerChild.cpp#73).

This crash is because we meet CanvasClientSharedSurface::Updated(), and it is using the TextureClient that is created by VRManagerChild. So, it hits the crash for this mCompositableForwarder is not null and is shared by other IPDL. In the general case, this kind of TextureClient should only be used by PVRManager.
Assignee: nobody → dmu
Flags: needinfo?(nical.bugzilla)
> This crash is because we meet CanvasClientSharedSurface::Updated()

Do you know why we go through this code if we are not expecting to? If running CanvasClientSharedSurface::Updated is the source of the issue I'd prefer to make sure we are fixing it rather than working around it.
Flags: needinfo?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #10)
> > This crash is because we meet CanvasClientSharedSurface::Updated()
> 
> Do you know why we go through this code if we are not expecting to? If
> running CanvasClientSharedSurface::Updated is the source of the issue I'd
> prefer to make sure we are fixing it rather than working around it.

After some investigation, I realize the root cause is our recycle mechanism of SurfaceFactory. When we start to do VR presentation at WebGLContext::StartVRPresentation(). The factory is changed to VRManagerChild. Then, we will push used textureClient into mRecycleFreePool but didn't separate them from different factories (VRManagerChild, CompositorForwarder). So, when asking for NewTexClient, we have chance to let CanvasClientSharedSurface use the wrong textureClient that is created by VRManagerChild channel and that is shared by VRLayerChild::SubmitFrame() as well.

The best way to fix it is checking LayersIPCChannel besides the texture size at SurfaceFactory::NewTexClient().
See Also: → 1326376
Summary: Crash in VRLayerChild::SubmitFrame when no other div element is displayed. → Crash in VRLayerChild::SubmitFrame when the textureClient size is the same with CanvasClient
This bug only happens on Nightly debug.

Bug 1326376 has a problem on VRLayerChild::SubmitFrame as well when we change the dimension of WebGLContext. It will clone the sharedSurface and blit frame, then, it causes the blitting fail assertion in Nightly debug.
Comment on attachment 8870762 [details]
Bug 1366941 - Checking LayersIPCChannel type when using recycled textureClient;

https://reviewboard.mozilla.org/r/142262/#review148782

::: gfx/gl/SharedSurface.cpp:321
(Diff revision 4)
>      // fail, call StopRecycling(), then return here and call it again.
>      mRecycleFreePool.clear();
>  }
>  
>  already_AddRefed<layers::SharedSurfaceTextureClient>
> -SurfaceFactory::NewTexClient(const gfx::IntSize& size)
> +SurfaceFactory::NewTexClient(const gfx::IntSize& size, const layers::LayersIPCChannel* aLayer)

nit: aLayer is a bit misleading because Layer is very specific and used all over the place, maybe "aLayersChannel" instead?
Attachment #8870762 - Flags: review?(nical.bugzilla) → review+
(In reply to Daosheng Mu[:daoshengmu] from comment #15)
> Comment on attachment 8870762 [details]
> Bug 1366941 - Checking LayersIPCChannel type when using recycled
> textureClient;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/142262/diff/4-5/

Thanks. Following the comment 14 to fix nits.
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca6881983573
Checking LayersIPCChannel type when using recycled textureClient; r=nical
(In reply to Daosheng Mu[:daoshengmu] from comment #19)
> Comment on attachment 8870762 [details]
> Bug 1366941 - Checking LayersIPCChannel type when using recycled
> textureClient;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/142262/diff/5-6/

Fix the build fail on Linux.
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b98b73c139eb
Checking LayersIPCChannel type when using recycled textureClient; r=nical
https://hg.mozilla.org/mozilla-central/rev/b98b73c139eb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: