738126 - WebGL doesn't check if texture is valid for generateMipmap

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Oleg Romashin (:romaxa)]

--- 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}

Comment 1

[Oleg Romashin (:romaxa)]

I tried to add simple check for !tex->MemoryUsage(), but it crashes later

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

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?

Comment 3

[Oleg Romashin (:romaxa)]

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

Comment 4

[Kelsey Gilbert [:jgilbert]]

This new one appears to be an unrelated crash, though.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Check that texture is valid for webgl.generateMipmap

Also adds check for compressed texture formats, since we will need to do that shortly.

Comment 6

[Oleg Romashin (:romaxa)]

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

Comment 7

[Kelsey Gilbert [:jgilbert]]

(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?

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: Enforce spec for webgl.generateMipmap, and zero is not PoT

It turns out zero is not an integer power-of-two.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: Enforce spec for webgl.generateMipmap, and zero is not PoT

Fixed, and emailed the WG about conformance tests.

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=ceb2971a6f3e

Comment 13

[Kelsey Gilbert [:jgilbert]]

Try run is clean.

Comment 14

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/162a102274e7

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/162a102274e7

Comment 16

[Gregg Tavares]

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.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Those looks sufficient, but the texture-mips tests in question are not part of the 1.0.1 test suite.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Removed webgl-test-needed per comment 16 and comment 17.