1255054 - Possible to get perma-checkerboarded after resizing window

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

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

While trying to reproduce bug 1250422 I ran into a scenario that can result in perma-checkerboarding. On Windows for example, if you have a window and resize it to be shorter, the displayport suppression kicks in and makes the displayport smaller. After you're done resizing, scroll down (mousewheel with APZ). Now the paint-skipping kicks in and avoids scheduling paints because it thinks you're still in the displayport - it uses the non-suppressed displayport check but the last painted displayport was a suppressed one. So you can scroll right into checkerboarding without layout scheduling a paint. If you scroll far enough eventually layout will schedule a paint but it's possible that on short pages that never happens.

This is a combination of effects from bug 1186662 and bug 1187432 (aggravated by bug 1192910).

Comment 1

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

If we schedule a paint after unsuppressing, as per https://bugzilla.mozilla.org/show_bug.cgi?id=1202503#c10, that should fix this problem.

Comment 2

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

Actually I'll put the patch on this bug rather than bug 1202503 since that one might have other issues mixed in.

Comment 3

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

Attachment: Schedule a paint after unsuppressing so we don't get stuck with a suppressed displayport

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8728571 [details] [diff] [review]
Schedule a paint after unsuppressing so we don't get stuck with a suppressed displayport

Review of attachment 8728571 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +2381,5 @@
>        child->Destroy();
>    }
>  
>    while (mActiveSuppressDisplayport > 0) {
> +    APZCCallbackHelper::SuppressDisplayport(false, nullptr);

This might be a reason to split SuppressDisplayport into SuppressDisplayport and UnsuppressDisplayport since it doesn't need a preshell. I'll leave that up to you.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +919,5 @@
>    if (aEnabled) {
>      sActiveSuppressDisplayport++;
>    } else {
>      sActiveSuppressDisplayport--;
> +    if (sActiveSuppressDisplayport == 0 && aShell && aShell->GetRootFrame()) {

Do we want to allow unsupressing without a shell? This would lead to this bug again in a situation that might be harder to reproduce?

If we have a good reason than perhaps it should be documented. Otherwise this should be an assert.

Comment 5

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

Comment on attachment 8728571 [details] [diff] [review]
Schedule a paint after unsuppressing so we don't get stuck with a suppressed displayport

Review of attachment 8728571 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +919,5 @@
>    if (aEnabled) {
>      sActiveSuppressDisplayport++;
>    } else {
>      sActiveSuppressDisplayport--;
> +    if (sActiveSuppressDisplayport == 0 && aShell && aShell->GetRootFrame()) {

Ideally no, but one of the callsites is from TabChild::RecvDestroy - it might not have a presShell at that point, and even if it did, calling SchedulePaint on it will likely not have any real effect. So in that case I figured it was ok to be passing in null. I can document though that we really want a presShell as much as possible.

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8728571 [details] [diff] [review]
Schedule a paint after unsuppressing so we don't get stuck with a suppressed displayport

Review of attachment 8728571 [details] [diff] [review]:
-----------------------------------------------------------------

r+, a comment would be nice.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7653f68de6

Comment 8

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

Added a comment in the header and landed. There might be some talos fallout from this, but hopefully it will be negligible.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c7653f68de6

Comment 10

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

Comment on attachment 8728571 [details] [diff] [review]
Schedule a paint after unsuppressing so we don't get stuck with a suppressed displayport

Approval Request Comment
[Feature/regressing bug #]: combination of bug 1186662 and bug 1187432 when APZ is enabled
[User impact if declined]: it's possible to get into a state with checkerboarding showing on the screen that doesn't clear itself. you have to scroll or switch tabs or something to force it to go away
[Describe test coverage new/current, TreeHerder]: tested locally, on m-c
[Risks and why]: small patch, pretty low risk
[String/UUID change made/needed]: none

Comment 11

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

[Tracking Requested - why for this release]: pretty bad user experience in a particular scenario with APZ enabled.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8728571 [details] [diff] [review]
Schedule a paint after unsuppressing so we don't get stuck with a suppressed displayport

regression fix in APZ, taking it.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/edcc2ad52ece

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f0b9e629896

Comment 16

[Petruta Horea [:phorea], Desktop QA]

I couldn't reproduce the bug as described in comment 0, so I tried to see the duplicate, bug 1202503: in an older Nightly I could see the flashing on the first scroll. This behavior doesn't reproduce on latest versions (46, 47, 48).

Still, there are two issues when resizing a window to be shorter: repaint (all platforms) - logged bug 1261355 and stretch content (specific to Windows) - tracked in bug 1256728 and bug 1251778.

Marking as verified based on this.