Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-10-09 11:25:00 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1677568. Handle a rare case of a placeholder and its out of flow frame not being in the same continuation of it's containing block frame in retained display list code. r=mstange

Browse Source 
This testcase hits a rare condition where the abs pos frame is under the first continuation of the containing block frame, but the placeholder frame is under the next continuation of th containing block frame. This means that the walk in FindContainingBlocks which adds the ForceDescendIntoIfVisible bit to all ancestors of modified frames (walking through placeholders) never marks the first continuation of the containing block frame, which is the actual frame that contains the modified frame. That means that we don't walk into the containing block for the modified abs pos frame, and so we don't call MarkAbsoluteFramesForDisplayList, which computes the dirty rect for the abs pos frame, and if it is non-empty it would walk it's ancestor chain through placeholders and add the NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO bit. But we don't do that, so when we come to build the display list for the placeholder frame, the placeholder frame has size 0x0 of course, so we determine it is not visible, and even though the ForceDescendIntoIfVisible bit is set on it, since it's not visible we don't descend into it. We need the NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO bit which would have been set.

So for this to work we need to make sure in addition to the ancestor chain walking through placeholders has the ForceDescendIntoIfVisible bit set, we also need to make sure the normal GetParent ancestor chain has that bit set. So anywhere that we use GetDisplayListParent where this kind of fix seems like it is needed we need to modify. We try to be a bit clever to try to avoid any extra work for this rare case. We do this by checking if we are going to be jumping to the placeholder, and if so we make sure to mark the parent of the current frame, before continuing up the GetDisplayListParent walk. This doesn't visit anymore frames as in the common case we stop the walk when we get to the parent frame because the ForceDescendIntoIfVisible bit is set on it. This does add a function call overhead though.

When we hit this condition we do have to walk more frames. To make sure this condition isn't too common, and thus aren't making ourselves walk a lot more frames, I did a full try run on linux to see how often it was hit. We hit it 3 times. layout/base/crashtests/1464641.html which is a reduced testcase of another retained display list bug. testing/web-platform/tests/svg/layout/svg-with-precent-dimensions-relayout.html. And layout/reftests/display-list/1709452-1.html which is another retained display list bug, which also has to do with continuations and it is almost an identical copy of the reduced testcase of this bug.

Differential Revision: https://phabricator.services.mozilla.com/D174458
This commit is contained in:
Timothy Nikkel 2023-04-12 10:52:05 +00:00
parent b80305abbb
commit 5fe231c04b
6 changed files with 125 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
layout/base/nsLayoutUtils.cpp
View File

@ -4115,6 +4115,9 @@ nsIFrame* nsLayoutUtils::GetNonGeneratedAncestor(nsIFrame* aFrame) {
}
nsIFrame* nsLayoutUtils::GetParentOrPlaceholderFor(const nsIFrame* aFrame) {
// This condition must match the condition in FindContainingBlocks in
// RetainedDisplayListBuider.cpp, MarkFrameForDisplayIfVisible and
// UnmarkFrameForDisplayIfVisible in nsDisplayList.cpp
if (aFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW) &&
!aFrame->GetPrevInFlow()) {
return aFrame->GetProperty(nsIFrame::PlaceholderFrameProperty());

17
layout/painting/RetainedDisplayListBuilder.cpp
View File

@ -1186,6 +1186,23 @@ static void FindContainingBlocks(nsIFrame* aFrame,
aExtraFrames);
AddFramesForContainingBlock(f, f->GetChildList(f->GetAbsoluteListID()),
aExtraFrames);
// This condition must match the condition in
// nsLayoutUtils::GetParentOrPlaceholderFor which is used by
// nsLayoutUtils::GetDisplayListParent
if (f->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW) && !f->GetPrevInFlow()) {
nsIFrame* parent = f->GetParent();
if (parent && !parent->ForceDescendIntoIfVisible()) {
// If the GetDisplayListParent call is going to walk to a placeholder,
// in rare cases the placeholder might be contained in a different
// continuation from the oof. So we have to make sure to mark the oofs
// parent. In the common case this doesn't make us do any extra work,
// just changes the order in which we visit the frames since walking
// through placeholders will walk through the parent, and we stop when
// we find a ForceDescendIntoIfVisible bit set.
FindContainingBlocks(parent, aExtraFrames);
}
}
}
}

47
layout/painting/nsDisplayList.cpp
View File

@ -818,14 +818,31 @@ void nsDisplayListBuilder::AddFrameMarkedForDisplayIfVisible(nsIFrame* aFrame) {
mFramesMarkedForDisplayIfVisible.AppendElement(aFrame);
}
void nsDisplayListBuilder::MarkFrameForDisplayIfVisible(
nsIFrame* aFrame, const nsIFrame* aStopAtFrame) {
AddFrameMarkedForDisplayIfVisible(aFrame);
static void MarkFrameForDisplayIfVisibleInternal(nsIFrame* aFrame,
const nsIFrame* aStopAtFrame) {
for (nsIFrame* f = aFrame; f; f = nsLayoutUtils::GetDisplayListParent(f)) {
if (f->ForceDescendIntoIfVisible()) {
return;
}
f->SetForceDescendIntoIfVisible(true);
// This condition must match the condition in
// nsLayoutUtils::GetParentOrPlaceholderFor which is used by
// nsLayoutUtils::GetDisplayListParent
if (f->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW) && !f->GetPrevInFlow()) {
nsIFrame* parent = f->GetParent();
if (parent && !parent->ForceDescendIntoIfVisible()) {
// If the GetDisplayListParent call is going to walk to a placeholder,
// in rare cases the placeholder might be contained in a different
// continuation from the oof. So we have to make sure to mark the oofs
// parent. In the common case this doesn't make us do any extra work,
// just changes the order in which we visit the frames since walking
// through placeholders will walk through the parent, and we stop when
// we find a ForceDescendIntoIfVisible bit set.
MarkFrameForDisplayIfVisibleInternal(parent, aStopAtFrame);
}
}
if (f == aStopAtFrame) {
// we've reached a frame that we know will be painted, so we can stop.
break;
@ -833,6 +850,13 @@ void nsDisplayListBuilder::MarkFrameForDisplayIfVisible(
}
}
void nsDisplayListBuilder::MarkFrameForDisplayIfVisible(
nsIFrame* aFrame, const nsIFrame* aStopAtFrame) {
AddFrameMarkedForDisplayIfVisible(aFrame);
MarkFrameForDisplayIfVisibleInternal(aFrame, aStopAtFrame);
}
void nsDisplayListBuilder::SetGlassDisplayItem(nsDisplayItem* aItem) {
// Web pages or extensions could trigger the "Multiple glass backgrounds
// found?" warning by using -moz-appearance:win-borderless-glass etc on their
@ -973,6 +997,23 @@ static void UnmarkFrameForDisplayIfVisible(nsIFrame* aFrame) {
return;
}
f->SetForceDescendIntoIfVisible(false);
// This condition must match the condition in
// nsLayoutUtils::GetParentOrPlaceholderFor which is used by
// nsLayoutUtils::GetDisplayListParent
if (f->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW) && !f->GetPrevInFlow()) {
nsIFrame* parent = f->GetParent();
if (parent && parent->ForceDescendIntoIfVisible()) {
// If the GetDisplayListParent call is going to walk to a placeholder,
// in rare cases the placeholder might be contained in a different
// continuation from the oof. So we have to make sure to mark the oofs
// parent. In the common case this doesn't make us do any extra work,
// just changes the order in which we visit the frames since walking
// through placeholders will walk through the parent, and we stop when
// we find a ForceDescendIntoIfVisible bit set.
UnmarkFrameForDisplayIfVisible(f);
}
}
}
}

26
layout/reftests/display-list/1677568-1-ref.html Normal file
View File

@ -0,0 +1,26 @@
<!DOCTYPE html>
<html>
<head>
<style>
a:hover { color: rgb(0, 86, 179); }
.d-relative { position: relative; }
.d-inline { display: inline;}
.d-flex { display: flex; }
</style>
</head>
<body>
<div class="d-flex">
<div>
<div class="d-inline d-relative">
<a href="#">
--
</a>
<div style="position: absolute; transform: translate(0px, 160px);">
<a style="color: rgb(0, 86, 179);" href="#">i vanish on-hover in Firefox</a>
</div>
</div>
</div>
</div>
</body></html>

34
layout/reftests/display-list/1677568-1.html Normal file
View File

@ -0,0 +1,34 @@
<!DOCTYPE html>
<html class="reftest-wait">
<head>
<style>
a:hover { color: rgb(0, 86, 179); }
.d-relative { position: relative; }
.d-inline { display: inline;}
.d-flex { display: flex; }
</style>
</head>
<body>
<div class="d-flex">
<div>
<div class="d-inline d-relative">
<a href="#">
--
</a>
<div style="position: absolute; transform: translate(0px, 160px);">
<a id="target" href="#">i vanish on-hover in Firefox</a>
</div>
</div>
</div>
</div>
<script>
function doTest() {
var elem = document.getElementById("target");
elem.style.color = "rgb(0, 86, 179)";
document.documentElement.removeAttribute("class");
}
document.addEventListener("MozReftestInvalidate", doTest);
</script>
</body></html>

1
layout/reftests/display-list/reftest.list
View File

@ -55,5 +55,6 @@ fuzzy(0-2,0-40000) skip-if(!asyncPan) == 1464288-1.html 1464288-ref.html
fuzzy-if(swgl,0-1,0-1) == 1551053-1.html 1551053-1-ref.html
== 1553828-1.html 1553828-1-ref.html
fuzzy-if(browserIsFission,0-1,0-300) == 1619370-1.html 1619370-1-ref.html
== 1677568-1.html 1677568-1-ref.html
== 1709452-1.html 1709452-ref.html
== 1719346-1.html 1719346-1-ref.html