1691774 - Crash in [@ memcpy | mozilla::ClientWebGLContext::DoReadPixels]

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[Pascal Chevrel:pascalc]

Crash report: https://crash-stats.mozilla.org/report/index/400c6455-8e7b-45d2-93ca-445f40210209

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 vcruntime140.dll memcpy d:\agent\_work\3\s\src\vctools\crt\vcruntime\src\string\i386\memcpy.asm:596
1 xul.dll mozilla::ClientWebGLContext::DoReadPixels const dom/canvas/ClientWebGLContext.cpp:4654
2 xul.dll mozilla::ClientWebGLContext::BackBufferSnapshot dom/canvas/ClientWebGLContext.cpp:1037
3 xul.dll mozilla::ClientWebGLContext::GetSurfaceSnapshot dom/canvas/ClientWebGLContext.cpp:863
4 xul.dll mozilla::dom::HTMLCanvasElement::GetSurfaceSnapshot dom/html/HTMLCanvasElement.cpp:1269
5 xul.dll static nsLayoutUtils::SurfaceFromElement layout/base/nsLayoutUtils.cpp:7007
6 xul.dll static nsLayoutUtils::SurfaceFromElement layout/base/nsLayoutUtils.cpp:7100
7 xul.dll mozilla::dom::CanvasRenderingContext2D::DrawImage dom/canvas/CanvasRenderingContext2D.cpp:4517
8 xul.dll mozilla::dom::CanvasRenderingContext2D_Binding::drawImage dom/bindings/CanvasRenderingContext2DBinding.cpp:2717
9 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3231

It looks like a rather new signature since 85 beta 9 so maybe a signature change.

Comment 1

[Pascal Chevrel:pascalc]

Jeff, could we get this bug triaged and assigned if necessary? Thanks!

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Jim, this has been a pretty high-volume crash for the last few releases now. Is there anything we can do to bump the priority?

Comment 3

[Kelsey Gilbert [:jgilbert]]

Sure I can bump this.

Comment 4

[Chris Peterson [:cpeterson]]

99% of these crash reports are from 32-bit Windows 7, 8.1, or 10.

Comment 5

[Chris Peterson [:cpeterson]]

This signature jumped from just a few crashes per day to about 150 per day around February 23–26. Firefox 86 was released on February 23, so perhaps this is a WebGL regression in 86? There were 2775 crashes from Release 86 but zero from Release 85.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

We never check if we successfully map in the shmem:

https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/dom/canvas/ClientWebGLContext.cpp#4770

If we didn't the range will be nullptr / zero bytes, and hence the crash when we first deref it:

https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/dom/canvas/ClientWebGLContext.cpp#4798

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Probably this is a virtual memory failure given the vast majority of the crashes are x86. So the shmem was successfully created on the other side, but we couldn't actually map it into the content process.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1691774 - Check shmem allocation/mapping failures in ClientWebGLContext.

We use webgl::RaiiShmem in a few places to read in pixel buffers from a
call to the compositor process. Shmems might fail to be mapped into our
process, probably due to virtual memory constraints, and we should check
for that condition.

Depends on D136355

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Hi Andrew, is this close to landing?

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

This is very much related to bug 1681861 so I am adding the signature here and duping it against this bug.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d3e92a1f042
Check shmem allocation/mapping failures in ClientWebGLContext. r=jgilbert

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/8d3e92a1f042

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This will require a bit of rebasing for ESR, but I think it'd be worth it given the volume of the DoReadPixels crashes there. Can you please attach a rebased patch and request approval when you get a chance, Andrew? Thanks!

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 91 ESR patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Users will continue to experience a modest volume crash due to OOMs and WebGL
Fix Landed on Version: 99
Risk to taking this patch (and alternatives if risky): I was unable to successfully do a try push to 91 ESR, so I'm not sure if it builds / passes the WebGL test suite. We should confirm that.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Try push:
https://treeherder.mozilla.org/jobs?repo=try&revision=c5fccb3e87c141d5d5224b21f8c7d8cb78271b62

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Try push:
https://treeherder.mozilla.org/jobs?repo=try&revision=c5fccb3e87c141d5d5224b21f8c7d8cb78271b62

Thanks, that looks good :).

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9268316 [details] [diff] [review]
91 ESR patch

Fixes various WebGL crashes, approved for 91.8esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/fa432093d33e