Closed Bug 1265676 Opened 4 years ago Closed 3 years ago

WebGL2.0 crash on assert failure in WebGLTexture::CopyTexSubImage on MOZ_RELEASE_ASSERT(false, "We should have caught all other errors.");

Categories

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

48 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: marcot, Assigned: jgilbert)

Details

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

Attachments

(5 files)

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

Steps to reproduce:

Ran one of our graphics tests on Nightly 48.0a1 (Windows 10)


Actual results:

Firefox Nightly Tab crashed


Expected results:

it should have rendered an image, like it does when using webgl1.0
repro: https://oc.unity3d.com/index.php/s/f0CnMb4MWQS7EmV

- try it on webgl1.0 to see expected result
- try it on webgl2.0 to repro crash
Superb find, I can reproduce on my Windows 10 on current Nightly.

Crash trace https://crash-stats.mozilla.com/report/index/14b0f8cc-c423-49b8-8050-c11a12160419
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: WebGL2.0 crash → WebGL2.0 crash on assert failure in WebGLTexture::CopyTexSubImage on MOZ_RELEASE_ASSERT(false, "We should have caught all other errors.");
2016-01-01 Firefox also crashes, so doesn't look like a regression anywhere. Release channel is safe from the crash, since it doesn't ship with WebGL2 yet (also the assert is not present in release(?) so the code would walk to just losing the WebGL context)

On email, Marco reported that this would be related to copying half-float textures.
First crashing version is 2015-12-17 with https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0babaa3edcf908c393b68a3dc2d1c2a2450c31ed&tochange=66fb852962c0d5f6f5fe0604204da4f5d17763c9, but doesn't mean much since before that the page gets a WebGL 1 context, and after that, it is getting a WebGL 2 context as per

10283ef31aee	Jeff Gilbert — Bug 1232864 - Cauterize and release WebGL 2 to Nightly. - r=jrmuizel

so this one has been with us since WebGL 2.
Component: Untriaged → Canvas: WebGL
Keywords: crash
Product: Firefox → Core
Flags: needinfo?(jgilbert)
Whiteboard: [gfx-noted]
I can reproduce it. I'll study this issue.
Assignee: nobody → ethlin
The test hit the size checking in angle[1] on Windows. The source format is RGBA8 and the destination format is RGBA16F. I think it's may be angle's problem. 
:jgilbert, could we remove the RELEASE_ASSERT in WebGLTextureUpload.cpp[2]? Or any suggestion?

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/validationES3.cpp#837
[2] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLTextureUpload.cpp#1945
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(jgilbert)
Angle follows the es spec 3.0.3, but 3.0.4 changed the size rules. The component size could be greater or smaller. I think we could just remove this check[1] in angle.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/validationES3.cpp#837
Status: UNCONFIRMED → NEW
Ever confirmed: true
Remove the component bit check since 3.0.4 doesn't limit the size. Could I send PR to mozilla/angle project?
Attachment #8756819 - Flags: feedback?(jgilbert)
(In reply to Ethan Lin[:ethlin] from comment #8)
> Created attachment 8756819 [details] [diff] [review]
> fix component size check
> 
> Remove the component bit check since 3.0.4 doesn't limit the size. Could I
> send PR to mozilla/angle project?

With this patch, the test can run normally.
Comment on attachment 8756819 [details] [diff] [review]
fix component size check

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

The spec requires that sized formats match exactly, whereas unsized formats do not necessarily need to.

ANGLE may need a change, but it is not this deletion.
Attachment #8756819 - Flags: feedback?(jgilbert) → feedback-
Per discussion with :jgilbert, he is working on texture image and this issue is included. So I untake the bug to prevent duplicate work.
Assignee: ethlin → nobody
Review commit: https://reviewboard.mozilla.org/r/57994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57994/
Attachment #8760431 - Flags: review?(ethlin)
Attachment #8760432 - Flags: review?(jmuizelaar)
Attachment #8760432 - Flags: review?(ethlin)
Attachment #8760433 - Flags: review?(jmuizelaar)
Attachment #8760433 - Flags: review?(ethlin)
Attachment #8760434 - Flags: review?(ethlin)
Attachment #8760433 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8760433 [details]
Bug 1265676 - Correct validation for CopyTexImage. -

https://reviewboard.mozilla.org/r/57998/#review55500

::: dom/canvas/WebGLTextureUpload.cpp:1721
(Diff revision 1)
> +
> +static const webgl::FormatUsageInfo*
> +ValidateCopyDestUsage(const char* funcName, WebGLContext* webgl,
> +                      const webgl::FormatInfo* srcFormat, GLenum internalFormat)
> +{
> +    const auto& fua = webgl->mFormatUsage;

fua is not a great variable name
Comment on attachment 8760432 [details]
Bug 1265676 - Add copyDecayFormats. -

https://reviewboard.mozilla.org/r/57996/#review55502
Attachment #8760432 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8760431 [details]
Bug 1265676 - Add component sizes to format tables. -

https://reviewboard.mozilla.org/r/57994/#review56390
Attachment #8760431 - Flags: review?(ethlin) → review+
Comment on attachment 8760433 [details]
Bug 1265676 - Correct validation for CopyTexImage. -

https://reviewboard.mozilla.org/r/57998/#review56394
Attachment #8760433 - Flags: review?(ethlin) → review+
Attachment #8760434 - Flags: review?(ethlin) → review+
https://reviewboard.mozilla.org/r/57994/#review56410

::: dom/canvas/WebGLFormats.cpp:179
(Diff revision 1)
>  
>      const CompressedFormatInfo* compressedFormatInfo = GetCompressedFormatInfo(format);
>      MOZ_ASSERT(!bytesPerPixel == bool(compressedFormatInfo));
>  
>      const FormatInfo info = { format, name, sizedFormat, unsizedFormat, componentType,
> -                              bytesPerPixel, isColorFormat, isSRGB, hasAlpha, hasDepth,
> +                              bytesPerPixel, r,g,b,a, isColorFormat, isSRGB, hasAlpha,

Add spaces between r,g,b,a.
Comment on attachment 8760432 [details]
Bug 1265676 - Add copyDecayFormats. -

https://reviewboard.mozilla.org/r/57996/#review56418

::: dom/canvas/WebGLFormats.cpp:314
(Diff revision 1)
>  
>  #undef FOO
> +
> +    ////////////////////////////////////////////////////////////////////////////
> +
> +    const auto fnSetCopyDecay = [](EffectiveFormat src, EffectiveFormat asR,

Should we add the decriptions in spec to here? like GLES 3.0.4, p140: "If internalformat is unsized, the internal format of the new texel array is determined by the following rules, applied in order..."
Attachment #8760432 - Flags: review?(ethlin) → review+
Assignee: nobody → jgilbert
You need to log in before you can comment on or make changes to this bug.