1750850 - LayerActivityTracker timers firing every 100ms forever on phabricator tabs

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Florian Quèze [:florian]]

See this profile: https://share.firefox.dev/3Knrefp

The 4 content processes included in the profile had phabricator tabs (eg https://phabricator.services.mozilla.com/D133599).

A quick look at the code gives me the impression that the LayerActivityTracker timer is expected to run 4 times before things get cleaned up, but it seems to run forever in these processes.

Comment 1

[Timothy Nikkel (:tnikkel)]

I tried to reproduce in a fresh profile and I was not able to. I logged in to my phab account in case that was needed. I put a printf when we called MarkUsed in the layer activity tracker and also checked the profile to see.

Comment 2

[Florian Quèze [:florian]]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

I tried to reproduce in a fresh profile and I was not able to.

I just tried to reproduce on my local build, and couldn't.

In my normal Nightly where I captured the profile from comment 0, I captured another profile where I close a phabricator tab (that was the only time in the process), then reopen it (it opens in a new process) then close it again. We see the LayerActivityTracker runnable markers in the new process too.
https://share.firefox.dev/3nEKfAe

Something else that seems abnormal in the profile is that I have "Composite#1" markers at 60Hz in the Renderer thread on the parent process, despite nothing being animated or repainted. That seems like bug 1690673, but it may be related.

Comment 3

[Timothy Nikkel (:tnikkel)]

Assuming we keep hitting this line

https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/layout/painting/ActiveLayerTracker.cpp#221

I would debug why it's getting called.

Comment 4

[Timothy Nikkel (:tnikkel)]

Worked with Florian on this.

I think we are hitting this case

https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/layout/painting/ActiveLayerTracker.cpp#184

in NotifyExpired so the expiry tracker is never empty. And the reason mAnimatingScrollHandlerFrame is always alive and non-null is that it seems to only get cleared if the frame is destroyed of the scroll frame becomes inactive

https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/layout/painting/ActiveLayerTracker.cpp#426

Some scrollframes never become inactive (the root scroll frame, a scroll frame with will-change: scroll, if the root scroll frame isn't scrollable we give the first scrollable element we encounter a displayport and that will keep it active). So this seems to be a bug in the active layer tracker. We need to make sure these expire even if the scrollframe stays active. I'll have to think on what a proper fix is.

Comment 5

[Florian Quèze [:florian]]

Attachment: Debug profiler markers

To debug this, I added debug profiler markers where Timothy told me they would help.

I was lucky enough to capture a profile where we see the beginning of the bug happening: https://share.firefox.dev/3rzudIW During this profile, we do 113 AddObject, 17 MarkUsed and 112 RemoveObject calls.

Apparently to reproduce this we need the page to do style changes from a scroll event handler, which is what we see happening in this profile.

Comment 6

[Markus Stange [:mstange]]

Now that we have WR everywhere, we could just revert bug 1201330. I think that's the patch which introduced the code that's causing this bug.

Comment 7

[Markus Stange [:mstange]]

I think these days, layer activeness is only used by the SVG layerization code.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1750850. Remove the code that tracks "active layers" from changes made in scroll handlers. r?mstange

This was added in bug 1201330 but with WR isn't really needed anymore and the code causes the expiration timer to fire until the scroll frame becomes inactive (the most common scroll frames stay active, ie root scroll frames).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9885119c0e47
Remove the code that tracks "active layers" from changes made in scroll handlers. r=mstange

Comment 10

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/9885119c0e47