Closed Bug 1449756 Opened 2 years ago Closed Last year

SuppressDisplayport should be tracked by PresShells and not globally

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

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.
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.
Thank you for filing this!
Priority: -- → P3
Whiteboard: [gfx-noted]
Blocks: 1457276
Originally, DisplayPort suppression was a process-global static. This change makes it possible
to control DisplayPort suppression on a per-PresShell basis.
Attachment #8986919 - Attachment description: Bug 1449756 - Make DisplayPort suppression PresShell specific and not process global. r?kats → [WIP] Bug 1449756 - Make DisplayPort suppression PresShell specific and not process global. r?kats
Attachment #8986919 - Attachment description: [WIP] Bug 1449756 - Make DisplayPort suppression PresShell specific and not process global. r?kats → Bug 1449756 - Make DisplayPort suppression PresShell specific and not process global. r?kats
Comment on attachment 8986919 [details]
Bug 1449756 - Make DisplayPort suppression PresShell specific and not process global. r?kats

Kartikaya Gupta (email:kats@mozilla.com) has approved the revision.

https://phabricator.services.mozilla.com/D1759
Attachment #8986919 - Flags: review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/987d0ed1d6f9
Make DisplayPort suppression PresShell specific and not process global. r=kats
https://hg.mozilla.org/mozilla-central/rev/987d0ed1d6f9
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → mconley
Is it possible this is related to the recently-filed bug 1471107? It seems the assert was touched in this bug.
Flags: needinfo?(mconley)
Yeah, very likely. Needinfo'ing myself on that bug instead.
Flags: needinfo?(mconley)
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.
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Blocks: 1485023
You need to log in before you can comment on or make changes to this bug.