Bug 52172 – Max area bound needed in creation of IOSurface in ImageBufferCG.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Matthew Delaney]

During IOSurface creation in ImageBufferCG.cpp, there is currently no bound on the size that is requested. This should be capped at a reasonable area of 8192 x 8192. 

<rdar://problem/8762744>

[Matthew Delaney]

Actually, I think the current max area should be at least as large as the max area allowed for a canvas since canvas is currently IOSurface's only client in WebCore. This max area is 32768 * 8192. The coming patch will be to enforce this size constraint.

[Simon Fraser (smfr)]

Does making an IOSurface that large succeed?

[Matthew Delaney]

(In reply to comment #2)
> Does making an IOSurface that large succeed?
Yes. Instead, this patch will fallback to not using an IOSurface if either the width of height is larger than a max dimension of 4096.

[Matthew Delaney]

Created attachment 78472 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78472 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:119
> +    if (size.width() > MaxIOSurfaceDimension || size.height() > MaxIOSurfaceDimension)

You should probably use >= here.

[Simon Fraser (smfr)]

Can you add Layout tests for this?

Comment on attachment 78472 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:57
> +static const int MaxIOSurfaceDimension = 4096;

Current WebKit style is that this should begin with a lowercase m.

[Matthew Delaney]

Created attachment 78478 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78478 [details]
Patch

Should you be adding to philip tests? It seems that this suite should only contain tests from him.

r- for that and lack of LayoutTests changelog.

[Matthew Delaney]

Created attachment 78573 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78573 [details]
Patch

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

> LayoutTests/canvas/tests.js:10
> +function _valToString(val)
> +{
> +        return '[' + typeof(val) + ']';
> +}
> +var _failed = false;
> +var _asserted = false;
> +

Why all the underscores?

You should really just use the js-test stuff in fast/js/resources.

[Matthew Delaney]

Created attachment 78589 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78589 [details]
Patch

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

r=me with some test cleanup.

> LayoutTests/ChangeLog:11
> +        * fast/canvas/height.largeValues-expected.txt: Added.
> +        * fast/canvas/height.largeValues.html: Added.
> +        * fast/canvas/width.largeValues-expected.txt: Added.
> +        * fast/canvas/width.largeValues.html: Added.

I see no reason to have separate tests for height and width. Also, the '.' naming convention isn't used in fast/ tests normally.

> LayoutTests/fast/canvas/height.largeValues.html:15
> +canvas.height = 1000;
> +if (canvas.height ==1000)
> +	testPassed("height == 1000");
> +else
> +	testFailed("height == 1000");

Does this really exercise the underlying ImageBuffer code? How about trying to draw a pixel at 0, 999 and then reading it back?

[Matthew Delaney]

Created attachment 78727 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78727 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-large-dimensions.html:33
> +		testPassed(msg);
> +	else
> +		testFailed(msg);
> +	x = canvas.width-2;

Weird indentation. Tabs?

[Matthew Delaney]

Committed r75648: <http://trac.webkit.org/changeset/75648>

[Ryosuke Niwa]

The test added by this patch (fast/canvas/canvas-large-dimensions.html) has been failing on Leopard, GTK, Qt, and Chromium.

[Ryosuke Niwa]

I didn't notice but https://bugs.webkit.org/show_bug.cgi?id=52341 had been filed.