1534459 - Avoid nested async zoom containers

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

We can currently get nested async zoom containers in some scenarios, namely OOP iframes, and desktop zooming + RDM. (See bug 1533607 comment 3 for details.)

We don't want nested async zoom containers, as they upset APZ assumptions.

To solve this:

• For OOP iframes, we don't want iframes to build an async zoom container, only the top-level content document.

• For desktop zooming + RDM, we do want the document inside the RDM to get an async zoom container, but not the enclosing document.

Once we implement the above changes, the check added in bug 1533607 can be made stricter by removing the escape hatch for nested async zoom containers.

Comment 1

[Botond Ballo [:botond]]

This doesn't need to block containerless scrolling, but it does need to block OOP iframes and desktop zooming.

Comment 2

[Botond Ballo [:botond]]

(I haven't come across a scenario where we can get multiple async zoom containers which are not nested, but that's something we don't want either, and should include in our check.)

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

You may know of this, but nsPresContext::IsRootContentDocumentCrossProcess() might be helpful here [1].

[1] https://searchfox.org/mozilla-central/source/layout/base/nsPresContext.h#971

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #0)

• For desktop zooming + RDM, we do want the document inside the RDM to get an async zoom container, but not the enclosing document.

I also believe bug 1562503 covers the RDM use case since RDM uses 'iframe mozbrowser' . So one remaining thing here is to enable the assertion in APZCTreeManager.cpp? A test case landed in bug 1562503 covers the OOPIF case, we might want a dedicated test case for the RDM case along with enabling the assertion?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I was wrong. There are still multiple async zoom containers in fission world. https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=255392462&revision=00591cc169ca7a728a5e1674d6938a10f6009b9c

Comment 6

[Chris Peterson [:cpeterson]]

Botond, do we need to fix this bug for the Fission MVP release?

Tentatively tracking for Fission Nightly (M6)

Comment 7

[Botond Ballo [:botond]]

(In reply to Chris Peterson [:cpeterson] from comment #6)

Botond, do we need to fix this bug for the Fission MVP release?

The fission part, yes. The RDM part only needs to block desktop zooming.

The fission part should be easy to fix; I'll post a fix here, and spin off the RDM part into another bug.

Comment 8

[Botond Ballo [:botond]]

Actually, even for Fission, it only needs to block it on mobile (or with desktop zooming).

Comment 9

[Botond Ballo [:botond]]

Oh, it looks like I've already split out the fission part into bug 1540298. It would make sense to track that for M6, rather than this bug.

Comment 10

[Botond Ballo [:botond]]

Moved M6 tracking flag to bug 1540298.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Botond Ballo [:botond] from comment #8)

Actually, even for Fission, it only needs to block it on mobile (or with desktop zooming).

(In reply to Botond Ballo [:botond] from comment #9)

Oh, it looks like I've already split out the fission part into bug 1540298. It would make sense to track that for M6, rather than this bug.

Thanks for updating the bugs.

btw, "Fission MVP" is desktop only. Android Fission will follow soon after desktop. (Android will require some more memory optimizations.)

Comment 12

[Chris Peterson [:cpeterson]]

(In reply to Botond Ballo [:botond] from comment #9)

Oh, it looks like I've already split out the fission part into bug 1540298. It would make sense to track that for M6, rather than this bug.

Setting [fission-] whiteboard tag so we don't track this bug for Fission MVP.

Comment 13

[Botond Ballo [:botond]]

A few relevant things that I discovered:

• Even if we don't get nested async zoom containers, we can get nested root content documents (RCDs), which is conceptually just as wrong, and is caught by an existing assertion (tracked in bug 1610657). The fix for the two is likely to be the same (use some logic to suppress both the RCD annotation and the async zoom container on the document that's currently getting the outer RCD annotation).
• Bug 1549775 (enabled locally by setting devtools.responsive.browserUI.enabled=true) partially fixes bug 1610657. It probably makes sense to continue investigating these issues with that change in place.

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)

(I haven't come across a scenario where we can get multiple async zoom containers which are not nested, but that's something we don't want either, and should include in our check.)

We've now ran into such a case (in bug 1650868) and Kats has convinced me that we should move in the direction of allowing multiple non-nested (sibling) async zoom containers, so I'm reducing the scope of this bug to cover nested async zoom containers only.

Comment 15

[Botond Ballo [:botond]]

It's not clear to me which scenarios (Fission, RDM, or both) remain affected. Let's do a try push with the assertion enabled and see:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8b362c6535bdc65206c3c574fe39277238199c8

Comment 16

[Botond Ballo [:botond]]

Not seeing any failures! I expect the Fission part was fixed by bug 1540298 and the RDM part by bug 1549775.

All that's left to do here is enable the assertion.

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1534459 - Enable assertion about nested async zoom containers. r=tnikkel

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/830d0f0a9fdc
Enable assertion about nested async zoom containers. r=tnikkel

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/830d0f0a9fdc