Validate lowest float cache when necessary.
FloatingObjects has a method LowestFloatLogicalBottom(), which calculates and caches the lowest FloatingObject if this information isn't already calculated and cached. Then there's the method LowestFloatingObject(), which would return that FloatingObject, if it was cached, and nullptr otherwise. So: One method that calculates it if needed, and one that relies on the cache and otherwise returns something bogus. The layout code is quite well peppered with calls to LowestFloatLogicalBottom(), so it requires significant amounts of bad luck to trigger a cache miss, and then another heap of bad luck to actually cause a crash from that. But it was NOT impossible. The fix is to rebuild the cache if needed in LowestFloatingObject(). Letting it return nullptr if the cache was invalid caused us to fail to propagate an overhanging float upon relayout, breaking some sort of link to subsequent blocks, so that when the float later on was removed, we'd fail to remove it from all the block flows that referred to it. The test included in this CL is as minimal as I could get it, but still not super-simple. Here's what went wrong: There is a float somewhat nested inside a container. This container has some siblings, one being an infinitely tall beast with a large negative (also infinite) block-start margin. Another sibling is empty, so that we can collapse through it. Then there's a last, pretty decent, sibling container, except that it contains another float. Because we're dealing with infinite LayoutUnit values, the engine cannot quite agree with itself whether the first float intrudes into the last sibling or not, and this is important when it comes to determining whether the float intrudes into the last container or not. When we lay out initially, we find it to intrude, and add it to the list. We then change the first float ever so slightly, to trigger relayout. This will break the magical chain of FloatingObject references to the first float (because the cache was invalid, and therefore incorrectly report that there's no float there). The last container (the one previously referred to as "decent") has a reference to the float that just got relaid out, because at some point it was thought to intrude. If we had laid out this container again now, we'd clear the FloatingObject list and the float would be gone from the list. But we don't lay it out again, because the relevant part of the engine decides that the float doesn't intrude, contrary to what the part of the engine that caused it to be added in the first place concluded. Then, we'll remove the float (display:none), and we'll fail to remove it from all block flows that referenced it. The last container still has a reference to the hungover (or actually now dead) float. Bug: 989305 Change-Id: I498344fc5b6426cf8225441d2dab195659e31112 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1738556 Commit-Queue: Emil A Eklund <eae@chromium.org> Reviewed-by:Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#684357}
Showing
Please register or sign in to comment