Closed Bug 1279443 Opened 4 years ago Closed 4 years ago

about:firefox is zoomed after restoring tab

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- verified
firefox50 --- verified

People

(Reporter: sebastian, Assigned: JanH)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
Related to any recent changes?
Flags: needinfo?(jh+bugzilla)
I need to take a look at what's actually happening, but bug 810981 is a very likely candidate.
Flags: needinfo?(jh+bugzilla)
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
Yeah, I can still reproduce it and I'll turn on logging.
Flags: needinfo?(s.kaspari)
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).
Flags: needinfo?(s.kaspari)
Attached file log-restore.txt
That's the log of one app startup where the page was zoomed in after restoring.
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.
Assignee: nobody → jh+bugzilla
Duplicate of this bug: 1282060
Blocks: 810981
See Also: → 1282902
Sebastian, could you try this build (http://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-3329e73878173f776bc678656766aa63c1691b9e/try-android-api-15/fennec-50.0a1.en-US.android-arm.apk) and see whether there's been any improvement?
Flags: needinfo?(s.kaspari)
Although the above approach seems to break the session store mochitests, so this needs some more thinking...
Flags: needinfo?(s.kaspari)
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/mozilla@buttercookie.de-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?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(bugmail.mozilla)
It seems to fix the problem, and doesn't cause any obviously incorrect behaviour for me.
Flags: needinfo?(bugmail.mozilla)
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/
Attachment #8766526 - Flags: review?(margaret.leibovic)
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
Attachment #8766526 - Flags: review?(margaret.leibovic) → review?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
Attachment #8766526 - Flags: review?(s.kaspari) → review+
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.
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.
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 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/
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/c4c4a4c44296
Don't capture session state during startup before we've restored history. r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4c4a4c44296
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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.
Flags: needinfo?(s.kaspari)
Keywords: regression
Verified as fixed in build 50.0a1 2016-07-14;
Device: Motorola Razr (Android 4.4.4).
(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?
Flags: needinfo?(s.kaspari)
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 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
Flags: needinfo?(jh+bugzilla)
Attachment #8766526 - Flags: approval-mozilla-aurora?
Comment on attachment 8766526 [details]
Bug 1279443 - Don't capture session state during startup before we've restored history.

Fix a regression, taking it.
Attachment #8766526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in build 49.0a2 (2016-07-29);
Device: Motorola Razr (Android 4.4.4) and LG G4 (Android 5.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.