1449756 - SuppressDisplayport should be tracked by PresShells and not globally

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Display port suppression lets us constrain the area to paint to just what's visible in the window, and nothing outside. We sometimes suppress the display port to make painting faster - for example, when switching tabs.

The way suppression works, however, is a bit foot-gunny. You call APZCCallbackHelper::SuppressDisplayport passing in whether or not you want to suppress, and a PresShell.

APZCCallbackHelper then uses that information to increment or decrement a static, process-wide counter. If that counter > 0, then we're suppressing _all_ display ports. If it's 0, then we're not suppressing any.

So it's all or nothing.

What's more, when we're unsuppressing multiple display ports, only the last one will get a re-paint scheduled, since the last one is the one that reaches 0 (which triggers the repaint on the lucky PresShell to be passed in).

We should probably make display port suppression something that the PresShells keep track of, and lose the whole static thing.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I originally thought I'd need to do this for bug 1447193, but I found a simpler route. Having spoken to kats though, this sounds like a good thing to fix in general, so I wanted to make sure it was on file.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Thank you for filing this!

Comment 3

[Phabricator Automation]

Attachment: Bug 1449756 - Make DisplayPort suppression PresShell specific and not process global. r?kats

Originally, DisplayPort suppression was a process-global static. This change makes it possible
to control DisplayPort suppression on a per-PresShell basis.

Comment 4

[Phabricator Automation]

Comment on attachment 8986919 [details]
Bug 1449756 - Make DisplayPort suppression PresShell specific and not process global. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

https://phabricator.services.mozilla.com/D1759

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/987d0ed1d6f9
Make DisplayPort suppression PresShell specific and not process global. r=kats

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/987d0ed1d6f9

Comment 7

[:Gijs (he/him)]

Is it possible this is related to the recently-filed bug 1471107? It seems the assert was touched in this bug.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Yeah, very likely. Needinfo'ing myself on that bug instead.

Comment 9

[Mats Palmgren (inactive)]

Hmm, the pres shell should always outlive all its frames so you should
never write code like this:
  if (nsIPresShell* shell = mCallee->mOuter->PresShell())
If you found that these null-checks were necessary, then you're
wallpapering over a likely exploitable bug.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Ah. I don't recall needing them to avoid crashes or anything - I suspect I was trying to be defensive, and my unfamiliarity with the code is showing.

I'll make them asserts instead. Filed bug 1485023.