576067 - Let WebGL use CheckedInt

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Attachment: Use CheckedInt in WebGL

Checking for integer overflow everywhere I could see a need for it. Many places (mostly additions) were already doing it with custom code --> replaced. Other places (mostly multiplications) were not checked.

Even though places like DrawArrays are performance-critical, I don't think that using CheckedInt there is a problem, because CheckedInt is quite cheap, at least compared to all the if()'s that we do anyway.

Comment 1

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

I'll look into this in more detail soon, but is there any reason why things like "mGeneration" shouldn't be a CheckedInt in the class itself?

Also, it seems that it would be really helpful to have:

typedef CheckedInt<PRUint32> CheckedUint32;
typedef CheckedInt<PRInt32> CheckedInt32;

in CheckedInt.h, since those are going to be the most common uses, and having to specify the template argument everywhere is ugly (could get away with <> in some cases, but still ugly).

Comment 2

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

(In reply to comment #1)
> I'll look into this in more detail soon, but is there any reason why things
> like "mGeneration" shouldn't be a CheckedInt in the class itself?

No particular reason, that indeed looks like a good idea.

Do you think that it would be a good idea to implement operator++ in CheckedInt?

Also, for the alignedRowSize computation, we were originally using bitwise operator&, but since that wasn't available in CheckedInt, I replaced that by using * and /. Which is slower, but that shouldn't make a difference as other things done in that context are more expensive. Anyway, my question is, do you think that CheckedInt should offer bitwise operator &, |, ^, ~ ?

> 
> Also, it seems that it would be really helpful to have:
> 
> typedef CheckedInt<PRUint32> CheckedUint32;
> typedef CheckedInt<PRInt32> CheckedInt32;
> 
> in CheckedInt.h, since those are going to be the most common uses, and having
> to specify the template argument everywhere is ugly (could get away with <> in
> some cases, but still ugly).

OK, this is actually something we had evocated with Jeff. For consistency, let's add typedefs for all 8 supported types.

Comment 3

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

Attachment: Use CheckedInt in WebGL

Here you go: new in this version:
 - store mGeneration as a CheckedInt
 - add increment/decrement operators
 - add convenience typedefs

Comment 4

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

Comment on attachment 455687 [details] [diff] [review]
Use CheckedInt in WebGL


>-        WebGLuint needed = vd.byteOffset +     // the base offset
>-            vd.actualStride() * (count-1) +    // to stride to the start of the last element group
>-            vd.componentSize() * vd.size;      // and the number of bytes needed for these components
>+        CheckedInt<PRUint32> checked_needed = CheckedInt<PRUint32>(vd.byteOffset) + // the base offset
>+            CheckedInt<PRUint32>(vd.actualStride()) * (count-1) + // to stride to the start of the last element group
>+            CheckedInt<PRUint32>(vd.componentSize()) * vd.size;   // and the number of bytes needed for these components


Use CheckedUint32 here too.

Comment 5

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

http://hg.mozilla.org/mozilla-central/rev/449aa183a6c3