1002426 - Zoom level mangled after restoring session

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristina Madaras, QA [:CristinaM]]

Environment:
Build: Nightly (2014-04-28)
Devices: Galaxy Nexus (Android 4.2.1) and Samsung S3 (Android 4.3)

Steps to reproduce:
1. Start Fennec and navigate to a page (e.g. www.cnn.com);
3. Tools-> New Guest Session;
4. Exit Guest Session.

Expected results:
The page opened before guest mode is displayed normally.

Actual results:
The page opened before guest mode is zoomed in.

Notes:
I'm not able to reproduce this on tablets.

Comment 1

[Aaron Train [:aaronmt]]

This is reproducible on other sites such as kernel.org, I was able to reproduce this on my Nexus 5 (Android 4.4.2). Likely a regression from recent landing bug 611556, I think. Eugen?

Comment 2

[Eugen Sawin [:esawin]]

This is reproducing on tablets, too, it's just unnoticeable on the common screen sizes.

Bug 611556 enables zoom session history, which means we override the resolution when moving in history. In this case, we override the resolution with the default value (1.0), because the resolution has not been properly restored from disk.

It's related to bug 697858, scrolling position is not being properly restored either (tested on Fennec 28+). I can provide a workaround to disable zoom restoring from disk, but ultimately we need to fix the |SessionStorage| to provide complete state restoring from the persistent session storage, including scroll position and zooming.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: (WIP) Disable zoom override with default zoom

That's just a hotfix to disable zoom level overriding with the default zoom by setting the default value to an invalid range.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: Set uninitialized resolution to invalid range

I think the original default (1.0) is good because it's a valid default resolution. But given a valid default resolution, there is no way for us to determine whether the session restoring (from disk or online) was successful or not.

Having the default resolution set to an invalid range (0.0) is good because that signals us that we don't have a valid resolution restored from a previous session and it's up to the frontend to determine something sensible for the given device and document.

In the end, we want to add zoom restoring from session storage, but having a way to know that it was successful would still be useful, as contrary to scroll position, there is no general default resolution across all devices and documents.

Comment 5

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

Comment on attachment 8414854 [details] [diff] [review]
Set uninitialized resolution to invalid range

Review of attachment 8414854 [details] [diff] [review]:
-----------------------------------------------------------------

I think this will cause regressions on other platforms that also use the resolution code but don't have Fennec's front-end zoom-restoring code.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: (WIP) Check restore status before overriding zoom

That's right, a lot of desktop code assumes a valid range for the default value, we can't change that easily.

This patch uses the existing scroll frame restore status to trigger resolution overriding. We have to make |isHistoryRestored| visible to the frontend for that. I've put it into |nsDOMWidowUtil| where the resolution is accessed through, is there a better location for that?

This should fix all situations, where we unexpectedly restore to the default resolution.

Comment 7

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

Comment on attachment 8416806 [details] [diff] [review]
(WIP) Check restore status before overriding zoom

Review of attachment 8416806 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a reasonable approach to me.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: Check restore status before overriding zoom

Just did some clean up. This should be good, unless we don't want to expose |isHistoryRestored| via |nsDOMWindowUtils|.

Comment 9

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

Comment on attachment 8417499 [details] [diff] [review]
Check restore status before overriding zoom

Review of attachment 8417499 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/base/nsDOMWindowUtils.cpp
@@ +591,5 @@
> +
> +  nsIScrollableFrame* sf = presShell->GetRootScrollFrameAsScrollable();
> +  if (sf) {
> +    *aIsHistoryRestored = sf->DidHistoryRestore();
> +  }

I would just do
*aIsHistoryRestored = (sf && sf->DidHistoryRestore());

and get rid of the assignment to false up at the top. If there is an error return code the bool return value won't be accessible anyway.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +231,5 @@
>    void getResolution(out float aXResolution, out float aYResolution);
>  
>    /**
> +   * Whether the current window has been restored from session history.
> +   * This gives a way to check whether the provided resolution and scroll

This file needs a UUID bump

::: mobile/android/chrome/content/browser.js
@@ +4208,5 @@
> +    let res = {x: {}, y: {}};
> +    let cwu = this.browser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +    cwu.getResolution(res.x, res.y);
> +    let zoom =  res.x.value * window.devicePixelRatio;
> +    return zoom;

nit: no need for 'zoom' local var, just return the result of multiplication. If you want to leave the local var in, at least remove the double-space after '='

@@ +4224,2 @@
>        return null;
>      }

To make this easier to read, invert the condition:

if (this._restoreZoom && cwu.isHistoryRestored) {
   return this._getGeckoZoom();
}
return null;

Or use ?:

Comment 10

[Eugen Sawin [:esawin]]

Attachment: Check restore status before overriding zoom

Thanks for the super fast review!
I've addressed all the issues, but left the local zoom variable for readability (will be optimized out) and added some more const-correctness.

Carrying over the r+.

Comment 11

[Eugen Sawin [:esawin]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=1221d9d7d256

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a96c878cdf8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a96c878cdf8

Comment 15

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed on:

Build: Nightly (2014-05-08);

Device: Galaxy Nexus (Android 4.2.1).

This is still reproducible on Aurora (2014-05-05).

Comment 16

[Eugen Sawin [:esawin]]

The proposed fix depends on a correctly set |isHistoryRestored| flag, which may not be the set when we restore via the fastback cache (bfcache).

I would like to reopen the bug to propose a patch, which depends on a new flag specifically designed to signal whether the resolution has been set by the user or not, to avoid any side effects with the bfcache.

Comment 17

[Eugen Sawin [:esawin]]

Attachment: (Fix) Check resolution restore status via specialized flag

This fix just adds and uses the |isResolutionSet| flag instead of |isHistoryRestored| to avoid any issues with bfcache, the logic stays the same.

Comment 18

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

Comment on attachment 8421121 [details] [diff] [review]
(Fix) Check resolution restore status via specialized flag

Review of attachment 8421121 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok to me, but I'd like a layout peer to look too.

Comment 19

[Eugen Sawin [:esawin]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=50376b812eef

Patch for check-in: attachment #8421121 [details] [diff] [review]

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3ba939c4a277

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ba939c4a277

Comment 22

[Cristina Madaras, QA [:CristinaM]]

I'm still able to reproduce this on Aurora (2014-05-27). On Nightly (2014-05-27) is not reproducible using Alcatel One Touch (Android 4.1.2).

Comment 23

[Aaron Train [:aaronmt]]

(In reply to cristina.madaras from comment #22)
> I'm still able to reproduce this on Aurora (2014-05-27). On Nightly
> (2014-05-27) is not reproducible using Alcatel One Touch (Android 4.1.2).

This has not been been uplifted to Aurora.

Comment 24

[Kevin Brosnan [Ex-Mozilla]]

NI to request uplift

Comment 25

[Eugen Sawin [:esawin]]

Comment on attachment 8417669 [details] [diff] [review]
Check restore status before overriding zoom

Review of attachment 8417669 [details] [diff] [review]:
-----------------------------------------------------------------

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 611556
User impact if declined: Restoring the previous tab session or returning from a guest session would render the page at a non-standard resolution. On most smartphone devices, this would open the pages at zoom level which is neither the default zoom level for the page nor the zoom level last used by the user.
Testing completed (on m-c, etc.): It's been in m-c for a few weeks now, no issues.
Risk to taking this patch (and alternatives if risky): Low, well tested in m-c.
String or UUID changes made by this patch: nsIDOMWindowUtils.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6aa71900212a

Comment 27

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Build: 31.0a2 (2014-06-04);
Device: Galaxy Nexus (Android 4.2.1).