1279443 - about:firefox is zoomed after restoring tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: about-firefox-zoomed.png

STR:
* Go to about:firefox
* Close browser
* Remove browser from recent apps
* Restart browser

Expected:
* about:firefox tab is restored normally

Actual:
* about:firefox is zoomed in and can't be zoomed manually.

Comment 1

[:Margaret Leibovic]

Related to any recent changes?

Comment 2

[Jan Henning [:JanH]]

I need to take a look at what's actually happening, but bug 810981 is a very likely candidate.

Comment 3

[Jan Henning [:JanH]]

I'm afraid I can't reproduce this directly - if this is still happening, could you turn on "browser.sessionstore.debug_logging" and see what logcat is showing for GeckoSessionStore, especially the zoom related bits?

I'm seeing a possibly related problem though:
If I rotate my phone from portrait to landscape or vice versa between closing and restarting the browser, about:firefox is displayed at a wrong zoom level (zoomed in when going from portrait to landscape, zoomed out when going the other way). This is because while Firefox is running, that page behaves like a mobile page, i.e. its resolution doesn't change when the window width changes on device rotation and instead always remains at ~0.6667. [1]
When queried through getViewportInfo() however, that page is reported as autoSize = false [2], which is the signal for the session store to scale the stored zoom level if the window width has changed between saving and restoring of a tab.

[1] <meta name="viewport" content="width=480; initial-scale=.6667; user-scalable=no"/> (https://dxr.mozilla.org/mozilla-central/rev/016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d/mobile/android/chrome/content/about.xhtml#18)
[2] Where I think that info is ultimately coming from: https://dxr.mozilla.org/mozilla-central/rev/016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d/dom/base/nsDocument.cpp#8131

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, I can still reproduce it and I'll turn on logging.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

I can't reproduce this with the STR in comment 0 if I start with a fresh, empty session. It seems to be only reproducible after restoring the session a couple of times (just observation).

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: log-restore.txt

That's the log of one app startup where the page was zoomed in after restoring.

Comment 7

[Jan Henning [:JanH]]

Timestamps would have been nice, but nevertheless I have a new theory about  what has possibly gone wrong. Comment 3 is still valid, but probably belongs in a separate bug.

Just going off my memory here, when we're restoring a tab, we first load the restored address normally, then we're sending the history entries for that tab to Gecko and finally we're reloading the current history entry.
Loading a normal webpage is slow enough to not progress very far before reloading, but on a faster phone than mine ;-) and with a small, locally loaded page like about:firefox, loading might occasionally progress as far as pageshow before session history gets restored. Since in that case the restoreDataOn... flags won't have been set yet, the session store attempts to record the page's resolution, which for some reason or other is reported as 1 at this stage, even though it ought to be 0.6667.

In any case this means that a wrong resolution gets stored for that tab and once history restoring has finished and the page reloads, we restore the wrong zoom level if my theory is correct.

I'll look into it when I actually get back home.

Comment 9

[Jan Henning [:JanH]]

Sebastian, could you try this build (http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-3329e73878173f776bc678656766aa63c1691b9e/try-android-api-15/fennec-50.0a1.en-US.android-arm.apk) and see whether there's been any improvement?

Comment 10

[Jan Henning [:JanH]]

Although the above approach seems to break the session store mochitests, so this needs some more thinking...

Comment 11

[Jan Henning [:JanH]]

On my phone, I can sort of intermittently reproduce this using the huge session store file from bug 1279273 - restoring that session takes long enough that occasionally the pageshow event for the original load of the foreground tab fires, which resets the stored zoom level for that tab back to 1.

This build seems to be working now both locally and in testing:
http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-a7e7035fbdfd649cee94e6240c8d5592e2cc0697/try-android-api-15/fennec-50.0a1.en-US.android-arm.apk

Kats, Sebastian, if you could check and see if this does indeed fix the problem and doesn't cause any readily apparent unwanted side effects on your phones?

Comment 12

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

It seems to fix the problem, and doesn't cause any obviously incorrect behaviour for me.

Comment 13

[Jan Henning [:JanH]]

Attachment: Bug 1279443 - Don't capture session state during startup before we've restored history.

When restoring tabs on startup, the Java UI creates tab stubs for the tabs from the previous session. The selected foreground tab then starts loading as soon as Gecko is up and running. Meanwhile, the session store gets initialised, too and starts restoring history and other things for that tab.

After history has been restored for an active tab, the session store reloads the current history entry, however by that time, depending on device speed, page size and how many other tabs the session store has to process during startup, the initial page load might have progressed far enough to have already triggered various events monitored by the session store, e.g. "pageshow".

If those events arrive before tab restoring has finished, the session store will attempt to capture that tab's state, which will overwrite the values stored from the previous session. Once the page is then reloaded for restoring, wrong values (e.g. form data, scroll position, zoom level) might then be restored.

Therefore, we now abort any attempts to capture a tab's state
- for all tabs until the "sessionstore-windows-restored" notification has been received as a signal that the initial session restore during startup has finished
- for the restored foreground tab until the location change notification is received after reloading

Review commit: https://reviewboard.mozilla.org/r/61390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61390/

Comment 14

[Teodora Vermesan (:TeoVermesan)]

I can reproduce the issue with the following steps:
1. Go to about:firefox
2. "Check for Updates "and "Install Update"
4. Tap the "Open" button after update
=> about:firefox is zoomed in

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #14)
> I can reproduce the issue with the following steps:
> 1. Go to about:firefox
> 2. "Check for Updates "and "Install Update"
> 4. Tap the "Open" button after update
> => about:firefox is zoomed in

Yeah, that's how I saw this for the first time too.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8766526 [details]
Bug 1279443 - Don't capture session state during startup before we've restored history.

https://reviewboard.mozilla.org/r/61390/#review59722

Does it make sense to add some optional logging to those early returns?

I'm a bit worried that the SessionStore gets more and more complex (Well, it's a complex thing..). I wonder if we should start to document the overall design/flow somewhere; either in the code or the wiki.

Comment 17

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/61390/#review59722

I wasn't quite sure about this and in the end, I didn't want to overload the session store with logging, even if it's turned off by default. However if you're feeling this way, I'm happy to resurrect some of my debug logging. [1] However I don't think we have to log the early returns - we log both the event and the successful completion of onTabLoad/Input/Scroll, so we can infer any early returns through that. Instead, I'd rather capture the moment where we turn that data capture blocking off.

[1] And since I couldn't reproduce this initially, the existing logging was indeed immensely helpful in finding out what went actually wrong.

Comment 18

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/61390/#review59722

Re documentation, we've got [this wiki page](https://wiki.mozilla.org/Mobile/SessionRestore) (which coincidentally hasn't been updated since 2014) which I might potentially resurrect. As for documentation in the code (or at least in the tree), do you have any good examples for that?
While I recognise that documentation outside of the code is always in danger of becoming outdated more easily than in-code docs (although I've encountered out-of-date code comments, too), some sort of overview in one place might still be useful since parts of the session store code are now spread out across multiple files.
I'll try and keep it in mind for when I've managed to burn through my current bug list.

Comment 19

[Jan Henning [:JanH]]

Comment on attachment 8766526 [details]
Bug 1279443 - Don't capture session state during startup before we've restored history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61390/diff/1-2/

Comment 20

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92f4ede3ee33

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c4c4a4c44296
Don't capture session state during startup before we've restored history. r=sebastian

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4c4a4c44296

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you think this is good for uplift to 49, considering the possible risk of uplift? It would be nice not to ship 49 with this regression.

Comment 24

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 2016-07-14;
Device: Motorola Razr (Android 4.4.4).

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #23)
> Do you think this is good for uplift to 49, considering the possible risk of
> uplift? It would be nice not to ship 49 with this regression.

Yeah, this is a good candidate. But we have made a lot of changes in the session store area lately, so I'm not sure how easy it is to uplift this.

@JanH: What do you think?

Comment 26

[Bill Gianopoulos [:WG9s]]

This needs ot be uplifted to fix this in aurora.  That is where i originally reported the issue the problem is less prevalent in release builds it occurs while you launch Aurora and go to the about page and use that to update then on the next launch it is all hosed.  on release builds the update is via the google store so the issue does not occur.

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8766526 [details]
Bug 1279443 - Don't capture session state during startup before we've restored history.

(In reply to Sebastian Kaspari (:sebastian) from comment #25)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #23)
> > Do you think this is good for uplift to 49, considering the possible risk of
> > uplift? It would be nice not to ship 49 with this regression.
> 
> Yeah, this is a good candidate. But we have made a lot of changes in the
> session store area lately, so I'm not sure how easy it is to uplift this.
> 
> @JanH: What do you think?

The bulk of changes has already landed and unless I'm mistaken, this doesn't depend on anything else that landed in 50.

Approval Request Comment
[Feature/regressing bug #]: Mobile session store, bug 810981
[User impact if declined]: When restoring a session or a single tab, about:firefox might be displayed zoomed in even though it shouldn't be. On other sites, we might loose the user-stored zoom level from the previous session and restore the page at a default zoom level.
[Describe test coverage new/current, TreeHerder]: mostly manual and a week on Nightly
[Risks and why]: Lowish - the changes are small and as far as I've been able to ascertain, they do exactly what they are supposed to do. I cannot completely exclude the potential for introducing some other subtle bug, though.
[String/UUID change made/needed]: none

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8766526 [details]
Bug 1279443 - Don't capture session state during startup before we've restored history.

Fix a regression, taking it.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/74541109158f

Comment 30

[Sorina Florean [:sflorean]]

Verified as fixed in build 49.0a2 (2016-07-29);
Device: Motorola Razr (Android 4.4.4) and LG G4 (Android 5.1).