Assertion failure: GetApzc()->GetParent() == aParent, at /buildssrc/gfx/layers/apz/src/HitTestingTreeNode.cpp:406
Categories
(Core :: Layout: Scrolling and Overflow, defect, P3)
Tracking
()
People
(Reporter: tsmith, Assigned: botond)
References
(Blocks 2 open bugs)
Details
(Keywords: assertion, testcase, Whiteboard: [gfx-noted])
Attachments
(3 files, 1 obsolete file)
|
||
Comment 1•7 years ago
|
||
|
||
Comment 2•7 years ago
|
||
|
Assignee | |
Comment 3•7 years ago
|
||
|
|
Assignee | |
Comment 4•7 years ago
|
||
|
Reporter | |
Updated•7 years ago
|
|
|
Reporter | |
Comment 6•5 years ago
|
||
A Pernosco session is available here: https://pernos.co/debug/GTLHN-tvOPNXYaPFl9fZag/index.html
|
|
Reporter | |
Comment 7•5 years ago
|
||
A Pernosco session is available: https://pernos.co/debug/OEK6hzTUCrHUK9zvAckkyg/index.html
This has layout.display-list.dump-content=true as requested.
|
|
Assignee | |
Comment 9•5 years ago
•
|
||
Got a fresh display list dump from the Pernosco recording.
Analysis so far, based on IDs from this dump:
- We have a container layer 0x55d747051e80 which has three scroll metadatas, for scroll ids 7, 6, and 5.
- We have a container layer 0x55d746c40420 which has one scroll metadata, for scroll id 6.
- These are siblings, their parent is a container layer with scroll metadata for scroll id 4.
- The first layer's branch in the scroll node tree yields the scroll parent chain 7 -> 6 -> 5 -> 4, the second layer's branch yields the chain 6 -> 4. We get an assertion because 6 has conflicting scroll parents.
- Both container layers come from
nsDisplayScrollInfoLayerdisplay items. For such items, the bottom-most metadata comes from the item'smScrollFrame, and the remaining ones are based on the item's ASR chain. - Looking at the two items' ASR chains, they are indeed (6, 5, 4) and (4), respectively. This seems wrong; the second item's ASR chain should be (5, 4).
|
||
Comment 10•5 years ago
|
||
From talking to Botond it sounds like hitting this code
leads to this situation. Since it can cause other things to go wrong maybe we should be recording some data, maybe with telemetry (or gfx critical note maybe if its rare/bad enough), about how often this happens.
|
|
Assignee | |
Comment 11•5 years ago
|
||
To summarize what happens here (continuing to use the IDs from comment 9):
- When display list building enters (5) (by which I mean, the scroll frame with scroll id 5), that scroll frame is not active, but its descendant (6) is.
- In such situations, items scrolled by (5) get assigned the enclosing ASR, which in this case is (4).
- Items scrolled by descendant ASRs, such as (6), get assigned that ASR. If you were to query the ASR chain of such an item at this time, you'd get
(6, 4)(note that (5) missing). Note that the item only has a pointer to its own ASR; that ASR then points to its parent, and so on, to form the chain. - When display list building leaves (5), we realize (5) actually needs to be active (because it has an active descendant), and we take the
InsertScrollFramecode path that Timothy linked to. This tries to "fix up" ASR chains, by e.g. "reparenting" (6) such that its parent is now (5), not (4). As a result, when the ASR chain of an item scrolled by (6) is queried, it is now the correct (6, 5, 4). However, nothing fixes up items which should have gotten (5) as their ASR but actually got (4); those remain with an incorrect ASR.
I can't think of a simple way to fix InsertScrollFrame to address situations like this. (Markus, if you have ideas, please let me know.)
However, having to activate a parent scroll frame because it has an active descendant is a fairly common scenario. So why does this bug not occur more often? The answer is that we have another mechanism to activate the ancestors of a scroll frame when we activate the scroll frame: by setting a zero-margin displayport on the ancestors. This happens between paints (usually in response to events), such that by the next paint we know the relevant scroll frames are active by the time display list building enters them.
Why does that mechanism fail in this test case? Because the method our displayport-setting code uses to determine a scroll frame's ancestor is not 100% accurate, i.e. it does not always match what the actual scroll parent will be during display list building. In this test case in particular, the descendant in question is an out-of-flow frame. The actual scroll parent ends up being an ancestor of its placeholder frame, while our displayport-setting code just looks at direct ancestors in the frame tree.
I have a candidate fix that improves our displayport-setting code to navigate to placeholder frames.
One thing that's not clear to me is along the lines of what Timothy pointed out in comment 10: is the InsertScrollFrame codepath ever really necessary, or is it just a fallback for cases where the displayport-based activation mechanism is buggy or fails for some reason?
|
|
Assignee | |
Comment 12•5 years ago
|
||
This ensures that the notion of a scroll frame's scrollable ancestor used in
SetZeroMarginDisplayPortOnAsyncScrollableAncestor() to activate ancestors
when activating a scroll frame, matches what the actual ancestor is
according to display list building logic.
This avoids us taking the buggy "activate a scroll parent after the fact"
codepath (AutoCurrentActiveScrolledRootSetter::InsertScrollFrame()), which
can result in a display list with incorrect ASRs, in at least some cases.
|
||
Comment 13•5 years ago
|
||
|
||
Comment 14•5 years ago
|
||
| bugherder | ||
|
||
Comment 15•5 years ago
|
||
(In reply to Botond Ballo [:botond] [away until Jan 4] from comment #11)
Why does that mechanism fail in this test case? Because the method our displayport-setting code uses to determine a scroll frame's ancestor is not 100% accurate, i.e. it does not always match what the actual scroll parent will be during display list building. In this test case in particular, the descendant in question is an out-of-flow frame. The actual scroll parent ends up being an ancestor of its placeholder frame, while our displayport-setting code just looks at direct ancestors in the frame tree.
Hmm, I actually find this a bit surprising - I would have thought that ignoring placeholder frames would give the correct ASR chain. For example:
<scrollbox id="A">
<div style="position:absolute">
<scrollbox id="B"></scrollbox>
</div>
</scrollbox>
Here, the ASR tree should be
- root
- A
- B
i.e. A and B should be siblings. Activating B does not need to activate A, because B hands off to the root scroll frame.
|
||
Comment 16•5 years ago
|
||
The test here makes us trip a fatal assertion in test-verify mode, apparently. See bug 1683842.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 17•5 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
From talking to Botond it sounds like hitting this code
leads to this situation. Since it can cause other things to go wrong maybe we should be recording some data, maybe with telemetry (or gfx critical note maybe if its rare/bad enough), about how often this happens.
Based on this Try push, it at least doesn't happen in our test suite. We could explore adding telemetry or a gfx critical note to learn more about occurrences in the wild if you think that's worth it.
|
|
Assignee | |
Comment 18•5 years ago
|
||
ni?myself as a reminder to look into comment 15
|
|
||
Comment 19•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #17)
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
From talking to Botond it sounds like hitting this code
leads to this situation. Since it can cause other things to go wrong maybe we should be recording some data, maybe with telemetry (or gfx critical note maybe if its rare/bad enough), about how often this happens.
Based on this Try push, it at least doesn't happen in our test suite. We could explore adding telemetry or a gfx critical note to learn more about occurrences in the wild if you think that's worth it.
It seems worth it since it can lead to bad situations. I filed bug 1685468.
|
|
||
Comment 20•5 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #15)
(In reply to Botond Ballo [:botond] [away until Jan 4] from comment #11)
Why does that mechanism fail in this test case? Because the method our displayport-setting code uses to determine a scroll frame's ancestor is not 100% accurate, i.e. it does not always match what the actual scroll parent will be during display list building. In this test case in particular, the descendant in question is an out-of-flow frame. The actual scroll parent ends up being an ancestor of its placeholder frame, while our displayport-setting code just looks at direct ancestors in the frame tree.
Hmm, I actually find this a bit surprising - I would have thought that ignoring placeholder frames would give the correct ASR chain. For example:
<scrollbox id="A"> <div style="position:absolute"> <scrollbox id="B"></scrollbox> </div> </scrollbox>Here, the ASR tree should be
- root
- A
- B
i.e. A and B should be siblings. Activating B does not need to activate A, because B hands off to the root scroll frame.
This comment is correct. The patch here made SetZeroMarginDisplayPortOnAsyncScrollableAncestor match mScrollParentId on the ScrollMetadata that we send to the compositor. So the ASR tree and mScrollParentId on the ScrollMetadata are different.
It seems like we might be able to make these match so I filed bug 1693451.
|
|
Assignee | |
Comment 21•5 years ago
|
||
Summarizing some discussions around this on chat:
There are at least three places where we compute some notion of a "scrollable ancestor":
- Parent links in the ASR chain.
mScrollParentIdin the scroll metadata.GetAsyncScrollableAncestorFrame(with the important call site beingSetZeroMarginDisplayPortOnAsyncScrollableAncestors)
With that in mind:
- There was (and still is) a pre-existing discrepancy between #1 (ignores placeholder frames) and #2 (follows placeholder frames).
- #3 used to be consistent with #1 (i.e. ignore placeholder frames), and in this bug we changed it to be consistent with #2 (i.e. follow placeholder frames) instead.
- It's important for #3 to be consistent with #2, to avoid relying on the buggy
InsertScrollFramecodepath which can cause the assertion in the bug title. - The actual behaviour we want is #1, i.e. we'd like to resolve the pre-existing discrepancy by changing #2 (and #3 along with it) to behave like #1. Bug 1693451 tracks this.
- (Mentioning for completeness since this confused me: in this testcase, prior to the fix, we did see an ASR chain that followed placeholder frames. The reason for this was that we were getting into the buggy
InsertScrollFramecodepath which "fixes up" ASR chains. The entry into this codepath is governed by scroll parent logic, so we ended up with an ASR chain that follows a placeholder.)
Conclusion: there is nothing further to do here besides fixing the pre-existing issue in bug 1693451.
Description
•