1644271 - Add a pref to always create the MVM, and turn it on

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task)

Description

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

One of the things causing test failures in bug 1621740 is that the MVM now gets created, and even if the page remains at 1.0 resolution (as it does during most tests), there is some non-negligible impact.

Per discussion in #apz:mozilla.org, I'm going to create another pref that allows enabling the MVM without the allow_zooming pref, so that we can isolate exactly which failures are resulting from the MVM alone vs the rest of the zooming-conditional code. We can turn this pref on first (after ensuring it doesn't cause failures), and that should reduce the number of failures we have to deal with for bug 1621740.

It will also give us another knob for users to fiddle with when diagnosing failures in the wild.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=69293199954ab5656fb37b92892cf791d0a7b54f will presumably have some failures that I'll dig into.

Comment 2

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

I've been trying to chase down the failure in layout/generic/test/test_scroll_position_restore.html that happens with the MVM enabled. What I've discovered so far, mostly by tracing backwards with rr:

• ScrollToRestoredPosition gets called correctly at the right times. However, after the first ScrollToRestoredPosition call, there is another ScrollToImpl call that clears the mRestorePos.y value.
• The ScrollToImpl call comes from one of the scrollbar attributes changing. In particular the maxpos for the x-scrollbar changes to -10 CSS pixels which forces the "current" attribute to get reclamped between min (0) and max (-10).
• Obviously the -10 maxpos attribute is bogus, so I chased that down, and that's coming from a visual viewport size that doesn't properly exclude the vertical scrollbar width.
• The scrollbar size is coming out as zero because the presShell's root scrollframe is nullptr.
• That in turn is happening because the visual viewport is getting recalculated as part of the nsGfxScrollFrame state restoration which happens immediately after the gfxscrollframe is constructed, here.

So if my high-level understanding is correct, we're basically setting the visual viewport so early that the scrollframe hasn't been installed and we can't compute the scrollbar width. So that's one thing to fix. Another would be to ensure the visual viewport gets updated as scrollbars appear and disappear, because right now I don't see anything in the code that would do that. The fix for the latter issue by itself may not fix this test because even if we fix the visual viewport a little later, it might still be after the bogus maxpos change causes the mRestorePos to get wiped out, and that's a non-reversible change that will abort scroll position restoration.

Comment 3

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

My solution to comment 2 basically involved making the MVM refresh the visual viewport size after the root scrollframe finishes a reflow. I think this makes sense because the root scrollframe reflow can make scrollbars appear or disappear, and so we need to update the visual viewport size for those changes. The updated VVS (which correctly deducts the area used by scrollbars) can then get used when computing the maxpos and other scrollbar attributes, which produces the right thing. That patch by itself fixed this test some of the time, but seemed racy. I was using rr to debug a capture of a bad case, and it seemed like a second unrelated problem.

The second problem I believe was due to mLastPos not properly getting updated during the scroll restoration process, because GetLogicalVisualViewportOffset was returning (0,0). I'm not sure if that was the root cause but while poking around I discovered bug 1644567 and fixing that made the test pass consistently, so I moved on to other tests. However the try push for bug 1644567 seems to indicate unexpected failures and will need some investigation. There might be multiple bugs canceling each other out here.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)

• Obviously the -10 maxpos attribute is bogus, so I chased that down, and that's coming from a visual viewport size

That seems like a bug, we should be using GetVisualScrollRange which should make sure that maxpos is 0 here. I filed bug 1644638 for this.

Comment 5

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

I have a set of patches for this bug that I expect to be green on try, although they only do part of the job. They add the pref and turn it on, but also disable one codepath (setting the visual viewport size) in the MVM that will need to remain enabled. However, enabling that codepath causes some test failures that need investigation, so I thought it might be worth landing the first set of patches and then continuing work on greening up those tests. i.e. just like how we split allow_zooming=true into "enable MVM" and "do everything else", this set of patches splits "enable MVM" into "enable most MVM code" and "start setting the visual viewport size" so that we can divide and conquer the overall problem.

Comment 6

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

Last couple of try pushes are here and here - they have the same set of patches, but the second push has more jobs via ./mach try auto. It's pretty green except some assertions on android crashtests, which I'm kind of surprised at, because my patchset should have no effect on Android. The assertions might be unrelated but I'll dig a bit more.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=8a63c05135aebf40c7625e41f75dabc32908d4e6 is looking better, I dropped the one patch that did have a functional effect on Android. I don't need that one right now, can try to land it again later if need be.

Comment 8

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

Attachment: Bug 1644271 - Add a pref to enable the MobileViewportManager. r?tnikkel

Currently false by default, so no functional change in the default
configuration.

Comment 9

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

Attachment: Bug 1644271 - Add a ManagerType field to the MVM, to allow conditional behaviour. r?tnikkel

The MVM is needed for both handling of meta-viewport tags and APZ zooming.
However, the set of functionality needed in the two modes are not the same.
This patch adds a mechanism to create an MVM with a flag that lets it know
which mode it is operating in. Eventually we may want to split this into two
or more classes but for now this seems like a reasonable way forward.

The flag is currently set on the MVM on creation based on whether or not the
meta-viewport support is needed. There's no code that meaningfully uses the
flag yet, so this patch should have no functional change. The bulk of the
patch is ensuring that we appropriately destroy and re-create the MVM if the
flag required changes.

Depends on D79223

Comment 10

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

Attachment: Bug 1644271 - Don't let the MVM control reflow dimensions unless we're using mobile viewport sizing. r?tnikkel

Allowing the MVM to control the reflow means that the requested reflow size
is ignored, and instead the existing CSS/layout viewport is used. This is
undesirable for calls to SizeToContent(), where the intent is to do a reflow
to figure out the smallest amount of space the content fits in.

In general though unless we are using mobile viewport sizing we shouldn't be
needing the MVM to drive reflows.

Depends on D79224

Comment 11

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

Attachment: Bug 1644271 - Don't let the MVM do reflows or change the resolution unless we're using mobile viewport sizing. r?tnikkel

If there's no meta-viewport handling, the MVM shouldn't need to do reflows
because it shouldn't be changing the layout viewport. Also there should be
no need for the MVM to adjust the resolution on the presShell since the
user will be driving those changes via user input. The MVM now just updates
the visual viewport sizing in response to changes.

It may turn out that some these conditions need to be tweaked later, but for
now this seems like a reasonable starting point.

Depends on D79225

Comment 12

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

Attachment: Bug 1644271 - Add some more MVM_LOG statements. r?tnikkel

Depends on D79226

Comment 13

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

Attachment: Bug 1644271 - Move the innerWidth/Height overriding behavior to be conditional on a more appropriate check. r?tnikkel

This code really cares about the case with a CSS viewport, which is only
activated with mobile viewport sizing.

Depends on D79227

Comment 14

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

Attachment: Bug 1644271 - Add an early-exit to temporarily disable an MVM codepath. r?tnikkel

This is a short-term step to ensure all tests pass with the mvm pref
turned on. It disables the visual viewport setting codepath for visual-only
MVM instances, unless the APZ zooming pref is also set (because other APZ
zooming code relies on this).

Depends on D79228

Comment 15

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

Attachment: Bug 1644271 - Enable the MVM pref by default. r?tnikkel

Depends on D79229

Comment 16

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)

The second problem I believe was due to mLastPos not properly getting updated during the scroll restoration process, because GetLogicalVisualViewportOffset was returning (0,0).

So this is true, mLastPos doesn't get updated properly. It's supposed to be tracking the current visual viewport offset as we incrementally restore the scroll position, but given how the presShell's visual offset doesn't update until a round-trip to APZ, it doesn't track properly. I tried also making it read the pending visual offset as :tnikkel suggested somewhere (on Matrix probably) but that doesn't work as expected either because the ScrollToVisual call uses the un-clamped scroll destination (i.e. mRestorePos) and so it's not what we actually want to know. If I change the ScrollToVisual to request the clamped restore position then things seem to work better.

I think even with this there's a bit of an edge case that doesn't get exercised by this particular test, because the pending visual offset gets cleared at the end of the paint and there might be a gap between that APZ sending back the new visual scroll offset to the presShell here. If the VV offset is queried in that gap it will return what is effectively a stale value. So this just goes back to our discussion on Matrix about how properly keep the VV offset synchronized between APZ and the main thread.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/281c46b1cee4
Add a pref to enable the MobileViewportManager. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/50e225dbb624
Add a ManagerType field to the MVM, to allow conditional behaviour. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/dfe9f24ecca6
Don't let the MVM control reflow dimensions unless we're using mobile viewport sizing. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/178810d1d464
Don't let the MVM do reflows or change the resolution unless we're using mobile viewport sizing. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/a882991461f2
Add some more MVM_LOG statements. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/4589c96107a0
Move the innerWidth/Height overriding behavior to be conditional on a more appropriate check. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e41719413900
Add an early-exit to temporarily disable an MVM codepath. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/01a1753056cf
Enable the MVM pref by default. r=tnikkel

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/281c46b1cee4
https://hg.mozilla.org/mozilla-central/rev/50e225dbb624
https://hg.mozilla.org/mozilla-central/rev/dfe9f24ecca6
https://hg.mozilla.org/mozilla-central/rev/178810d1d464
https://hg.mozilla.org/mozilla-central/rev/a882991461f2
https://hg.mozilla.org/mozilla-central/rev/4589c96107a0
https://hg.mozilla.org/mozilla-central/rev/e41719413900
https://hg.mozilla.org/mozilla-central/rev/01a1753056cf

Comment 19

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

I've been continuing to investigate the test failures. One of the tricky things about the visual viewport is making sure it gets updated at the exact right time relative to a reflow. Fixed-pos item positioning uses the visual viewport here and occurs during reflow. But also the VV size depends on the presence or absence of root scrollbars, and that only gets determined during the root scrollframe's reflow which (I believe) occurs later. So in some cases we might end up needing to do two reflows. Also slightly worrisome is that the act of changing the VV size marks some things as dirty here which is not allowed during a reflow. But I think we might want to set the VV during reflow anyway, and just skip marking things dirty if we're in the reflow. This is fine as long as the VV set during reflow happens early during the reflow but if we later add more places where we're setting the VV later during reflow that might result in incorrect state remaining painted. I've been tweaking this as I work my way through the tests and wrapping my head around the difference cases.

Latest try push is at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=8407aab6e09e3621be3ac15f510bb5a8ef0361c5

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)

I've been continuing to investigate the test failures. One of the tricky things about the visual viewport is making sure it gets updated at the exact right time relative to a reflow. Fixed-pos item positioning uses the visual viewport here and occurs during reflow. But also the VV size depends on the presence or absence of root scrollbars, and that only gets determined during the root scrollframe's reflow which (I believe) occurs later.

Looking at ViewportFrame::Reflow, ViewportFrame::AdjustViewportSizeForFixedPosition looks to be called after reflowing the regular children of the viewport frame, it after reflowing the root scroll frame.

Comment 21

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

Ah, ok. That makes my life easier, but also puzzles me a bit. I'll have to redo my investigation where I came up with this conclusion and figure out what I was doing wrong...

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=bdd0c85ca1429d12f600dacf22e42f2af01eafec is the latest try push. Getting pretty close. I know how to deal with the R5 and C failures. Looking into the dt6 failure. The R8 might be fuzzable, but it looks like maybe rounding error on the WR codepath somewhere. Not sure about a11y failure, not convinced yet it's from my patches.

Comment 23

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

The dt6 failure (devtools/client/inspector/test/browser_inspector_highlighter-rulers_03.js) is interesting. The test starts off with the devtools open and docked to the bottom, and checks some things. Then it moves the devtools to the right side of the window and checks more things. The problem is that when the devtools get moved over to the right, we end up with a spurious vertical scrollbar on the content.

The reason that happens is as follows. When the root scrollframe goes through reflow, it tries the various combinations of scrollbars to see which one is "consistent". For each attempt, it reflows the contents assuming the indicated scrollbar configuration, and then checks to see if the scrollbars wanted at that layout match the initial assumption.

However, in order to see if the scrollbars are wanted, it computes a visual viewport using this fishy code where in particular the call to CalculateCompositionSizeForFrame turns around and asks the scrollframe for its scroll port rect. But that scrollport rect is stale, because it doesn't get updated until after there's a consistent layout arrived at, here.

So in this test case, the stale scrollport height that gets used is the one from when the devtools was docked on the bottom, and so it ends up "wanting" a vertical scrollbar.

That being said, this addition of the vertical scrollbar does shrink the visual viewport width, which could/should kick off another reflow cycle because it marks the scrollbars dirty here. And that second reflow should then get rid of the vertical scrollbar by detecting a consistent layout with no scrollbars. However, that second reflow doesn't happen because of local changes I added to fix other issues.

I think overall it's better to try and make this arrive at a consistent state in a single reflow, so I'd prefer to make the visual viewport size calculation use a not-stale scrollport rect, rather than find another solution for the other issues.

Comment 24

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

The fishy code is also wrong because it is dividing by resolution before subtracting off the desired scrollbar size. That's the opposite (and incorrect) order as compared to the actual visual viewport calculation.

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)

However, in order to see if the scrollbars are wanted, it computes a visual viewport using this fishy code where in particular the call to CalculateCompositionSizeForFrame turns around and asks the scrollframe for its scroll port rect. But that scrollport rect is stale, because it doesn't get updated until after there's a consistent layout arrived at, here.

I came across this problem recently while looking at something else but it wasn't causing my any problems and I meant to file it eventually. Sorry, I guess that is a lesson to me that I should have filed.

I think overall it's better to try and make this arrive at a consistent state in a single reflow, so I'd prefer to make the visual viewport size calculation use a not-stale scrollport rect, rather than find another solution for the other issues.

Yeah, that seems like the best option.

Comment 26

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)

However, in order to see if the scrollbars are wanted, it computes a visual viewport using this fishy code where in particular the call to CalculateCompositionSizeForFrame turns around and asks the scrollframe for its scroll port rect. But that scrollport rect is stale, because it doesn't get updated until after there's a consistent layout arrived at, here.

Filed bug 1645954 for this since I didn't see a patch for this on your latest try push linked here. Patching uploaded in a minute.

Comment 27

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

(In reply to Timothy Nikkel (:tnikkel) from comment #26)

Filed bug 1645954 for this since I didn't see a patch for this on your latest try push linked here. Patching uploaded in a minute.

Thanks. However I'm not sure it's a great use of your time, unless you have a good way to test that your patch actually fixes the problem. By itself that patch (and the ones I was working on) don't fix the problem I was seeing.

Comment 28

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)

Thanks. However I'm not sure it's a great use of your time, unless you have a good way to test that your patch actually fixes the problem. By itself that patch (and the ones I was working on) don't fix the problem I was seeing.

This is because the widget size is also stale, and that gets picked up from here and overrides the scrollport for the scrollframe. The widget size is stale because it gets updated here in RecvUpdateDimensions, which is just after the SetPositionAndSize call that updates the content viewer and triggers the reflow.

The comment there seems to imply that the widget size updating should be the thing triggering the reflow, but that's not what's happening here, it's the reverse.

Comment 29

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

With all the dependent bugs fixed (which I have patches for, just waiting for the individual try pushes) plus some other patches I have for this bug, the only remaining failure are the two R8 reftests which so far seem to only affect linux64-qr and look kind of fuzzable. At least I might fuzz them to get this stuff landed, and file a follow-up to dig in there and maybe un-fuzz them.

Comment 30

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

Attachment: Bug 1644271 - Log the URI for each MVM created. r?tnikkel

There's a lot of documents that get created (about:blank and friends) and this
makes it easier to figure out which MVM we actually care about.

Comment 31

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

Attachment: Bug 1644271 - Add some MOZ_LOG for scrollbar layout attempts. r?tnikkel

I found this useful while debugging. It might be overkill to land it.

Depends on D80085

Comment 32

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

Attachment: Bug 1644271 - Block some codepaths that try to use a zero display size. r?tnikkel

This fixes a number of scroll-anchoring WPT tests that otherwise fail with the
final patch in this patchset. This patch also fixes a pre-existing failure on
Android, so we remove the corresponding failure annotation here. The guard
being added already exists on some of the codepaths leading to this function,
and is used to short-circuit other bits of code. However, there are still some
codepaths that end up in this function with a zero display size, and it's better
to leave the visual viewport unset in the presShell than to set it to a zero
size.

Depends on D80086

Comment 33

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

Attachment: Bug 1644271 - Possible fix for bad assumption in browser_test_resolution.js. r?tnikkel

There's a bug on file for understanding this test better, but for now this
seems like a not-unreasonable fix.

Depends on D80087

Comment 34

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

Attachment: Bug 1644271 - Adjust assertion counts on some crashtests. r?tnikkel

I looked at the assertion stacks and checked that there were no new kinds of
assertions being triggered, just more of the same old assertions. Seems
reasonable that as the desktop behaviour moves closer to mobile (by turning
on visual viewport codepaths) the assertion count will also match that of
mobile.

Depends on D80088

Comment 35

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

Attachment: Bug 1644271 - Mark a couple of reftests fuzzy with WR. r?tnikkel

The top end of the scrollbar seems off by a subpixel amount. Some initial
investigation is documented in bug 1646527 and points to a WR internal bug.

Depends on D80089

Comment 36

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

Attachment: Bug 1644271 - Enable setting the visual viewport size for VisualViewportOnly-type MVM instances. r?tnikkel

Depends on D80090

Comment 37

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

With the latest set of patches and dependent bugs that should close out this bug.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a780f5111efa
Log the URI for each MVM created. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/03aaac05c759
Add some MOZ_LOG for scrollbar layout attempts. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/efaddcd67a33
Block some codepaths that try to use a zero display size. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/3cf3176c45d4
Adjust assertion counts on some crashtests. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/681a48643a84
Mark a couple of reftests fuzzy with WR. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/9d4e746c732d
Enable setting the visual viewport size for VisualViewportOnly-type MVM instances. r=tnikkel

Comment 39

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/a780f5111efa
https://hg.mozilla.org/mozilla-central/rev/03aaac05c759
https://hg.mozilla.org/mozilla-central/rev/efaddcd67a33
https://hg.mozilla.org/mozilla-central/rev/3cf3176c45d4
https://hg.mozilla.org/mozilla-central/rev/681a48643a84
https://hg.mozilla.org/mozilla-central/rev/9d4e746c732d

Comment 40

[Razvan Maries]

Backed out for perma failures on browser_bug1170531.js.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C7%2Cshippable%2Copt%2Cmochitests%2Ctest-windows7-32-shippable%2Fopt-mochitest-browser-chrome-e10s-1%2Cm%28bc1%29&revision=9d4e746c732dccd53212745d2a24e61939a44df2&selectedTaskRun=ABplaBGnTIGjUZSojmRwbg.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306978072&repo=autoland&lineNumber=8339

Backout: https://hg.mozilla.org/integration/autoland/rev/acd6b0f6454a4f00812e707501b3956f11ae78ad

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/acd6b0f6454a

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7e12cd61210
Log the URI for each MVM created. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/95858c1c668c
Add some MOZ_LOG for scrollbar layout attempts. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/3ef752fcbb18
Block some codepaths that try to use a zero display size. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/411bde6ec24f
Adjust assertion counts on some crashtests. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/2dfbbefc3eee
Mark a couple of reftests fuzzy with WR. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/cc56a4cc0415
Enable setting the visual viewport size for VisualViewportOnly-type MVM instances. r=tnikkel

Comment 43

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/c7e12cd61210
https://hg.mozilla.org/mozilla-central/rev/95858c1c668c
https://hg.mozilla.org/mozilla-central/rev/3ef752fcbb18
https://hg.mozilla.org/mozilla-central/rev/411bde6ec24f
https://hg.mozilla.org/mozilla-central/rev/2dfbbefc3eee
https://hg.mozilla.org/mozilla-central/rev/cc56a4cc0415

Comment 44

[Ryan VanderMeulen [:RyanVM]]

This was disabled for Fx79 in bug 1648687. It remains enabled for 80+.