Closed Bug 1261320 Opened 4 years ago Closed 4 years ago

crash in mozilla::webgl::TexUnpackSurface::ConvertSurface

Categories

(Core :: Canvas: WebGL, defect, critical)

45 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
firefox-esr45 47+ fixed

People

(Reporter: philipp, Assigned: pchang)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

[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.
Assignee: nobody → jgilbert
Peter, anybody with WebGL experience that could take a look?
Assignee: jgilbert → howareyou322
[Tracking Requested - why for this release]:
newly introduced crash in 45
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 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.
Attachment #8739907 - Flags: review+
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/
Attachment #8739907 - Attachment description: MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r?jgilbert → MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan
Attachment #8739907 - Flags: review?(jgilbert)
https://hg.mozilla.org/mozilla-central/rev/bd8532d1f56b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Milan, could you fill the uplift request to aurora and esr at least? The volume is quite high. Thanks
Flags: needinfo?(milan)
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
Flags: needinfo?(milan)
Attachment #8739907 - Flags: approval-mozilla-esr45?
Attachment #8739907 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(sledru)
Attachment #8739907 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8739907 [details]
MozReview Request: Bug 1261320 - Check DataSurface is vaild before using, r=milan

Taking it to improve esr45 stability!
Flags: needinfo?(sledru)
Attachment #8739907 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.