Closed Bug 1554985 Opened 5 years ago Closed 5 years ago

document splitting + RDL chrome: Page jumps up when hovering a different tab

Categories

(Core :: Graphics: WebRender, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- disabled

People

(Reporter: jan, Assigned: kats)

References

Details

(Keywords: nightly-community)

Attachments

(4 files)

Attached video 2019-05-28_15-55-18.mp4

Debian Testing, KDE, X11, Macbook Pro
Document splitting depends on bug 1413546, so I tested it.
Scrolled down by touchpad.

mozregression --launch 2019-05-28 --pref gfx.webrender.all:true gfx.webrender.split-render-roots:true layout.display-list.retain.chrome:true -a https://bugzilla.mozilla.org -a https://hg.mozilla.org/try/rev/2b6744c368170b768d913ef443407eb5f716ff59

Not reproducible if either document splitting or RDL chrome is disabled.

Same on Win10, GTX1060.

OS: Linux → All
Hardware: x86_64 → All
Priority: -- → P3

For clarity - this only seems to happen with both document splitting and parent process RDL enabled.

No longer blocks: 1413546
See Also: → 1557761
See Also: 1557761

I can repro on Windows, will investigate. At first glance it's also related to paint skipping, since I can't reproduce with apz.paint_skipping.enabled=false. That also explains why it jumps up just a little bit - it's probably reverting to the last un-paint-skipped scroll position.

Assignee: nobody → kats
Status: NEW → ASSIGNED

Looking into this a bit deeper. Looks like when the tooltip appears on the background tab, it destroys the APZC tree and all the APZC instances. Then the subsequent transaction rebuilds it. But the rebuild (which presumably comes from a retained display list in the UI process) has the "old" scroll offset, because paint skipping means that was the last one that was baked into the display list.

The next question is why does the tooltip destroy/rebuild the APZC tree. This doesn't seem to happen with WR disabled, and I believe tooltips should get their own widgets and so would have a separate APZCTreeManager, if they have one at all. Probably somewhere in the document splitting code we're getting wires crossed and sending a DLs from one widget to another widget, or something along those lines.

More investigation debunked my theory about wires getting crossed. What's actually happening is that around the same time the tooltip shows up, the main window has another transaction, but the render root boundary object that's supposed to be persisted from one transaction to the next isn't persisted. In code terms, this call to EnsureHasBoundary emplaces a new boundary instead of reusing the old one (because the old one was destroyed for reasons I have yet to uncover). Because the boundary id is different, walking the WebRenderScrollDataWrapper tree on the compositor side doesn't work quite as expected because the chrome document's scene build finishes before the content document's scene build (i.e. we go into this codepath). This results in APZ getting a truncated tree which results in the destroy/rebuild in my last comment.

Ah, with retained display list building we apparently explicitly mark render roots as needing a build if they are invalidated here. And if they aren't marked as needing a build, they get skipped over here because the condition forces an early-exit. This causes the WebRenderUserData attached to the node to not be touched, and if it's not touched, it gets recycled at the end of the transaction. That triggers the new boundary creation on the next transaction. I think the whole tooltip thing was probably a red herring, the more important thing is that the background tab gets a highlight when you mouse over it which triggers the transaction sans content document and destroys the WebRenderUserData.

With retained display lists, a content render root might get marked as not
needing a build, in which case the nsDisplayRenderRoot::CreateWRCommands
function does an early exit. In this case, we don't mark the associated
WebRenderUserData as used during the display list build, which causes it to
get deleted at the end of the transaction. The next transaction that
doesn't early-exit will re-create the WebRenderUserData with a new boundary
object. The compositor therefore thinks it's a brand new thing and, if
conditions are right, could end up destroying and re-creating much of the
APZC tree. That in turn can have effects like discarding paint-skipped
scrolling.

This patch ensures we always touch the WebRenderUserData during the display
list build, so we don't discard it. This problem may still affect nested
nsDisplayRenderRoot instances but I don't think we ever cases where those
occur.

Depends on D36386

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8edcb5f6ceff
Invert condition to reduce indentation. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/e16eda1824f7
Dump the render root boundary information when dumping WR scroll data. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/5ebb4441aa24
Prevent the WebRenderUserData on the render root item from getting discarded prematurely. r=jrmuizel
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: