Crash in mozilla::WebGLFramebuffer::BlitFramebuffer (Webgl2.0 regression)

RESOLVED FIXED in Firefox 51

Status

()

Core
Canvas: WebGL
P3
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: marcot, Assigned: Lenzak)

Tracking

({crash, regression, testcase})

51 Branch
mozilla53
crash, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox49 unaffected, firefox50 unaffected, firefox51+ fixed, firefox52+ fixed, firefox53 fixed)

Details

(Whiteboard: [gfx-noted], crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

- enable webgl 2.0 in Firefox 52
- open the attached repro


Actual results:

tab crashed


Expected results:

renders a scene with some lights. See firefox 49 for expected visuals.
(Reporter)

Comment 1

a year ago
More findings:

1) if webgl.enable-webgl2;false on Firefox52-Windows, lights don't render and I get errors like:
Error: WebGL: drawElements: Feedback loop detected between tex target 0x0de1, tex unit 0, levels 0-0; and framebuffer attachment 0x821a, level 0.

2) If webgl.enable-webgl2;true and webgl.disable-angle;true on Firefox49-Windows: Everything renders fine.

I should also mention that this repro will try to use WEBGL_draw_buffers, so issue #2 probably indicates a bug with WEBGL_draw_buffers and Angle (can repro on Chrome-Windows but not on Edge)

Updated

a year ago
Severity: normal → critical
Component: Untriaged → Canvas: WebGL
Keywords: crash, regression, testcase-wanted
Product: Firefox → Core

Comment 2

a year ago
Could you attach the testcase, please.
Flags: needinfo?(marcot)
(Reporter)

Comment 3

a year ago
It looks like the test case failed to upload because exceeded the size limit. You can download it here: https://oc.unity3d.com/index.php/s/hZnlYroDEspI3zT

Comment 4

a year ago
Thanks for the testcase. 

I can confirm the issue on Windows 10 (Intel(R) HD Graphics) on latest Nightly 52 (Build ID 20161109030210).

I did a regression range and this is what I found:

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25f2e739a961677e6eb1f296ddaf59c46207bcbc&tochange=b32a687a0a3ebd6db5806e69621563a0300fe6ce

Regressed by bug 1300946.

The issue is reproducible also on Aurora 51 (Build ID 20161108004019), probably because of the uplift on that bug. Not sure if Beta is affected.
Blocks: 1300946
Status: UNCONFIRMED → NEW
status-firefox50: --- → ?
status-firefox51: --- → affected
status-firefox52: --- → affected
Ever confirmed: true
Flags: needinfo?(marcot)
Keywords: testcase-wanted → testcase

Comment 5

a year ago
The issue is also reproducible on Mac 10.12.1 and Ubuntu 16.04 (Nightly and Aurora). Beta is not affected.
status-firefox50: ? → unaffected
OS: Unspecified → All
Hardware: Unspecified → All
Version: 52 Branch → Trunk
QA Whiteboard: [qe-webgl2]

Comment 6

a year ago
https://crash-stats.mozilla.com/report/index/9a243261-d658-4137-a9b7-3388b2161110
Crash Signature: [@ mozilla::WebGLFramebuffer::BlitFramebuffer ]
status-firefox49: --- → unaffected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Flags: needinfo?(jgilbert)
Summary: Webgl2.0 regression crash → Crash in mozilla::WebGLFramebuffer::BlitFramebuffer (Webgl2.0 regression)
Version: Trunk → 51 Branch
Tracking 52+ for this reproducible crash.
tracking-firefox52: ? → +
Priority: -- → P3
Whiteboard: [gfx-noted]
Hi Peter,
Can you help find an owner for this?
Flags: needinfo?(howareyou322)
Track 51+ as regression related to WebGL 2.
tracking-firefox51: ? → +
Assertion failure: IsDefined(), at /Users/msreckovic/Repos/mozilla-central/dom/canvas/WebGLFramebuffer.cpp:75
#01: mozilla::WebGLFBAttachPoint::Format() const[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x36728ec]
#02: mozilla::WebGLFramebuffer::BlitFramebuffer(mozilla::WebGLContext*, mozilla::WebGLFramebuffer const*, int, int, int, int, mozilla::WebGLFramebuffer const*, int, int, int, int, unsigned int, unsigned int)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x367a091]
#03: mozilla::WebGL2Context::BlitFramebuffer(int, int, int, int, int, int, int, int, unsigned int, unsigned int)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x360ac0d]
#04: mozilla::dom::WebGL2RenderingContextBinding::blitFramebuffer(JSContext*, JS::Handle<JSObject*>, mozilla::WebGL2Context*, JSJitMethodCallArgs const&)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2f4b215]
...
Michael, please take a look.
Assignee: nobody → cleu
Flags: needinfo?(howareyou322)
(Assignee)

Comment 12

a year ago
I can reproduce it on Mac OSX 10.11, too.

I'll look into this one.
https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLFramebuffer.cpp#1625 :

for (const auto& drawBuffer : dstFB->mColorDrawBuffers) {
    fnCheckColorFormat(drawBuffer->Format()->format);   <---- drawBuffer->IsDefined() fails here
}

I'm guessing the WebGLFramebuffer constructor should not do this:

mColorDrawBuffers.push_back(&mColorAttachments[0]);

when the color attachment isn't defined.
Flags: needinfo?(jgilbert)
Michael, any update?
Flags: needinfo?(howareyou322)
(Assignee)

Comment 15

a year ago
I am still investigating this crash.

Trying to figure out why the draw buffer is undefined here.

We can skip undefined buffers by adding if-checks and the testcase would not crash, but I don't think it is a right solution.
(Assignee)

Comment 16

a year ago
Created attachment 8812681 [details] [diff] [review]
Check colorbuffer is defined or not when constructing WebGLFramebuffer
(Assignee)

Comment 17

a year ago
I thought it must encounter some failure which prevent ColorBuffer from being initialized and defined correctly, but I didn't see any failure about framebuffer manipulation before the crash happens. Maybe the ColorBuffer was not going to be defined throughout the WebGL program execution.

So I think Milan's idea is right, we just simply skip adding those undefined buffers when we create WebGLFramebuffer.
(Assignee)

Updated

a year ago
Attachment #8812681 - Attachment description: WIP - Check colorbuffer is defined or not when constructing WebGLFramebuffer → Check colorbuffer is defined or not when constructing WebGLFramebuffer
Attachment #8812681 - Flags: review?(jgilbert)
Comment on attachment 8812681 [details] [diff] [review]
Check colorbuffer is defined or not when constructing WebGLFramebuffer

Review of attachment 8812681 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLFramebuffer.cpp
@@ +623,5 @@
> +    if (mColorAttachments[0].IsDefined()) {
> +        mColorDrawBuffers.push_back(&mColorAttachments[0]);
> +        mColorReadBuffer = &mColorAttachments[0];
> +    } else {
> +        mColorReadBuffer = nullptr;

mColorReadBuffer makes no assumptions about whether it is defined. It is simply how we track which attachment the ReadBuffer is pointed at.

See WebGLFramebuffer::ResolvedData for where we gather a list of non-empty attachments.
Attachment #8812681 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 19

a year ago
Created attachment 8813971 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.
Attachment #8812681 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8813971 - Attachment description: Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorBuffers. → Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.
(Assignee)

Comment 20

a year ago
Comment on attachment 8813971 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.

Review of attachment 8813971 [details] [diff] [review]:
-----------------------------------------------------------------

Access ColorDrawBuffers by mResolvedCompleteData to prevent accessing undefined buffer.
Attachment #8813971 - Flags: review?(jgilbert)
Comment on attachment 8813971 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.

Review of attachment 8813971 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLFramebuffer.cpp
@@ +1632,4 @@
>  
>          dstDepthFormat = fnGetFormat(dstFB->mResolvedCompleteData->depthBuffer);
>          dstStencilFormat = fnGetFormat(dstFB->mResolvedCompleteData->stencilBuffer);
> +        if (dstFB->mResolvedCompleteData) {

This check is either not needed, or is needed in the two lines above.
Since it's not used in the above two lines, I suspect it's not needed below.
Attachment #8813971 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 22

a year ago
Created attachment 8815600 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.

Remove unnecessary null check.
Attachment #8813971 - Attachment is obsolete: true
(Assignee)

Comment 23

a year ago
Created attachment 8815601 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.
Attachment #8815600 - Attachment is obsolete: true
(Assignee)

Comment 24

a year ago
Comment on attachment 8815601 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.

Review of attachment 8815601 [details] [diff] [review]:
-----------------------------------------------------------------

I think the null check can be just removed.
Attachment #8815601 - Flags: review?(jgilbert)
Attachment #8815601 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 25

a year ago
Here is try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=115469d212892173bb0b8da43d30eade46df28d0
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 26

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57509edabda2
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorBuffers. r=jgilbert
Keywords: checkin-needed

Comment 27

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/57509edabda2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

a year ago
Flags: needinfo?(howareyou322)
Hi :jgilbert,
Is this worth uplifting to Beta51?
Flags: needinfo?(jgilbert)
Comment on attachment 8815601 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.

Approval Request Comment
[Feature/Bug causing the regression]: webgl2 crash
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:
Flags: needinfo?(jgilbert)
Attachment #8815601 - Flags: approval-mozilla-beta?
Attachment #8815601 - Flags: approval-mozilla-aurora?
Comment on attachment 8815601 [details] [diff] [review]
Iterate through defined ColorBuffers from mResolvedCompleteData instead of mColorDrawBuffers.

WebGL2 related patch. Beta51+ & Aurora52+. Should be in 51 beta 8.
Attachment #8815601 - Flags: approval-mozilla-beta?
Attachment #8815601 - Flags: approval-mozilla-beta+
Attachment #8815601 - Flags: approval-mozilla-aurora?
Attachment #8815601 - Flags: approval-mozilla-aurora+

Comment 31

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0afc1865fcc
status-firefox52: affected → fixed

Comment 32

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6d12cc5b5087
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.