1742241 - page partial render or not render when switch to tabs util scroll in that tab

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[mix5003]

Attachment: 20211120_212729.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:96.0) Gecko/20100101 Firefox/96.0

Steps to reproduce:

1. open google.com for 2 tabs
2. open https://phabricator.services.mozilla.com/D128992 and wait for it loaded and scroll to bottom
3. switch to first google tab
4. right click at phabricator tab and click "Reload Tab". wait until it reloaded
5. addition wait for 5 second
6. switch to phabricator you can see that tab not render anything. but if you scroll down it will render properly

please try 2-3 times if it not happen. sometime it can works normally.

i attach video, so you can see reproduce step.

Actual results:

some tab not render (some tab still partial render) after reload in background.

for me i can see this many times when i open firefox and tab just restore from previous session. (it works normally you just need a little scroll in that tab)

Expected results:

tab should render normally when switch to that tab

Comment 1

[mix5003]

because reproduce step may not reliable (sometime tab can render normally), so this mozregression may not correctly but you can take a look it may able to help you

Comment 2

[mix5003]

2021-11-20T21:37:31.395000: DEBUG : Found commit message:
Bug 1687926. Don't request a repaint and set a display port if it's the first time for an azpc getting metrics always via the visualScrollOffsetUpdated path. r=botond

https://searchfox.org/mozilla-central/rev/2c06b16a0c15ae340a0532e319cbf89ef9d21b68/gfx/layers/apz/src/AsyncPanZoomController.cpp#5034

That RequestContentRepaint call gets executed every time isDefault is true (and ignoreVisualUpdate is false), ie whenever the azpc gets metrics for the first time. RequestContentRepaint causes a full display port to be set on the content. That is undesirable because we set zero margin display ports (via SetZeroMarginDisplayPortOnAsyncScrollableAncestors) and we don't want to expand them to full display ports

This bug is to fix the regression caused by bug 1627012. Two other bugs also regressed this (bug 1662013 and bug 1667475), and we need to fix all of them to fix the problem. Bug 1687886 is filed for the regression from bug 1667475. Bug 1687927 is filed for the regression from bug 1662013.

https://hg.mozilla.org/integration/autoland/rev/47328d6c1b40 made us request a repaint any time we have default metrics because we might get a new visual scroll offset, but if our visual scroll offset doesn't change we shouldn't need to do anything.

Differential Revision: https://phabricator.services.mozilla.com/D102586

2021-11-20T21:37:31.395000: DEBUG : Did not find a branch, checking all integration branches
2021-11-20T21:37:31.398000: INFO : The bisection is done.
2021-11-20T21:37:31.400000: INFO : Stopped

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Session Restore' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 4

[Timothy Nikkel (:tnikkel)]

Thanks for filing this. I think I've seen something like this on macos, but haven't been able to pin down steps to reproduce. Your regression range actually makes sense to me.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1687926

Comment 6

[mix5003]

Attachment: soundcloud_position_fixed_bottom.png

i think this bug may relate to https://bugzilla.mozilla.org/show_bug.cgi?id=1742274 .
because in some website. that use position: fixed to bottom: 0, it render at middle of page.

Comment 7

[Dzmitry Malyshau [:kvark]]

Glenn, this looks like an issue with WR picture invalidation. Just pinging in case you have some thoughts :)
One particularly interesting case is https://bugzilla.mozilla.org/attachment.cgi?id=9255716, where one of the tiles shows but others do not.

Comment 9

[Glenn Watson [:gw]]

Checked with tnikkel in matrix, and it sounds like this is APZ related rather than WR.

Comment 10

[Timothy Nikkel (:tnikkel)]

We get a couple of ScrollToImpl calls with origin == Restore, that scroll the page to about halfway to the final destination. They don't update the visual viewport offset here because of the origin

https://searchfox.org/mozilla-central/rev/e3a7a72713e1ba8696cb2af55e928059bc822572/layout/generic/nsGfxScrollFrame.cpp#3238

Then we get some ScrollToImpl calls with origin that does clobber apz so we update the visual scroll offset, but because he visual scroll offset is zero and we have a large scroll offset on the scrollframe already relativeOffset is large. We end up with a very large difference between the scroll position of the scroll frame and the visual viewport offset.

The regressing changeset removed a repaint request which would have reconciled this, without that we have a problem.

Comment 11

[Timothy Nikkel (:tnikkel)]

I think the problem is that we're calling PresShell()->GetVisualViewportOffset(), that returns (0,0) if the visual viewport offset hasn't been set, but here we want the layout scroll offset if we haven't set a visual scroll offset. This is what ScrollFrameHelper::GetVisualViewportOffset does. If this is correct, an audit of the other callers should be done to see if they suffer this same problem.

Comment 12

[Timothy Nikkel (:tnikkel)]

Okay, I think I have a workable fix for that specific issue, but I want to also look into teaching NotifyLayersUpdated to detect this situation and ask for a repaint, even if we think that it is a bug on the content side, so that if there are other similar bugs on the content side we handle it robustly and the user sees no breakage.

Comment 13

[Timothy Nikkel (:tnikkel)]

I think bug 1543485 introduced the bug here, it's just that bug 1687926 removed an (unnecessary) repaint request that "fixed" the problem.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1742241. Don't access the PresShell's visual viewport offset if we haven't set one. r?botond

If the visual viewport offset is not set on the presshell then calling GetVisualViewportOffset returns (0, 0). This then causes us to introduce an incorrect offset between the layout scroll offset and the visual viewport offset that didn't exist before.

Logically we want to treat an unset visual viewport offset as the layout scroll offset.

Other possible fixes:
-make PresShell::GetVisualViewportOffset return the layout scroll offset if a visual viewport offset is not set
-make this if conditional on a visual viewport offset already being set
-the combination of the two above
All of these fail some tests on try server. I haven't investigated why. If we want to go with any of those potential fixes in the future then this patch is a step on the way there

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1742241. Add test. r?botond,hiro

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba1e8079ac3a
Don't access the PresShell's visual viewport offset if we haven't set one. r=botond
https://hg.mozilla.org/integration/autoland/rev/d7e1407dbefe
Add test. r=botond

Comment 19

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/ba1e8079ac3a
https://hg.mozilla.org/mozilla-central/rev/d7e1407dbefe

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9262387 [details]
Bug 1742241. Don't access the PresShell's visual viewport offset if we haven't set one. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: switching back to a tab, or after session restore, half of tab will be blank until the user scrolls
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): simple fix to not use a value that has not been set
• String changes made/needed:

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9262387 [details]
Bug 1742241. Don't access the PresShell's visual viewport offset if we haven't set one. r?botond

Fix for an old regression with multiple duplicates, we are still in early betas, approved for 98 beta 5, thanks.

Comment 23

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc7325140a82
https://hg.mozilla.org/releases/mozilla-beta/rev/5e1b1589a0b4

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9262387 [details]
Bug 1742241. Don't access the PresShell's visual viewport offset if we haven't set one. r?botond

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: simple patch to fix a case that looks bad
• User impact if declined: switching back to a tab, or after session restore, half of tab will be blank until the user scrolls
• Fix Landed on Version: 99
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): simple fix to not use a value that has not been set

Comment 25

[Peter Magyari (Desktop QA)]

I have reproduced the issue on Firefox 97.0 and verified the fix on Firefox Beta 98.0b5 and Firefox Nightly 99.0a1.

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9262387 [details]
Bug 1742241. Don't access the PresShell's visual viewport offset if we haven't set one. r?botond

Approved for the ESR91 branch

Comment 27

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-esr91/rev/d92316d8e415
https://hg.mozilla.org/releases/mozilla-esr91/rev/0131547e550a

Comment 28

[Peter Magyari (Desktop QA)]

Verified the fix on Firefox 91.7.0esr (20220218112705)