Closed Bug 1326385 Opened 3 years ago Closed 3 years ago

Crash in <T>::operator() | mozilla::WebGLFramebuffer::BlitFramebuffer

Categories

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

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: bogdan_maris, Assigned: jgilbert)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-5c83670a-a3e8-4295-be0f-c84152161230.
=============================================================

Other reports:
- Windows:
bp-fc81f28e-6e01-4cf4-8ffc-8bd3b2161230
bp-5c83670a-a3e8-4295-be0f-c84152161230
bp-1cc57cde-4f2b-4501-b91a-8593e2161230
bp-2590302d-bec3-4026-866a-8bc4c2161230
bp-ee20dcb0-08c3-49d3-8392-b03172161230
bp-2555d61a-c455-479e-842c-ba5f22161230
bp-91dfcdfa-b02d-48d6-a98a-0cf3d2161230
bp-eb9f0dcf-c3ee-4fce-bab6-1082b2161230
bp-d95a6bbb-f7eb-42ac-b8ee-2ee1b2161230
- Ubuntu:
bp-39c070f2-59c2-46ee-a3da-473a72161230
- Mac OS X:
bp-f529d8a1-b08f-4dd7-8011-cf0892161230

[Affected builds]:
- Firefox 51 beta 10
- latest Developer Edition 52.0a2
- latest Nightly 53.0a1

[Affected platforms]:
- Windows 10 64-bit
- Ubuntu 16.04 64-bit
- Mac OS X 10.12.2

[Steps to reproduce]:
1. Start Firefox
2. Uncheck all tests
3. Search for conformance2/rendering/read-draw-when-missing-image.html
4. Check the test and Run it

[Actual results]:
- Firefox crashes

[Regression range]:
- This is not a recent regression, it also crashes on Nightly build from 2015-12-17 although with other signature:
bp-fc81f28e-6e01-4cf4-8ffc-8bd3b2161230 - @ mozilla::WebGLTexture::CopyTexImage2D
QA Whiteboard: [qa-webgl2]
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
I think the remaining two failing subtests after this patch are bad subtests.
Comment on attachment 8822645 [details]
Bug 1326385 - Handle undefined images in BlitFramebuffer. -

https://reviewboard.mozilla.org/r/101522/#review102104

Looks a bit cleaner than the old code :+1:
Got only a few minor notes/questions

::: dom/canvas/WebGLFramebuffer.cpp:1648
(Diff revision 3)
> +    fnGetDepthAndStencilAttach(dstFB, &dstDepthAttach, &dstStencilAttach);
> +
> +    ////
> +
> +    const auto fnGetFormat = [](const WebGLFBAttachPoint* cur,
> +                                bool* const out_hasSamples) -> const webgl::FormatInfo*

this is really `inout_hasSamples`

::: dom/canvas/WebGLFramebuffer.cpp:1819
(Diff revision 3)
>          return;
>      }
>  
> -    if (srcSampleBuffers) {
> +    if (srcHasSamples) {
>          if (mask & LOCAL_GL_COLOR_BUFFER_BIT &&
> -            !colorFormatsMatch)
> +            dstHasColor && !colorFormatsMatch)

this extra `dstHasColor` doesn't really do much. If it's false, we are guaranteed to have `colorFormatsMatch == true` anyway

::: dom/canvas/WebGLFramebuffer.cpp:1847
(Diff revision 3)
>  
>      if (srcFB && dstFB) {
>          const WebGLFBAttachPoint* feedback = nullptr;
>  
>          if (mask & LOCAL_GL_COLOR_BUFFER_BIT) {
> -            for (const auto& cur : dstFB->mResolvedCompleteData->colorDrawBuffers) {
> +            MOZ_ASSERT(srcFB->mColorReadBuffer->IsDefined());

Shouldn't we return an error instead of asserting here?
Attachment #8822645 - Flags: review?(kvark) → review+
Comment on attachment 8822645 [details]
Bug 1326385 - Handle undefined images in BlitFramebuffer. -

Approval Request Comment
[Feature/Bug causing the regression]: 
[User impact if declined]: Crash when using expected WebGL 2 content.
[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]:
Attachment #8822645 - Flags: approval-mozilla-beta?
Attachment #8822645 - Flags: approval-mozilla-aurora?
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd28250d3dcb
Handle undefined images in BlitFramebuffer. - r=kvark
https://hg.mozilla.org/mozilla-central/rev/bd28250d3dcb
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8822645 [details]
Bug 1326385 - Handle undefined images in BlitFramebuffer. -

WebGL crash fix, let's take it for 51 along with the webgl2 changes.
Attachment #8822645 - Flags: approval-mozilla-beta?
Attachment #8822645 - Flags: approval-mozilla-beta+
Attachment #8822645 - Flags: approval-mozilla-aurora?
Attachment #8822645 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
I've managed to reproduce this issue with 51 beta 10 using Bogdan's Str from comment 0.

This is verified fixed on latest Nightly 53.0a1 (2017-01-05), latest Aurora 52.0a2 (2017-01-05) and 51 beta 11 (20170103031119), under the following OSes:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.