1661480 - GeckoView no longer restoring scroll position

Status: VERIFIED FIXED

(GeckoView :: General, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Fenix Nightly 200827, GV 81.0a1-20200820093209

Using the latest Fenix Nightly the scroll position doesn't seem to get restored anymore. Other state like the back/forward history still gets restored, so I am assuming that we still pass the state correctly to GeckoView.

About 1.5 weeks ago I was reading a loooong text in a tab and every time I got back to the app, the scroll position was restored properly. So I guess that this may be a recent regression. Also it seems to work as intended in Fenix 79, currently on the release track.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

We discussed internally and we thought this was related to the iframe problem Bug 1661270, but I can reproduce with no iframes too, so there's more going on.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

My current theory is that this code is completely busted:

https://searchfox.org/mozilla-central/rev/ce21a13035623c1d349980057d09000e70669802/mobile/android/actors/GeckoViewContentChild.jsm#269-273

          progressFilter.addProgressListener(progressListener, flags);
          const webProgress = docShell
            .QueryInterface(Ci.nsIInterfaceRequestor)
            .getInterface(Ci.nsIWebProgress);
          webProgress.addProgressListener(progressFilter, flags);

Because it starts a progressListener on a about:blank child actor. The actor is recreated once the page navigates, making the progress listener useless. I think we could try to track this on the parent side and send the message once the page has navigated.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

Looks like we haven't had coverage for this for quite a while 🤦‍♂️ Bug 1547849

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1661480 - Restore scrolling position and form data.

When migrating RestoreState to actors we didn't consider that the child actor
gets recreated at every navigation, as its lifetime is tied to the inner
window.

This means that restoring state in one step is not possible, as restoring the
history will trigger a navigation from about:blank to the restored page.

To achieve this, we split restoring in two steps and we keep the state on the
parent actor instead of the child.

We move the restoring logic to a newly added GeckoViewContent parent actor,
which is more readibly accessible from both geckoview.js and
GeckoViewContent.jsm.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06ecf3c39c78
Restore scrolling position and form data. r=droeh

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/06ecf3c39c78

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

Comment on attachment 9172735 [details]
Bug 1661480 - Restore scrolling position and form data.

Beta/Release Uplift Approval Request

• User impact if declined: Scrolling position / form data won't be restored when restarting the browser
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The code is pretty involved but the test passes reliably now (used to be intermittent). Worst case scenario restoring tabs won't work in certain scenarios.
• String changes made/needed:

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172735 [details]
Bug 1661480 - Restore scrolling position and form data.

Approved for 81.0b4.

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f2ebd2e69bcd

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

ugh, looks like the fix is not enough for some websites (e.g. nytimes.com).

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1661480 - Handle subsessions displays too.

Comment 13

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1661480 - Restore scrolling position in new session.

When restoring state into a new session, the actor gets recreated after
navigating away from about:blank so we have to query for it again to get the
right instance.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ae043062d04
Handle subsessions displays too. r=droeh
https://hg.mozilla.org/integration/autoland/rev/d50edab964ef
Restore scrolling position in new session. r=droeh

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6ae043062d04
https://hg.mozilla.org/mozilla-central/rev/d50edab964ef

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please nominate the new patches for Beta uplift.

Comment 17

[[ex-Mozilla] Agi Sferro | :agi]

Comment on attachment 9174431 [details]
Bug 1661480 - Restore scrolling position in new session.

Beta/Release Uplift Approval Request

• User impact if declined: Scroll position and form data is not restored when restarting the app.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Open an article from nytimes.com, scroll all the way down, swipe away the browser, reopen it from home, reopen the tab, scroll position should be the same as when the app was swiped away.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This code is contained to restoring scroll position.
• String changes made/needed:

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9174430 [details]
Bug 1661480 - Handle subsessions displays too.

Approved for 81.0b9.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e3a959e6743
https://hg.mozilla.org/releases/mozilla-beta/rev/5b4d7972976c

Comment 20

[Andrei Bodea[:andrei]]

Verified as fixed on the latest Nightly from 9/17 with Google Pixel 4 XL (11)(GV 81).
Note that everything works as expected.