Bug 62422 – TiledBackingStore endlessly creates and destroys tiles due to an off-by-one error.

• Component: Platform
• Status: RESOLVED FIXED

[Dongseong Hwang]

If the viewport width equals the contents width, especially in the mobile device, TiledBackingStore endlessly creates and deletes the rightmost column and bottom row of tiles.

[Dongseong Hwang]

Created attachment 96687 [details]
patch

[Andreas Kling]

Comment on attachment 96687 [details]
patch

Golly!
We'll need this change in Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp as well.

[Dongseong Hwang]

Created attachment 96723 [details]
patch

I also changed TiledDrawingAreaProxy.
I'm glad to change WebKit2!

[Kenneth Rohde Christiansen]

Jocelyn please look at this

[Jocelyn Turcotte]

Comment on attachment 96723 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        REGRESSION(410d71df): Fix Qt bustage.

Please use SVN revision numbers, I guess you meant r77312.

Please also say why it broke, that bottomRight used to do this - 1 correction itself and that it was replaced everywhere by maxX/Y, which doesn't anymore.
I wonder, though, how this can be the only thing that broke, I don't understand.

> Source/WebKit2/ChangeLog:9
> +        REGRESSION(410d71df): Fix Qt bustage.

Same.


Otherwise looks fine, thanks for catching this!

[Darin Adler]

Comment on attachment 96723 [details]
patch

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

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:80
>      Tile::Coordinate topLeft = tileCoordinateForPoint(dirtyRect.location());
> -    Tile::Coordinate bottomRight = tileCoordinateForPoint(IntPoint(dirtyRect.maxX(), dirtyRect.maxY()));
> +    Tile::Coordinate bottomRight = tileCoordinateForPoint(IntPoint(dirtyRect.maxX() - 1, dirtyRect.maxY() - 1));
>      
>      for (unsigned yCoordinate = topLeft.y(); yCoordinate <= bottomRight.y(); ++yCoordinate) {
>          for (unsigned xCoordinate = topLeft.x(); xCoordinate <= bottomRight.x(); ++xCoordinate) {

I think a bette fix would be to change the <= into a < rather than changing the value in bottomRight.

[Jocelyn Turcotte]

(In reply to comment #6)
> (From update of attachment 96723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96723&action=review
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:80
> >      Tile::Coordinate topLeft = tileCoordinateForPoint(dirtyRect.location());
> > -    Tile::Coordinate bottomRight = tileCoordinateForPoint(IntPoint(dirtyRect.maxX(), dirtyRect.maxY()));
> > +    Tile::Coordinate bottomRight = tileCoordinateForPoint(IntPoint(dirtyRect.maxX() - 1, dirtyRect.maxY() - 1));
> >      
> >      for (unsigned yCoordinate = topLeft.y(); yCoordinate <= bottomRight.y(); ++yCoordinate) {
> >          for (unsigned xCoordinate = topLeft.x(); xCoordinate <= bottomRight.x(); ++xCoordinate) {
> 
> I think a bette fix would be to change the <= into a < rather than changing the value in bottomRight.

I don't like having to add the -1 everywhere either but the thing is that tileCoordinateForPoint translates the rect's coordinate by doing a int division by the tile size.

So I might be missing something, but I think this would skip the last row/column of tiles in cases where dirtyRect is not on the edge of a tile and tileCoordinateForPoint(IntPoint(dirtyRect.maxX() - 1, dirtyRect.maxY() - 1)) == tileCoordinateForPoint(IntPoint(dirtyRect.maxX(), dirtyRect.maxY())).

[Dongseong Hwang]

Created attachment 96904 [details]
It show why this patch is needed.

To Darin Adler,
'<=' is right on most case. However, if tile rect's width or height is power of n of cover(or dirty) rect's width or height, right bottom point is wrong.
This picture explains it.

To Jocelyn Turcotte,
I agree that -1 everywhere is ugly. I will patch about it.

[Dongseong Hwang]

Created attachment 96932 [details]
patch

[Yael]

Comment on attachment 96932 [details]
patch

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

I am not a reviewer, bellow are some comments.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:200
> +IntPoint TiledBackingStore::preciseBottomRight(const IntRect& rect)
> +{

No need to make this a member, it should be a static function.

> Source/WebKit2/ChangeLog:11
> +        REGRESSION(r77928): Cause off-by-one error.

The off-by-one error was introduced in r78783 and r78785.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:337
> +IntPoint TiledDrawingAreaTile::preciseBottomRight(const IntRect& rect)
> +{

Same here. No need to make this a member, it should be a static function.

[Eric Seidel (no email)]

Comment on attachment 96932 [details]
patch

I'm not sure how "precision" has anything to do with this.

[Dongseong Hwang]

Created attachment 98285 [details]
patch

Changed innerBottomRight static function from preciseBottomRight member method.

[Dongseong Hwang]

Created attachment 98286 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 98286 [details]
patch

Any way to test this using a pixel test or timeout test (you mentioned an infinite loop)

[Dongseong Hwang]

I think it is difficult to make the test.
The test requires to control tile size, test html content size, and window size.

I have no idea about using Layout test to test this.

[Kenneth Rohde Christiansen]

Comment on attachment 98286 [details]
patch

OK, but next time explain the lack of tests (and the reason why) in the changelog

[Dongseong Hwang]

OK.

[WebKit Review Bot]

Comment on attachment 98286 [details]
patch

Clearing flags on attachment: 98286

Committed r89803: <http://trac.webkit.org/changeset/89803>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.