Closed Bug 1239144 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jrmuizel, Unassigned)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8707185 - Attachment is patch: true
Attachment #8707185 - Flags: review?(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 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.
Attachment #8707185 - Flags: review?(jgilbert) → review-
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?
(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.
(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)
(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.
Blocks: webgl2
Attachment #8707185 - Attachment is obsolete: true
Attachment #8715037 - Flags: review?(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.
Attachment #8715037 - Flags: review?(jgilbert) → review-
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?
Flags: needinfo?(jmuizelaar)
Also specify which subtest it fails on, if you can.
I'm not seeing this crash anymore. Maybe we should just throw this bug in the trash.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jmuizelaar)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.