Bug 82107 – [chromium] When prepainting fails, tiles dirty rects may be cleared

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] When prepainting fails, tiles dirty rects may be cleared

[Dana Jansens]

This is a super yummy bug. If a layer was prepainting and failed halfway through, it would lose the dirty bit on all the tiles it had already looked at. But those tiles would also have a valid reserved texture and a textureId of 0.

These tiles are no longer considered dirty (even though they are) so they would get pushed to impl, but they have a textureId of 0 so they would not display.

Then when it went to paint the layer if it became visible, those tiles would not be invalid, or dirty. So they would not get updated and keep a textureId of 0 on the impl thread.

Once the tile became dirty thru invalidation, it would paint the invalidated region, leaving the rest black if it was able to do a partial update.

Simple fix: don't clear dirty rects until you know you're going to update the layer.

[Dana Jansens]

Created attachment 133617 [details]
Patch

[Adrienne Walker]

Comment on attachment 133617 [details]
Patch

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

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:160
> +    bool hasTextureIdForTileAt(int i, int j)
> +    {
> +        return CCTiledLayerImpl::hasTextureIdForTileAt(i, j);
> +    }

You could shorten this and the function above to just "using CCTiledLayerImpl::hasTextureIdForTileAt;"

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:505
> +    EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 0));

Was hasTileAt not enough? I would have expected the isValid call to fail during pushPropertiesTo if the textureId was zero.

[Dana Jansens]

Comment on attachment 133617 [details]
Patch

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

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:160
>> +    }
> 
> You could shorten this and the function above to just "using CCTiledLayerImpl::hasTextureIdForTileAt;"

ah, nice.

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:505
>> +    EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 0));
> 
> Was hasTileAt not enough? I would have expected the isValid call to fail during pushPropertiesTo if the textureId was zero.

Yeh, it just checks the tile is present. And most tests are okay with that. Making this a condition would make those tests fail at present (lack of calling updater.update()).

bool CCTiledLayerImpl::hasTileAt(int i, int j) const { return m_tiler->tileAt(i, j); }

[Dana Jansens]

Created attachment 133619 [details]
Patch

Using using.

I like having separate checks for hasTileAt() and hasTextureForTileAt(). It lets me check FALSE of the hasTileAt() separately so I can know that I didn't push a dirty texture (I noticed that we were in this situation because of this unit test).

[Adrienne Walker]

(In reply to comment #4)
> (From update of attachment 133617 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133617&action=review
> 
> >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:505
> >> +    EXPECT_TRUE(layerImpl2->hasTextureIdForTileAt(0, 0));
> > 
> > Was hasTileAt not enough? I would have expected the isValid call to fail during pushPropertiesTo if the textureId was zero.
> 
> Yeh, it just checks the tile is present. And most tests are okay with that. Making this a condition would make those tests fail at present (lack of calling updater.update()).
> 
> bool CCTiledLayerImpl::hasTileAt(int i, int j) const { return m_tiler->tileAt(i, j); }

Ah, right.  isValid would still succeed even if textureId is zero.  The texture doesn't care about whether it was allocated or not, just whether its token has been evicted yet.

[WebKit Review Bot]

Comment on attachment 133619 [details]
Patch

Clearing flags on attachment: 133619

Committed r111978: <http://trac.webkit.org/changeset/111978>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Penner]

This was yummy to track down indeed! ;) It looks like this was a knock-on from the lost-invalidations fix and then this fix didn't quite make it into m18.   I'm going to cherry-pick it.