1226872 - Fennec with C++ APZ sometimes get stuck in a state with scrollbars in the top-left and zooming not working properly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

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

While testing with C++ APZ I'm able to reproduce (somewhat intermittently) a scenario where upon zooming in, the page remains low-res. If I scroll at this point so that the dynamic toolbar goes on or off, the page immediately zooms back out. Also the scrollbars end up positioned on a fictitious viewport area in the top-left corner of the screen. We saw this on nytimes.com as well a few times last week.

What I'm seeing when it gets into this state is that the presShell resolution has been reset to 1.0 somehow (not yet sure how) and so when APZCCH gets a repaint request from the APZ, it doesn't update the resolution because there's a resolution mismatch [1]. The root cause here is the resolution getting set to 1.0 out of band, not yet sure why that's happening.

[1] https://dxr.mozilla.org/mozilla-central/rev/3f5afaf4e6b72c4b1a20749b4ce7d945add5299f/gfx/layers/apz/util/APZCCallbackHelper.cpp#219

Comment 1

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

It looks like the resolution is being read from the root scrollframe rather than the presShell [1], and the root scrollframe's resolution is 1.0. It actually gets set to the correct resolution initially but then we throw away that scrollframe and create about 40 new ones in quick succession which all have a 1.0 resolution. I think we might need to have the root scrollframe pick up the resolution from the presShell on creation. Still digging.

[1] https://dxr.mozilla.org/mozilla-central/rev/3f5afaf4e6b72c4b1a20749b4ce7d945add5299f/layout/base/nsLayoutUtils.cpp#8450

Comment 2

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

So yeah, picking up the resolution from the presShell in the ScrollFrameHelper constructor fixes the problem. However storing the resolution in two different places seems unnecessary and redundant. IIRC this was added for fennec to be able to restore the zoom when going back (so that the zoom is saved along with the scroll position in the history) but maybe we can find a better way to do this so it doesn't duplicate state which can get out of sync.

Comment 3

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

Attachment: Part 1 - Don't keep a copy of the resolution in the root scrollframe

This stuff was added back in bug 611556 to be able to save the resolution when navigating back to a page. However, having two copies of the resolution means that they can get out of sync, which is what was causing the bug here. We can either get rid of the resolution from the presshell (which is what was discussed in that bug) or get rid of it from the scrollframe (which is what I'm doing here). The reason I chose to do this was because our current architecture is more consistent with the resolution being a property of the presshell; additional resolution-related state is also currently on the presshell. Also I'm not sure there's much value is supporting zoom on arbitrary scrollable elements; it just feels cleaner to only allow it per-presshell.

Comment 4

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

Attachment: Part 2 - Remove nsLayoutUtils wrappers

Comment 5

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> It
> actually gets set to the correct resolution initially but then we throw away
> that scrollframe and create about 40 new ones in quick succession which all
> have a 1.0 resolution.

Just to clarify the "40 new ones" wasn't accurate - we only re-created the root scrollframe once, the rest of the scrollframes I was seeing were not root scrollframes. So the problem here was that:
- the presShell and root scrollframe were created (with 1.0 resolution)
- a 0.39 resolution was set on the presshell and root scrollframe
- the root scrollframe was destroyed a new one was created, with a 1.0 resolution
- no saved state was found to restore onto the new root scrollframe

This left things in a state where the presShell had resolution 0.39 and the root scrollframe had resolution 1.0 which screwed up a lot of stuff.

Comment 6

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

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bcae751db8e

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35ed8d9de74e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4347df8d3bcf

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/35ed8d9de74e
https://hg.mozilla.org/mozilla-central/rev/4347df8d3bcf