1261320 - crash in mozilla::webgl::TexUnpackSurface::ConvertSurface

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:
newly introduced crash in 45

This bug was filed from the Socorro interface and is 
report bp-815bd339-2ee1-421a-b082-307002160319.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::webgl::TexUnpackSurface::ConvertSurface(mozilla::WebGLContext*, mozilla::webgl::DriverUnpackInfo const*, mozilla::gfx::DataSourceSurface*, bool, mozilla::UniqueBuffer* const, unsigned char* const, bool* const) 	dom/canvas/TexUnpackBlob.cpp
1 	xul.dll 	mozilla::webgl::TexUnpackSurface::TexOrSubImage(bool, bool, char const*, mozilla::WebGLTexture*, StrongGLenum<TexImageTargetDetails>, int, mozilla::webgl::DriverUnpackInfo const*, int, int, int, unsigned int* const) 	dom/canvas/TexUnpackBlob.cpp
2 	xul.dll 	mozilla::WebGLTexture::TexImage(char const*, StrongGLenum<TexImageTargetDetails>, int, unsigned int, int, unsigned int, unsigned int, mozilla::webgl::TexUnpackBlob*) 	dom/canvas/WebGLTextureUpload.cpp
3 	xul.dll 	mozilla::WebGLTexture::TexOrSubImage(bool, char const*, StrongGLenum<TexImageTargetDetails>, int, unsigned int, int, int, int, int, unsigned int, unsigned int, mozilla::webgl::TexUnpackBlob*) 	dom/canvas/WebGLTextureUpload.cpp
4 	xul.dll 	nsExpirationTracker<mozilla::ImageCacheEntryData, 4>::nsExpirationTracker<mozilla::ImageCacheEntryData, 4>(unsigned int, char const*) 	xpcom/ds/nsExpirationTracker.h
5 	xul.dll 	mozilla::dom::WebGLRenderingContextBinding::texImage2D 	obj-firefox/dom/bindings/WebGLRenderingContextBinding.cpp
6 	xul.dll 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp
7 		@0x7ef8fa8 	
8 		@0xffffff80

this is a new crash signature in firefox 45 builds and upwards which has apparently been introduced by the work in bug 1221822.
it is mid- to low-level in volume - on 45.0.1 it is #86 & making up 0.15% of all crashes.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Peter, anybody with WebGL experience that could take a look?

Comment 2

[[:philipp]]

[Tracking Requested - why for this release]:
newly introduced crash in 45

Comment 3

[Peter Chang[:pchang]]

The following gfxCriticalError in crash report is generated from[1] and return the DataSurface with nullptr.
We only assert this DataSurface[2] in debug build and got crash in release build.

 [14][GFX1]: Failed to create software bitmap: Size(2048,2048) Code: 0x8007000e.

[1]https://dxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceD2D1.cpp#58
[2]https://dxr.mozilla.org/mozilla-central/source/dom/canvas/TexUnpackBlob.cpp#758

Comment 4

[Peter Chang[:pchang]]

Attachment: MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan

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

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8739907 [details]
MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan

https://reviewboard.mozilla.org/r/45433/#review42005

::: dom/canvas/TexUnpackBlob.cpp:759
(Diff revision 1)
>      // MakeCurrent is a big mess in here, because mapping (and presumably unmapping) on
>      // OSX can lose our MakeCurrent. Therefore it's easiest to MakeCurrent just before we
>      // call into GL, instead of trying to keep MakeCurrent-ed.
>  
>      RefPtr<gfx::DataSourceSurface> dataSurf = mSurf->GetDataSurface();
>      MOZ_ASSERT(dataSurf);

I'd remove this assert, since GetDataSurface() can return nullptr, and we handle it.

Comment 6

[Peter Chang[:pchang]]

Comment on attachment 8739907 [details]
MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd8532d1f56b

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Milan, could you fill the uplift request to aurora and esr at least? The volume is quite high. Thanks

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8739907 [details]
MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan

[Approval Request Comment]
[Feature/regressing bug #]: 1221822
If this is not a sec:{high,crit} bug, please state case for ESR consideration: High volume of crashes (see previous comment.)
User impact if declined: Crashes, high volume.
Fix Landed on Version: 48
Risk to taking this patch (and alternatives if risky): This is a null pointer check, the risk should be minimal.
String or UUID changes made by this patch: n/a

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8739907 [details]
MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan

Crash fix, Aurora47+. Sylvestre, fyi for esr45 uplift.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6a267e0ce6c08f7af8393d522b92c2a2a4ba14e

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8739907 [details]
MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan

Taking it to improve esr45 stability!

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/f1be5b6a974e