WebGL doesn't check if texture is valid for generateMipmap

RESOLVED FIXED in mozilla14

Status

()

Core
Canvas: WebGL
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: romaxa, Assigned: jgilbert)

Tracking

({crash})

Trunk
mozilla14
ARM
Android
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mobile-crash][native-crash], crash signature, URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
--- WebGL context created: 0x43f3a800
GL ERROR: void mozilla::gl::GLContext::fGenerateMipmap(GLenum) generated GL error GL_OUT_OF_MEMORY(0x0505)
###!!! ABORT: file ../../../dist/include/GLContext.h, line 1754



68	    gDummyCounter += *p;   // TODO annotation saying we know 
(gdb) bt
#0  TouchBadMemory () at memory/mozalloc/mozalloc_abort.cpp:68
#1  0x3db93f34 in mozalloc_abort (msg=<value optimized out>)
    at memory/mozalloc/mozalloc_abort.cpp:89
#2  0x3f5bff68 in Abort (aMsg=0xa <Address 0xa out of bounds>)
    at xpcom/base/nsDebugImpl.cpp:388
#3  0x3f5c01e4 in NS_DebugBreak_P (aSeverity=3, aStr=0x0, aExpr=<value optimized out>, aFile=<value optimized out>, 
    aLine=1754) at xpcom/base/nsDebugImpl.cpp:375
#4  0x3ea31200 in mozilla::gl::GLContext::AfterGLCall (this=0x43f3a800, 
    glFunction=0x3fe13464 "void mozilla::gl::GLContext::fGenerateMipmap(GLenum)")
    at ../../../dist/include/GLContext.h:1754
#5  0x3ea4df90 in mozilla::WebGLContext::GenerateMipmap (this=0x439266e0, target=3553)
    at content/canvas/src/WebGLContextGL.cpp:1927
#6  0x3f0564f0 in nsIDOMWebGLRenderingContext_GenerateMipmap (cx=0x40d27b80, argc=1, vp=0x41400158)
    at obj-build/js/xpconnect/src/dom_quickstubs.cpp:23853
#7  0x3f9302d0 in js::CallJSNative (cx=0x40d27b80, 
    native=0x3f056428 <nsIDOMWebGLRenderingContext_GenerateMipmap(JSContext*, unsigned int, jsval*)>, args=...)
    at js/src/jscntxtinlines.h:314
#8  0x3f94a140 in js::InvokeKernel (cx=0x40d27b80, args=..., construct=js::NO_CONSTRUCT)
---Type <return> to continue, or q <return> to quit---q
 at /home/romaxa/mozdev/mozillahQuit
(gdb) frame 5
#5  0x3ea4df90 in mozilla::WebGLContext::GenerateMipmap (this=0x439266e0, target=3553)
    at content/canvas/src/WebGLContextGL.cpp:1927
1927	    gl->fGenerateMipmap(target);
(gdb) p tex->mImageInfos.ElementAt(0)
$1 = (mozilla::WebGLTexture::ImageInfo &) @0x444c36c8: {<mozilla::WebGLRectangleObject> = {mWidth = 0, mHeight = 0}, 
  mFormat = 0, mType = 0, mIsDefined = false}
(Reporter)

Comment 1

6 years ago
I tried to add simple check for !tex->MemoryUsage(), but it crashes later
Please ping me later about this as I'm traveling/PTO today, and might forget about it, but I'm interested.

There are better ways to check if a texture has image data at level 0, see HasImageInfo.

If you add the check, what is the crash you get later? Is it a different bug?

Updated

6 years ago
Severity: normal → critical
Crash Signature: [@ TouchBadMemory | mozalloc_abort | NS_DebugBreak_P | mozilla::gl::GLContext::AfterGLCall]
Keywords: crash
OS: Linux → Android
Whiteboard: [mobile-crash][native-crash]
(Reporter)

Comment 3

6 years ago
I added simple check like:

+    size_t face = WebGLTexture::FaceForTarget(target);
+    if (!tex->HasImageInfoAt(0, face))
+        return ErrorInvalidOperation("texSubImage2D: no texture image previously defined for this level and face");

And see this crash:
#0  glFlush () at misc.c:731
#1  0x3bde1a80 in mozilla::gl::GLContext::AfterGLCall (this=0xaed6f2f4, 
    glFunction=0x3d1faf88 "void mozilla::gl::GLContext::raw_fBindFramebuffer(GLenum, GLuint)")
    at ../../../dist/include/GLContext.h:1744
#2  0x3be07060 in mozilla::WebGLContext::BindFramebuffer (this=0x3f0f84e0, target=36160, 
    fbobj=<value optimized out>)
    at content/canvas/src/WebGLContextGL.cpp:268
#3  0x3c407b24 in nsIDOMWebGLRenderingContext_BindFramebuffer (cx=0x3e2e26e0, argc=2, vp=0x3ea001a0)
    at obj-build-n9-honk-x11/js/xpconnect/src/dom_quickstubs.cpp:22372
#4  0x3cc93920 in js::CallJSNative (cx=0x3e2e26e0, 
    native=0x3c407a0c <nsIDOMWebGLRenderingContext_BindFramebuffer(JSContext*, unsigned int, jsval*)>, args=...)
    at js/src/jscntxtinlines.h:314
#5  0x3ccad4b0 in js::InvokeKernel (cx=0x3e2e26e0, args=..., construct=js::NO_CONSTRUCT)
    at js/src/jsinterp.cpp:513
(Assignee)

Comment 4

6 years ago
This new one appears to be an unrelated crash, though.
(Assignee)

Updated

6 years ago
Summary: WebGL crashes on attempt to gen Mipmap for undefined texture → WebGL doesn't check if texture is valid for generateMipmap
(Assignee)

Comment 5

6 years ago
Created attachment 608419 [details] [diff] [review]
Check that texture is valid for webgl.generateMipmap

Also adds check for compressed texture formats, since we will need to do that shortly.
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Attachment #608419 - Flags: review?(bjacob)
(Reporter)

Comment 6

6 years ago
semicolon missing in patch, but with that I see:
--- WebGL context created: 0x41fc7000
JavaScript warning: http://learningwebgl.com/lessons/lesson16/index.html, line 273: WebGL: generateMipmap: Texture has no data at level zero.
...
still crashes on attempt to bindFramebuffer but I guess that is different problem
(Assignee)

Comment 7

6 years ago
(In reply to Oleg Romashin (:romaxa) from comment #6)
> semicolon missing in patch, but with that I see:
> --- WebGL context created: 0x41fc7000
> JavaScript warning: http://learningwebgl.com/lessons/lesson16/index.html,
> line 273: WebGL: generateMipmap: Texture has no data at level zero.
> ...
> still crashes on attempt to bindFramebuffer but I guess that is different
> problem

Thanks, can we file a new bug for that?
(Assignee)

Comment 8

6 years ago
Created attachment 608467 [details] [diff] [review]
Enforce spec for webgl.generateMipmap, and zero is not PoT

It turns out zero is not an integer power-of-two.
Attachment #608419 - Attachment is obsolete: true
Attachment #608419 - Flags: review?(bjacob)
Attachment #608467 - Flags: review?(bjacob)
Comment on attachment 608467 [details] [diff] [review]
Enforce spec for webgl.generateMipmap, and zero is not PoT

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

r=me with 2 requested changes. Also, we need a conformance test.

::: content/canvas/src/WebGLContextGL.cpp
@@ +1923,5 @@
> +    if (IsTextureFormatCompressed(format))
> +        return ErrorInvalidOperation("generateMipmap: Texture data at level zero is compressed.");
> +
> +    if (!tex->AreAllLevel0ImageInfosEqual())
> +        return ErrorInvalidOperation("generateMipmap: The six faces of this cube map have different dimensions, format, or type.");

This uses WebGLTexture::ImageInfo::operator!= which omits to compare that the mIsDefined are equal. Please fix it.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +255,5 @@
> +        case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
> +        case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
> +        case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
> +        case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
> +            return true;

Please add a default: with a NS_ABORT() so we'll catch that in debug builds.
Attachment #608467 - Flags: review?(bjacob) → review+
Blocks: 728357
Blocks: 736436
(Assignee)

Comment 10

6 years ago
Created attachment 611105 [details] [diff] [review]
Enforce spec for webgl.generateMipmap, and zero is not PoT

Fixed, and emailed the WG about conformance tests.
Attachment #608467 - Attachment is obsolete: true
Attachment #611105 - Flags: review?(bjacob)
(Assignee)

Comment 11

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ceb2971a6f3e
(Assignee)

Updated

6 years ago
Duplicate of this bug: 612194
(Assignee)

Comment 13

6 years ago
Try run is clean.
Attachment #611105 - Flags: review?(bjacob) → review+
No longer blocks: 736436
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/162a102274e7
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/162a102274e7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
No longer blocks: 728357
Whiteboard: [mobile-crash][native-crash] → [mobile-crash][native-crash] webgl-test-needed

Comment 16

5 years ago
I'm not really sure what test you need here

this test tests that 0x0 pixel level 0 fails on genereteMipmap
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/textures/texture-mips.html

this test tests that an npot level 0 fails on generateMipmap
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/textures/texture-npot.html

I'm not sure it's possible to test out of memory

If there's something that needs to be added I'm happy to do it.
(Assignee)

Comment 17

5 years ago
Those looks sufficient, but the texture-mips tests in question are not part of the 1.0.1 test suite.
Removed webgl-test-needed per comment 16 and comment 17.
Whiteboard: [mobile-crash][native-crash] webgl-test-needed → [mobile-crash][native-crash]
You need to log in before you can comment on or make changes to this bug.