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)
Core
WebVR
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: daoshengmu, Assigned: daoshengmu)
References
Details
Attachments
(2 files)
10.30 KB,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
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•7 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•7 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•7 years ago
|
||
Kip, could you help me confirm this crash is unnormal? I am going to investigate the root cause.
Flags: needinfo?(kgilbert)
Comment 4•7 years ago
|
||
(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 5•7 years ago
|
||
This happens when mCompositableForwarder of TextureChild is not null at https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/gfx/layers/client/TextureClient.cpp#948.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 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•7 years ago
|
Assignee: nobody → dmu
Flags: needinfo?(nical.bugzilla)
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
> 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•7 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•7 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•7 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•7 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•7 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•7 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 18•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/f89ae1cf86fb for https://treeherder.mozilla.org/logviewer.html#?job_id=103931983&repo=autoland
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b98b73c139eb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•