1239144 - Check that the level passed in is in the required range.

Status: RESOLVED WORKSFORME

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Check that the level passed in is in the required range.

This avoids a crash in gles3/negativetextureapi.html

The return type of MaxMipmapLevels is changed so that we don't get a warning about comparing between unsigned and signed. It seems like GLint is a better type for level related stuff.

Comment 1

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> Created attachment 8707185 [details] [diff] [review]
> Check that the level passed in is in the required range.
> 
> This avoids a crash in gles3/negativetextureapi.html
> 
> The return type of MaxMipmapLevels is changed so that we don't get a warning
> about comparing between unsigned and signed. It seems like GLint is a better
> type for level related stuff.

The idea here is that while we take in signed types, we generally validate them as being non-negative. Since these types will always be non-negative internally, we use unsigned types.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8707185 [details] [diff] [review]
Check that the level passed in is in the required range.

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

::: dom/canvas/WebGLTexture.h
@@ +153,5 @@
>      protected:
>          ImageInfo& operator =(const ImageInfo& a);
>  
>      public:
> +        GLint MaxMipmapLevels() const {

I would prefer to keep this uint32_t.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +325,5 @@
>      }
>  
>      WebGLTexture::ImageInfo& imageInfo = texture->ImageInfoAt(target, level);
>  
> +    if (level >= imageInfo.MaxMipmapLevels()) {

We can't check this here. (It's valid to create a TexImage with level 1 first, which this will fail)
We should be checking this in ValidateTexImageSelection.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8707185 [details] [diff] [review]
Check that the level passed in is in the required range.

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +325,5 @@
>      }
>  
>      WebGLTexture::ImageInfo& imageInfo = texture->ImageInfoAt(target, level);
>  
> +    if (level >= imageInfo.MaxMipmapLevels()) {

I don't understand this comment. Can you clarify what would fail here?

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> Comment on attachment 8707185 [details] [diff] [review]
> Check that the level passed in is in the required range.
> 
> Review of attachment 8707185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLTextureUpload.cpp
> @@ +325,5 @@
> >      }
> >  
> >      WebGLTexture::ImageInfo& imageInfo = texture->ImageInfoAt(target, level);
> >  
> > +    if (level >= imageInfo.MaxMipmapLevels()) {
> 
> I don't understand this comment. Can you clarify what would fail here?

MaxMipmapLevels is based on the imageInfo's current dimensions, and tells you the max number of levels from this imageInfo and above. It is meant to be used on the 'base image info' (level=0). We should change the code here so this is more obvious.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> MaxMipmapLevels is based on the imageInfo's current dimensions, and tells
> you the max number of levels from this imageInfo and above. It is meant to
> be used on the 'base image info' (level=0). We should change the code here
> so this is more obvious.

My comment wasn't clear. I wanted you to explain this comment:

> We can't check this here. (It's valid to create a TexImage with level 1 first, which this will fail)

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > MaxMipmapLevels is based on the imageInfo's current dimensions, and tells
> > you the max number of levels from this imageInfo and above. It is meant to
> > be used on the 'base image info' (level=0). We should change the code here
> > so this is more obvious.
> 
> My comment wasn't clear. I wanted you to explain this comment:
> 
> > We can't check this here. (It's valid to create a TexImage with level 1 first, which this will fail)

tl;dr: We just need a WebGLContext::MaxMipmapLevel(texTarget).

My comment goes in an unnecessary direction:

While we should make sure we don't accept an invalid level, doing it the presented way (checking the existing imageInfo object for that level) is only viable for TexSubImage, not TexImage. TexImage respecifies the imageInfo if it succeeds, but TexSubImage does not change the imageInfo. (except to maybe mark it as fully-initialized)

We also can't check the MaxMipmapLevels based on the existing texture in TexImage, because we're respecifying the texture's imageInfo. We also can't assume that level 0 was specified before level 1.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Check that the level passed in is in the required range v2

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8715037 [details] [diff] [review]
Check that the level passed in is in the required range v2

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +319,5 @@
>          return false;
>      }
>  
> +    if (level >= WebGLTexture::kMaxLevelCount ||
> +        static_cast<uint32_t>(level) >= texture->MaxMipmapLevels())

We can't forbid this here, since ValidateTexImage is also called by TexImage*:
Assume the texture is already specified with level=0 1x1, then MapMipmapLevels() will be 1, even through specifying TexImage with level=5 2x2 would be fine.

I'll take a look at the crash more carefully to see what we need.

Comment 9

[Kelsey Gilbert [:jgilbert]]

I cannot replicate the crash on Win10, nor does ANGLE generate a GL error that we don't expect. (though since these are the texture calls, which are all wrapped, this is not surprising)

Is this on OSX?

Comment 10

[Kelsey Gilbert [:jgilbert]]

Also specify which subtest it fails on, if you can.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I'm not seeing this crash anymore. Maybe we should just throw this bug in the trash.