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

Status: VERIFIED FIXED

(Core :: Canvas: WebGL, defect)

Description

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

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

Comment 1

[Jeff Gilbert [:jgilbert]]

STR 1.5: https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html?version=2.0.1

Comment 2

[Jeff Gilbert [:jgilbert]]

Attachment: Bug 1326385 - Handle undefined images in BlitFramebuffer. -

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

Comment 3

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8822645 [details]
Bug 1326385 - Handle undefined images in BlitFramebuffer. -

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

Comment 4

[Jeff Gilbert [:jgilbert]]

I think the remaining two failing subtests after this patch are bad subtests.

Comment 5

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8822645 [details]
Bug 1326385 - Handle undefined images in BlitFramebuffer. -

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

Comment 6

[Dzmitry Malyshau [:kvark]]

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?

Comment 7

[Jeff Gilbert [:jgilbert]]

Looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e48fda8cb7b54c981ffdee5d2fcb8741182741a

Comment 8

[Jeff Gilbert [:jgilbert]]

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]:

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd28250d3dcb
Handle undefined images in BlitFramebuffer. - r=kvark

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 12

[Jeff Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd773d028628
https://hg.mozilla.org/releases/mozilla-beta/rev/817d5c09bd43

Comment 13

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

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