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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: daoshengmu, Assigned: daoshengmu)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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++
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
(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.
(Assignee)

Comment 3

2 years ago
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)
(Assignee)

Comment 7

2 years ago
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.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → dmu
Flags: needinfo?(nical.bugzilla)
Comment hidden (mozreview-request)
> 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)
(Assignee)

Comment 11

2 years ago
(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().
(Assignee)

Updated

2 years ago
See Also: → 1326376
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 12

2 years ago
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 hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
(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.

Comment 17

2 years ago
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca6881983573
Checking LayersIPCChannel type when using recycled textureClient; r=nical
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
(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.

Comment 21

2 years ago
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b98b73c139eb
Checking LayersIPCChannel type when using recycled textureClient; r=nical

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b98b73c139eb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.