[webkit-reviews] review denied: [Bug 72202] [chromium] Set texture limits as multiples of viewport size instead of hardcoded values : [Attachment 117718] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Eric Penner
<epenner at chromium.org>'s request for review:
Bug 72202: [chromium] Set texture limits as multiples of viewport size instead
of hardcoded values
https://bugs.webkit.org/show_bug.cgi?id=72202

Attachment 117718: Patch
https://bugs.webkit.org/attachment.cgi?id=117718&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117718&action=review


Sorry, this has a bad math error that I should have caught sooner.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:308
> +    size_t maxLimit = max<size_t>(0,
TextureManager::highLimitBytes(viewportSize()) - contentsMemoryUseBytes);
> +    size_t reclaimLimit = max<size_t>(0,
TextureManager::reclaimLimitBytes(viewportSize()) - contentsMemoryUseBytes);

wait a second, are you sure this does what you expect it does?
TextureManager::reclaimLimitBytes() returns a size_t, which is an unsigned
type. contentsMemoryUseBytes is an unsigned type. The binary "-" operator is
thus run as an unsigned operation. So if we had, for instance. a
reclaimLimitBytes of 5 and a contentsMemoryUseBytes of 12 - the output of this
operation when size_t is 64 bits would be 2^64-8 and the max of 0, 2^64-8 would
be 2^64-8 - not zero.

The way this code was structured before was deliberate to avoid this overflow
issue.	The fact that this bug is here also makes me wonder how much testing
this logic has received.