Bug 62422 - TiledBackingStore endlessly creates and destroys tiles due to an off-by-one error.
Summary: TiledBackingStore endlessly creates and destroys tiles due to an off-by-one e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-09 19:07 PDT by Dongseong Hwang
Modified: 2011-09-01 11:33 PDT (History)
13 users (show)

See Also:


Attachments
patch (4.23 KB, patch)
2011-06-09 19:11 PDT, Dongseong Hwang
akling: review-
Details | Formatted Diff | Diff
patch (7.90 KB, patch)
2011-06-10 03:35 PDT, Dongseong Hwang
jturcotte: review-
jturcotte: commit-queue-
Details | Formatted Diff | Diff
It show why this patch is needed. (29.61 KB, image/png)
2011-06-12 18:45 PDT, Dongseong Hwang
no flags Details
patch (10.81 KB, patch)
2011-06-13 01:11 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
patch (9.74 KB, patch)
2011-06-22 18:28 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
patch (9.74 KB, patch)
2011-06-22 18:31 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2011-06-09 19:07:54 PDT
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.
Comment 1 Dongseong Hwang 2011-06-09 19:11:14 PDT
Created attachment 96687 [details]
patch
Comment 2 Andreas Kling 2011-06-10 00:39:05 PDT
Comment on attachment 96687 [details]
patch

Golly!
We'll need this change in Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp as well.
Comment 3 Dongseong Hwang 2011-06-10 03:35:17 PDT
Created attachment 96723 [details]
patch

I also changed TiledDrawingAreaProxy.
I'm glad to change WebKit2!
Comment 4 Kenneth Rohde Christiansen 2011-06-10 04:21:09 PDT
Jocelyn please look at this
Comment 5 Jocelyn Turcotte 2011-06-10 07:18:01 PDT
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!
Comment 6 Darin Adler 2011-06-10 09:23:24 PDT
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.
Comment 7 Jocelyn Turcotte 2011-06-10 10:35:20 PDT
(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())).
Comment 8 Dongseong Hwang 2011-06-12 18:45:26 PDT
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.
Comment 9 Dongseong Hwang 2011-06-13 01:11:50 PDT
Created attachment 96932 [details]
patch
Comment 10 Yael 2011-06-13 05:38:02 PDT
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.
Comment 11 Eric Seidel 2011-06-13 15:05:57 PDT
Comment on attachment 96932 [details]
patch

I'm not sure how "precision" has anything to do with this.
Comment 12 Dongseong Hwang 2011-06-22 18:28:52 PDT
Created attachment 98285 [details]
patch

Changed innerBottomRight static function from preciseBottomRight member method.
Comment 13 Dongseong Hwang 2011-06-22 18:31:15 PDT
Created attachment 98286 [details]
patch
Comment 14 Kenneth Rohde Christiansen 2011-06-23 07:49:08 PDT
Comment on attachment 98286 [details]
patch

Any way to test this using a pixel test or timeout test (you mentioned an infinite loop)
Comment 15 Dongseong Hwang 2011-06-23 20:16:53 PDT
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.
Comment 16 Kenneth Rohde Christiansen 2011-06-27 00:47:57 PDT
Comment on attachment 98286 [details]
patch

OK, but next time explain the lack of tests (and the reason why) in the changelog
Comment 17 Dongseong Hwang 2011-06-27 00:58:11 PDT
OK.
Comment 18 WebKit Review Bot 2011-06-27 01:11:06 PDT
Comment on attachment 98286 [details]
patch

Clearing flags on attachment: 98286

Committed r89803: <http://trac.webkit.org/changeset/89803>
Comment 19 WebKit Review Bot 2011-06-27 01:11:12 PDT
All reviewed patches have been landed.  Closing bug.