629149 - Crash in [ @ mozilla::WebGLTexture::NeedFakeBlack() ]

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=mozilla%3A%3AWebGLTexture%3A%3ANeedFakeBlack%28%29&version=Firefox%3A4.0b10

Crash address near zero (typically 0x58) suggests that we have a null WebGLTexture here.

Comment 1

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

Attachment: check if deleted

Comment 2

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

My understanding of the stack traces is that WebGLContext::NeedFakeBlack() has been inlined, and the crash happens when it calls WebGLTexture::NeedFakeBlack().

What puzzles me is that the near-zero addresses were suggesting a null WebGLTexture object, but the code was checking for that already:

   if (mBound2DTextures[i] && mBound2DTextures[i]->NeedFakeBlack())

so my second best chance is to check for Deleted...

Comment 3

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

I admit to be completely in the fuzzy on this one.

The fact is that we are crashing when doing

    if (mBound2DTextures[i] &&
        mBound2DTextures[i]->NeedFakeBlack())

Is there some nsRefPtr subtlety that I'm missing here, that's causing NeedFakeBlack() to be called even though the pointer is null?

I don't like my own patch because I don't understand why suddenly here we'd have to check for Deleted.

Comment 4

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

Ah OK... if I understand well, when a WebGLTexture falls out of scope in the JS program, it enters this 'Deleted' state and really we should check for Deleted everytime we access objects in our C++ code? If so, then a much bigger patch is needed.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 507242 [details] [diff] [review]
check if deleted

hm, no, this isn't right; something weird is going on with those crashes

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix TextureImageUnits -> TextureUnits

I don't know if this is the fix, but the loops should be over mGLMaxTextureUnits.  However, mGLMaxTextureImageUnits *should* always be <= mGLMaxTextureUnits, so I don't think we could have read over the end of the array or something here.

Comment 7

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

Comment on attachment 507664 [details] [diff] [review]
fix TextureImageUnits -> TextureUnits

Yup, checked the code, that is the size of these arrays created in InitAndValidateGL(), and this value is indeed MAX_COMBINED_TEXTURE_IMAGE_UNITS:

    gl->fGetIntegerv(LOCAL_GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, (GLint*) &mGLMaxTextureUnits);
    mBound2DTextures.SetLength(mGLMaxTextureUnits);
    mBoundCubeMapTextures.SetLength(mGLMaxTextureUnits);

so r+.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

marking this blocking final, to make sure we get this in.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/5eb64cb4592d

Optimistically calling this fixed..