[chromium] When prepainting fails, tiles dirty rects may be cleared
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.
Created attachment 133617 [details] Patch
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.
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); }
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).
(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.
Comment on attachment 133619 [details] Patch Clearing flags on attachment: 133619 Committed r111978: <http://trac.webkit.org/changeset/111978>
All reviewed patches have been landed. Closing bug.
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.