1756762 - make all parts of gfx/layers/apz/test/mochitest/browser_test_background_tab_load_scroll.js pass

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1756762. Use the current origin to determine if we should update the main thread copy of the visual viewport update. r?botond

I observed the following sequence of events when running gfx/layers/apz/test/mochitest/browser_test_background_tab_load_scroll.js in one of the cases that it fails for me locally.

-ScrollFrameHelper::NotifyApzTransaction is called after sending a transaction to webrender, this sets mAllowScrollOriginDowngrade = true.
-the scroll frame is destroyed and re-created (I'm not sure why this destroy and re-create happened, I didn't investigate), we call SaveState and RestoreState. So now mLastScrollOrigin == Other, and mAllowScrollOriginDowngrade == true.
-the test calls ScrollBy, so we get a call to ScrollToImpl with aOrigin == Relative
-aOrigin gets changed to Other here https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/generic/nsGfxScrollFrame.cpp#2979
-the code just below that determines that it is not a downgrade, so we update mLastScrollOrigin to Other (not a change) and set mAllowScrollOriginDowngrade = false
-ScrollFrameHelper::NotifyApzTransaction is not called (probably because we are in a background tab), so mAllowScrollOriginDowngrade remains false
-the scroll frame is now at position 20000 and the visual viewport offset (stored on the presshell) is also 20000.
-the scroll frame is destroyed and re-created (this one I investigated, it's because the font subsystem has finished loading and asks for a global reconstruct https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/base/PresShell.cpp#9830), we call SaveState and RestoreState. So mLastScrollOrigin == Other, and mAllowScrollOriginDowngrade == true (the same values.
-we get a call to ScrollToRestoredPosition for the newly re-created scroll frame, we are trying to scroll from the current layout scroll position of 0 to the one we had before destruction and re-creation (20000). The visual viewport offset that is stored on the presshell has not been affected, it is still 20000.
-this gets to ScrollToImpl and with aOrigin == Restore
-we arrive at https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/generic/nsGfxScrollFrame.cpp#3000 and Restore is a downgrade from Other, and mAllowScrollOriginDowngrade is false so we do not change mLastScrollOrigin (still Other) and mAllowScrollOriginDowngrade remains false
-we arrive at the code in ScrollToImpl that updates the visual viewport offset on the presshell https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/generic/nsGfxScrollFrame.cpp#3232
-mLastScrollOrigin is Other, which can clobber apz, so we enter the if and add the difference between the before/after layout scroll position from this ScrollToImpl call (20000-0 = 20000) to the visual viewport offset (20000), getting 40000, which gets clamped to the max ~38000.
-and now the layout scroll position and the visual viewport offset have been detached when they shouldn't be.

Whenever we are doing a Restore scroll we expect that there has been a ScrollToImpl call in the past that got us to the position we are restoring to, and that past ScrollToImpl call should have resulted in the visual viewport offset on the presshell getting updated. So if the origin is Restore then the visual viewport offset should have already incorporated this scroll, and so we don't need to update the visual viewport offset for this scroll. If our assumption that the visual viewport offset on the presshell is already up-to-date is wrong that is okay because after an apz transaction they will be reconciled: the main thread is only trying to make an effort to sync with apz in advance and it is better to be conservative here, only updating the visual viewport offset when we know its correct, because as we've seen if we incorrectly update it they can get out of sync and cause bugs.

Another way to think of it: mLastScrollOrigin is the accumulation of all scroll origins since sending the last apz transaction and tells apz if the main thread should take precedence over the apz scroll position, and similarly the layout scroll position that we sned to apz is the accumulation of all scrolls since the last apz transaction. Whereas aOrigin tells us about one scroll, and we want to treat each scroll in that accumulation separately for determining how to update the main thread copy of the visual viewport offset.

Previously in https://phabricator.services.mozilla.com/D137873 I had stated that bug 1753881 (have PresShell::GetVisualViewportOffset() either return a maybe or layout scroll offset if vv offset is not set) would fix these cases in the test but I didn't understand why. Now I understand why. The reason is that although there is an obvious patch for bug 1753881 (return the layout scroll offset if the visual viewport offset isn't set) that patch is not correct. The reason is that in PresShell::SetVisualViewportOffset https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/base/PresShell.cpp#11199 we compare the new offset to the old offset which we get by calling GetVisualViewportOffset. So in the common case where we are in ScrollToImpl and we update the layout scroll offset and then call SetVisualViewportOffset to update the visual viewport offset then GetVisualViewportOffset is going to return the already updated layout scroll offset (if the visual viewport offset hasn't been set on the presshell yet). So SetVisualViewportOffset will determine that we are already up to date and not end up setting any visual viewport offset on the presshell at all. And so no visual viewport offset gets set at all. And since the original bug comes about because of a disagreement between the visual viewport offset and the layout scroll offset, we avoid the bug entirely by never setting a visual viewport offset. This is not acceptable because we thus never send visual viewport offset change events (mozvisualscroll) which we have several tests depending on (browser/components/sessionstore/test/browser_scrollPositions.js is one in particular that I want to note).

The reason I wanted to note browser/components/sessionstore/test/browser_scrollPositions.js was that this patch causes that test to fail. What happens in the test is a scroll frame gets a non-zero layout scroll position (and the same visual viewport offset on the presshell), then the page is reloaded. The layout scroll position is restored via a call to ScrollToImpl with origin Restore, thus it does not touch the visual viewport offset on the presshell. Before this patch we would have looked at mLastScrollOrigin insteand and touched the visual viewport offset. And because the page was reloaded we got a new presshell so it does not have a visual viewport offset. The test then gets the visual viewport offset and expects it to have the non-zero layout scroll offset, but it does not. So we can fix this by landing the (corrected) patch for bug 1753881 (have PresShell::GetVisualViewportOffset() either return a maybe or layout scroll offset if vv offset is not set).

Depends on D139472

Comment 2

[Timothy Nikkel (:tnikkel)]

I should note that some users apparently still see a similar bug to bug 1742241, ie https://bugzilla.mozilla.org/show_bug.cgi?id=1752942#c5 and this bug fixes it, so this is not just fixing a test.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1756762. When the root scroll frame is destroyed, reset the visual viewport offset on it's presshell. r?botond

The regular scroll position restoration code will restore it.

Specifically, in ScrollFrameHelper::SaveState

https://searchfox.org/mozilla-central/rev/44ae61c5eeb150cb77b6b8511ceee7ddd6892cb7/layout/generic/nsGfxScrollFrame.cpp#7376

we store the visual viewport offset. ScrollFrameHelper::ScrollToRestoredPosition will try to restore both the layout scroll offset and the visual viewport offset by calling ScrollToWithOrigin (which leads to ScrollToImpl) and PresShell::ScrollToVisual respectively.

I observed the following sequence of events when running gfx/layers/apz/test/mochitest/browser_test_background_tab_load_scroll.js in one of the cases that it fails for me locally.

-ScrollFrameHelper::NotifyApzTransaction is called after sending a transaction to webrender, this sets mAllowScrollOriginDowngrade = true.
-the scroll frame is destroyed and re-created (I'm not sure why this destroy and re-create happened, I didn't investigate), we call SaveState and RestoreState. So now mLastScrollOrigin == Other, and mAllowScrollOriginDowngrade == true.
-the test calls ScrollBy, so we get a call to ScrollToImpl with aOrigin == Relative
-aOrigin gets changed to Other here https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/generic/nsGfxScrollFrame.cpp#2979
-the code just below that determines that it is not a downgrade, so we update mLastScrollOrigin to Other (not a change) and set mAllowScrollOriginDowngrade = false
-ScrollFrameHelper::NotifyApzTransaction is not called (probably because we are in a background tab), so mAllowScrollOriginDowngrade remains false
-the scroll frame is now at position 20000 and the visual viewport offset (stored on the presshell) is also 20000.
-the scroll frame is destroyed and re-created (this one I investigated, it's because the font subsystem has finished loading and asks for a global reconstruct https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/base/PresShell.cpp#9830), we call SaveState and RestoreState. So mLastScrollOrigin == Other, and mAllowScrollOriginDowngrade == true (the same values.
-we get a call to ScrollToRestoredPosition for the newly re-created scroll frame, we are trying to scroll from the current layout scroll position of 0 to the one we had before destruction and re-creation (20000). The visual viewport offset that is stored on the presshell has not been affected, it is still 20000.
-this gets to ScrollToImpl and with aOrigin == Restore
-we arrive at https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/generic/nsGfxScrollFrame.cpp#3000 and Restore is a downgrade from Other, and mAllowScrollOriginDowngrade is false so we do not change mLastScrollOrigin (still Other) and mAllowScrollOriginDowngrade remains false
-we arrive at the code in ScrollToImpl that updates the visual viewport offset on the presshell https://searchfox.org/mozilla-central/rev/9b0bdcc37419e6765223358a31a4a54d62e1cd97/layout/generic/nsGfxScrollFrame.cpp#3232
-mLastScrollOrigin is Other, which can clobber apz, so we enter the if and add the difference between the before/after layout scroll position from this ScrollToImpl call (20000-0 = 20000) to the visual viewport offset (20000), getting 40000, which gets clamped to the max ~38000.
-and now the layout scroll position and the visual viewport offset have been detached when they shouldn't be.

If we reset the visual viewport offset when the scroll frame is destroyed we avoid the problem here.

Comment 4

[Phabricator Automation]

Comment on attachment 9265140 [details]
Bug 1756762. Use the current origin to determine if we should update the main thread copy of the visual viewport update. r?botond

Revision D139473 was moved to bug 1757314. Setting attachment 9265140 [details] to obsolete.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/923d22e44eba
When the root scroll frame is destroyed, reset the visual viewport offset on it's presshell. r=botond

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/923d22e44eba