Accurately clip copyTexImage2D and copyTexSubImage2D

[Dean Jackson]

Accurately clip copyTexImage2D and copyTexSubImage2D

[Dean Jackson]

<rdar://problem/35083877>

[Dean Jackson]

Created attachment 330609 [details]
Patch

[Eric Carlson]

Comment on attachment 330609 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        The code to make sure copyTexSubImage2D and copyTexImage2D don't try to read

Nit: don't => doesn't

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:363
> +    if (checkedInputRight.hasOverflowed() || checkedInputBottom.hasOverflowed()) {
> +        *clippedX = 0;
> +        *clippedY = 0;
> +        *clippedWidth = 0;
> +        *clippedHeight = 0;
> +        return true;
> +    }
> +
> +    GC3Dint right = std::min(checkedInputRight.unsafeGet(), sourceWidth);
> +    GC3Dint bottom = std::min(checkedInputBottom.unsafeGet(), sourceHeight);
> +
> +    if (left >= right || top >= bottom) {
> +        *clippedX = 0;
> +        *clippedY = 0;
> +        *clippedWidth = 0;
> +        *clippedHeight = 0;
> +        return true;
> +    }

It is a shame to have the same failure code twice. You might rearrange the code so you only have to include it once, eg something like:

    GC3Dint left = std::max(x, 0);
    GC3Dint top = std::max(y, 0);
    GC3Dint right = 0;
    GC3Dint bottom = 0;

    Checked<GC3Dint, RecordOverflow> checkedInputRight = Checked<GC3Dint>(x) + Checked<GC3Dsizei>(width);
    Checked<GC3Dint, RecordOverflow> checkedInputBottom = Checked<GC3Dint>(y) + Checked<GC3Dsizei>(height);
    if (!checkedInputRight.hasOverflowed() && !checkedInputBottom.hasOverflowed()) {
        right = std::min(checkedInputRight.unsafeGet(), sourceWidth);
        bottom = std::min(checkedInputBottom.unsafeGet(), sourceHeight);
    }

    if (left >= right || top >= bottom) {
       *clippedX = 0;
       *clippedY = 0;
       *clippedWidth = 0;
       *clippedHeight = 0;
       return true;
    }

> LayoutTests/fast/canvas/webgl/copy-tex-image-and-sub-image-2d-bad-input.html:43
> +// These two declarations need to be global for "shouldBe" to see them

Nit: These two => These three, or just These

[EWS Watchlist]

Comment on attachment 330609 [details]
Patch

Attachment 330609 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5949239

New failing tests:
fast/canvas/webgl/copy-tex-image-and-sub-image-2d-bad-input.html

[EWS Watchlist]

Created attachment 330627 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Dean Jackson]

Created attachment 330649 [details]
Patch

[Dean Jackson]

Thanks Eric. Updated patch to see if EWS will be happier.

[Dean Jackson]

Committed r226490: <https://trac.webkit.org/changeset/226490>