179476 2017-11-09 05:37:36 -0800 [GTK][WPE] CoordinatedGraphicsLayer::setNeedsDisplayInRect() converts FloatRect to IntRect erroneously 2017-11-10 00:51:08 -0800 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 magomez magomez bugs-noreply buildbot cmarcelo commit-queue kondapallykalyan luiz noam zan oldest_to_newest 1369851 0 magomez 2017-11-09 05:37:36 -0800 I found this while digging into a rendering issue that is not reproducible upstream, but in a WPE stable branch downstream, and only in certain platforms (I was only able to reproduce it in MIPS, while the same code on ARM doesn't show the problem). Nevertheless I think this is a bug upstream as well and, despite I'm not able to reproduce with the test I'm using, I think it could be reproduced eventually. I'll try to explain what happens: - A page renders a div at position (100.5,100) with size 200x200 with a property that makes it have its own GraphicsLayer. - The CoordinatedGraphicsLayer gets created. The size and position of that GraphicsLayer gets calculated in RenderLayerBacking::computePrimaryGraphicsLayerRect(), which properly turns the float values into integers, setting the position to (100,100) and the size to 201x200 (notice that the resulting rect encloses the original one). - The CoordinatedGraphicsLayer will create a backingStore, then a Tile with the size set to the layer, and use an UpdateAtlas with size 201x200 - Then RenderBox will draw the div content using a rect with position (0.5,0) (the offset of the div inside the layer) and size 200x200, and everything works fine - Then for some reason the div is modified and needs to be repainted. - After the layout and rendering calculations, RenderLayerBacking::setContentsNeedDisplayInRect() calls CoordinatedGraphicsLayer::setNeedsDisplayInRect() passing a rect with position (0.5,0) and size 200x200, which is the rect really used by the div inside the CoordinatedGraphicsLayer that needs to be repainted. - CoordinatedGraphicsLayer::setNeedsDisplayInRect() turns the FloatRect into an IntRect using IntRect(FloatRect), turning the position from (0.5,0) to (0,0) and keeping the size to 200x200. Then calls invalidate on the backingStore and then the Tile, which will store that rect as the dirty one. - During the layer flush, the tile will update its contents when Tile::updateBackBuffer() gets called. For that it will allocate a new UpdateAtlas with the size of the dirty rect (200x200) - When drawing the div, RenderBox tries to draw a rectangle with position (0.5,0) and size 200x200, but that doesn't fit into the UpdateAtlas we are using (we would need it to be 201x200), so we are not able to draw the last column of the div, which is the glitch I found. As I said, I'm not able to reproduce the glitch upstream with the page I'm testing, because the RenderBox seems to get a size of 201x200 instead of 200x200 there, which causes the UpdateAtlas to have the appropriate size. But still, converting a FloatRect into an IntRect with IntRect(FloatRect) will generate a rect that, in almost all of the cases, will be smaller than the original one. I think the appropriate way to do it is by using enclosingIntRect(FloatRect). 1369852 1 326445 magomez 2017-11-09 06:33:32 -0800 Created attachment 326445 Patch 1369856 2 magomez 2017-11-09 06:48:08 -0800 I've just remembered that upstream the UpdateAtlas we use are always 1024x1024. They are not created just with the size requested as we do downstream. But the glitch could happen anyway, as the rect we are painting to inside the UpdateAtlas is smaller than the div we want to paint. 1370269 3 326445 commit-queue 2017-11-10 00:51:06 -0800 Comment on attachment 326445 Patch Clearing flags on attachment: 326445 Committed r224671: <https://trac.webkit.org/changeset/224671> 1370270 4 commit-queue 2017-11-10 00:51:08 -0800 All reviewed patches have been landed. Closing bug. 326445 2017-11-09 06:33:32 -0800 2017-11-10 00:51:06 -0800 Patch bug-179476-20171109153331.patch text/plain 1813 magomez U3VidmVyc2lvbiBSZXZpc2lvbjogMjI0NjI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2U5Mjg0MWZmZDQwOTQ1 MGQ3NWRlN2YxOTU1ZjJkMzQxNWVhNzVkNi4uYWQwYjBiMGNmMWNlY2M0Zjc0NWJiYTMyMjMyOWEw MTI4ZTc0NWVlNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE3LTExLTA5ICBNaWd1 ZWwgR29tZXogIDxtYWdvbWV6QGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dUS11bV1BFXSBDb29y ZGluYXRlZEdyYXBoaWNzTGF5ZXI6OnNldE5lZWRzRGlzcGxheUluUmVjdCgpIGNvbnZlcnRzIEZs b2F0UmVjdCB0byBJbnRSZWN0IGVycm9uZW91c2x5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzk0NzYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBDb252ZXJ0IHRoZSBwYXNzZWQgRmxvYXRSZWN0IGludG8g YW4gSW50UmVjdCB1c2luZyBlbmNsb3NpbmdJbnRSZWN0KCksIHNvIHdlIGFyZSBndWFyYW50ZWVk IHRoYXQKKyAgICAgICAgdGhlIHJlY3RhbmdsZSB0byBwYWludCBmaXRzIGludG8gdGhlIGJ1ZmZl ciB0aGF0IHRoZSBiYWNraW5nU3RvcmUgd2lsbCBhbGxvY2F0ZS4KKworICAgICAgICBObyBiZWhh dmlvdXIgY2hhbmdlLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvdGV4bWFwL2Nvb3Jk aW5hdGVkL0Nvb3JkaW5hdGVkR3JhcGhpY3NMYXllci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpD b29yZGluYXRlZEdyYXBoaWNzTGF5ZXI6OnNldE5lZWRzRGlzcGxheUluUmVjdCk6CisKIDIwMTct MTEtMDggIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KIAogICAgICAgICBB VlNhbXBsZUJ1ZmZlckdlbmVyYXRvciBsZWFrcyBzZWVuIG9uIGxlYWtzIGJvdApkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdGV4bWFwL2Nvb3JkaW5hdGVkL0Nv b3JkaW5hdGVkR3JhcGhpY3NMYXllci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy90ZXhtYXAvY29vcmRpbmF0ZWQvQ29vcmRpbmF0ZWRHcmFwaGljc0xheWVyLmNwcAppbmRl eCAwMjI3NjdiYjBiZjUxZmFjN2U1NjBlNzNkYmUxY2UwZmE5ZmIxODMwLi41NTIyYTNkMTJiYzVi YTdmZGNjZTA5ZmY1ODU3NTA5MjEyMmQ3NzIxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9ncmFwaGljcy90ZXhtYXAvY29vcmRpbmF0ZWQvQ29vcmRpbmF0ZWRHcmFwaGljc0xh eWVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy90ZXhtYXAvY29v cmRpbmF0ZWQvQ29vcmRpbmF0ZWRHcmFwaGljc0xheWVyLmNwcApAQCAtNTMyLDcgKzUzMiw3IEBA IHZvaWQgQ29vcmRpbmF0ZWRHcmFwaGljc0xheWVyOjpzZXROZWVkc0Rpc3BsYXkoKQogdm9pZCBD b29yZGluYXRlZEdyYXBoaWNzTGF5ZXI6OnNldE5lZWRzRGlzcGxheUluUmVjdChjb25zdCBGbG9h dFJlY3QmIHJlY3QsIFNob3VsZENsaXBUb0xheWVyKQogewogICAgIGlmIChtX21haW5CYWNraW5n U3RvcmUpCi0gICAgICAgIG1fbWFpbkJhY2tpbmdTdG9yZS0+aW52YWxpZGF0ZShJbnRSZWN0KHJl Y3QpKTsKKyAgICAgICAgbV9tYWluQmFja2luZ1N0b3JlLT5pbnZhbGlkYXRlKGVuY2xvc2luZ0lu dFJlY3QocmVjdCkpOwogCiAgICAgZGlkQ2hhbmdlTGF5ZXJTdGF0ZSgpOwogCg==