Bug 103816 – [CoordinatedGraphics] Use unsigned integers for CoordinatedTile IDs

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

CoordinatedTile currently uses *signed* integer type for its identifier. Due to the way we generate those IDs, it would be safer to use *unsigned* integers. This is because the generated ID will overflow at some point and the C and C++ language standards say that overflow of a signed value is undefined behaviour. 

In the C99 standard this is in section 6.5. In the C++98 standard it is in section 5 [expr], paragraph 5. "This means that a correct C/C++ program must never generate signed overflow when computing an expression. It also means that a compiler may assume that a program will never generated signed overflow".

Note that gcc has -fwrapv flag to assume that signed arithmetic overflow of addition, subtraction and multiplication wraps around using twos-complement representation. However, I still believe it is safer and more consistent to use unsigned integers here.

Note that some other classes in CoordinatedGraphics uses signed integers for those IDs but I'm planning to update those in separate patches (in this one gets accepted).

[Chris Dumez]

Created attachment 177110 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 177110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.h:27
> +#include "CoordinatedTile.h"

For layering protection _I think_ that we try to avoid WebKit2/UIProcess files including WebKit2/WebProcess files. This is the purpose of WebKit2/Shared.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109
> -    return !!m_ID;
> +    return m_ID != InvalidCoordinatedTileID;

To be honest I'm not totaly for this change because:
- If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX
- This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either
So overall I think it adds unecessary complexity.

That said, I'm ok if it gets in, the change itself looks fine.

[Jocelyn Turcotte]

(In reply to comment #2)
> - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX
Strike that, I forgot an important division, it would rather be 248 days for 100 layers created per frame per seconds.
It get almost possible at this point.

[Chris Dumez]

Comment on attachment 177110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109
>> +    return m_ID != InvalidCoordinatedTileID;
> 
> To be honest I'm not totaly for this change because:
> - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX
> - This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either
> So overall I think it adds unecessary complexity.
> 
> That said, I'm ok if it gets in, the change itself looks fine.

Why are you saying there is still bad behavior once we wrap? I updated the code in CoordinatedTile::updateBackBuffer() to handle the case where it wraps. To my knowledge, there is no bad behavior on wrapping anymore.
As you said, it is going to take a while to reach INT_MAX but I think it is cleaner and safer to use unsigned IDs still. To my knowledge, others IDs in WebKit are also unsigned, right?

You said it brings unnecessary complexity, but I'm not sure I follow. The only code I added if an extra if case in updateBackBuffer() to handle the wrapping case (make sure we don't reuse 0 as ID for a tile). The rest of the changes are purely about using an enum for the ID type instead of the actual integer type. In my opinion, this improves readability but of course it is optional.

[Jocelyn Turcotte]

Comment on attachment 177110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109
>>> +    return m_ID != InvalidCoordinatedTileID;
>> 
>> To be honest I'm not totaly for this change because:
>> - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX
>> - This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either
>> So overall I think it adds unecessary complexity.
>> 
>> That said, I'm ok if it gets in, the change itself looks fine.
> 
> Why are you saying there is still bad behavior once we wrap? I updated the code in CoordinatedTile::updateBackBuffer() to handle the case where it wraps. To my knowledge, there is no bad behavior on wrapping anymore.
> As you said, it is going to take a while to reach INT_MAX but I think it is cleaner and safer to use unsigned IDs still. To my knowledge, others IDs in WebKit are also unsigned, right?
> 
> You said it brings unnecessary complexity, but I'm not sure I follow. The only code I added if an extra if case in updateBackBuffer() to handle the wrapping case (make sure we don't reuse 0 as ID for a tile). The rest of the changes are purely about using an enum for the ID type instead of the actual integer type. In my opinion, this improves readability but of course it is optional.

Yes you're right, the overflow handling code that you added seems right (I didn't see it).

The unecessary complexity I see is the header cross-include that is required for the typedef + the overflow handling code that is impossible to test and that no living user will likely ever run. But again, this is not a strong opposition, just a personal taste.
Other parts of WebKit2 are using uint64_t actually, without typedef for types not defined in WebCore. How would that sound to you?

[Chris Dumez]

(In reply to comment #5)
> (From update of attachment 177110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177110&action=review
> 
> >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp:109
> >>> +    return m_ID != InvalidCoordinatedTileID;
> >> 
> >> To be honest I'm not totaly for this change because:
> >> - If a page creates 100 layers per frame per second, it would take 926 years to reach INT_MAX
> >> - This will still have a bad behavior once we wrap around to 0, so it's not 100% correct either
> >> So overall I think it adds unecessary complexity.
> >> 
> >> That said, I'm ok if it gets in, the change itself looks fine.
> > 
> > Why are you saying there is still bad behavior once we wrap? I updated the code in CoordinatedTile::updateBackBuffer() to handle the case where it wraps. To my knowledge, there is no bad behavior on wrapping anymore.
> > As you said, it is going to take a while to reach INT_MAX but I think it is cleaner and safer to use unsigned IDs still. To my knowledge, others IDs in WebKit are also unsigned, right?
> > 
> > You said it brings unnecessary complexity, but I'm not sure I follow. The only code I added if an extra if case in updateBackBuffer() to handle the wrapping case (make sure we don't reuse 0 as ID for a tile). The rest of the changes are purely about using an enum for the ID type instead of the actual integer type. In my opinion, this improves readability but of course it is optional.
> 
> Yes you're right, the overflow handling code that you added seems right (I didn't see it).
> 
> The unecessary complexity I see is the header cross-include that is required for the typedef + the overflow handling code that is impossible to test and that no living user will likely ever run. But again, this is not a strong opposition, just a personal taste.
> Other parts of WebKit2 are using uint64_t actually, without typedef for types not defined in WebCore. How would that sound to you?

Yes, I definitely need to fix that. Including the WebProcess headers like I did is not good.

[Chris Dumez]

Created attachment 177260 [details]
Patch

Take Jocelyn's feedback into consideration.

[WebKit Review Bot]

Comment on attachment 177260 [details]
Patch

Clearing flags on attachment: 177260

Committed r136658: <http://trac.webkit.org/changeset/136658>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.