Last Comment Bug 738126 - WebGL doesn't check if texture is valid for generateMipmap
: WebGL doesn't check if texture is valid for generateMipmap
Status: RESOLVED FIXED
[mobile-crash][native-crash]
: crash
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: ARM Android
: -- critical (vote)
: mozilla14
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
http://learningwebgl.com/lessons/less...
: 612194 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 20:02 PDT by Oleg Romashin (:romaxa)
Modified: 2012-05-10 10:57 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Check that texture is valid for webgl.generateMipmap (3.96 KB, patch)
2012-03-22 12:07 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Review
Enforce spec for webgl.generateMipmap, and zero is not PoT (4.50 KB, patch)
2012-03-22 14:05 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Review
Enforce spec for webgl.generateMipmap, and zero is not PoT (5.91 KB, patch)
2012-03-30 18:49 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Review

Description Oleg Romashin (:romaxa) 2012-03-21 20:02:02 PDT
--- 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) 2012-03-21 23:19:27 PDT
I tried to add simple check for !tex->MemoryUsage(), but it crashes later
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-03-22 02:26:31 PDT
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) 2012-03-22 10:17:13 PDT
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 Jeff Gilbert [:jgilbert] 2012-03-22 10:58:49 PDT
This new one appears to be an unrelated crash, though.
Comment 5 Jeff Gilbert [:jgilbert] 2012-03-22 12:07:51 PDT
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.
Comment 6 Oleg Romashin (:romaxa) 2012-03-22 13:23:13 PDT
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 Jeff Gilbert [:jgilbert] 2012-03-22 13:27:02 PDT
(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 Jeff Gilbert [:jgilbert] 2012-03-22 14:05:51 PDT
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.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-03-28 14:24:57 PDT
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 Jeff Gilbert [:jgilbert] 2012-03-30 18:49:46 PDT
Created attachment 611105 [details] [diff] [review]
Enforce spec for webgl.generateMipmap, and zero is not PoT

Fixed, and emailed the WG about conformance tests.
Comment 11 Jeff Gilbert [:jgilbert] 2012-03-30 18:52:47 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ceb2971a6f3e
Comment 12 Jeff Gilbert [:jgilbert] 2012-03-30 19:17:00 PDT
*** Bug 612194 has been marked as a duplicate of this bug. ***
Comment 13 Jeff Gilbert [:jgilbert] 2012-04-02 17:22:19 PDT
Try run is clean.
Comment 15 Marco Bonardo [::mak] 2012-04-04 04:58:24 PDT
https://hg.mozilla.org/mozilla-central/rev/162a102274e7
Comment 16 Gregg Tavares 2012-05-10 10:10:47 PDT
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 Jeff Gilbert [:jgilbert] 2012-05-10 10:47:24 PDT
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) 2012-05-10 10:57:57 PDT
Removed webgl-test-needed per comment 16 and comment 17.

Note You need to log in before you can comment on or make changes to this bug.