1489852 - The browser size will increase with each reboot when you exit the browser in maximized mode.

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

Build ID 	20180908220614
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

This is an annoyance regression bug.
Browser size increases if quit browser in maximized mode.

Reproducible: always, with new profile.

Steps To Reproduce:
1. Switch browser to normal sizemode, if not
   --- Remember browser size
2. Switch the browser to maximize mode (click maximize button)
3. Quit browser(Alt File > Exit or click x button)

4. Start browser again (The browser is now in maximized mode as expected)
5. Switch browser to normal size mode(click restore button)
   --- Compare with the browser size at step 1

Actual Results:
The size is getting bigger

Expected Results:
Should be same size.


Threre 2 regression at least.


#1 Regression window(fails to restore browser sizemode, but correct size in normal sizemode):

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cad403cafed9b17d1b801837abcd039a0bce82d5&tochange=990a8eb972cd2a734da04ed8d787564752e8c9c7

Regressed by: Bug 1439875


#2 Regression window(browser size of normal sizemode is getting bigger):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e21a2a57d05dfe3c5ff8ba71131127fa781ffdd0&tochange=f026ead5dbfce9d6530429ac568ecc5544cc9b3b

Regressed by:
f026ead5dbfc	Florian Quèze — Bug 1447719 - Set browser.startup.blankWindow to true on Windows and Linux, r=mconley.


:emilio , :florian,
The bunch of patch causes the regression, can you please look into this?

Comment 1

[Florian Quèze [:florian]]

The size seems to increase by the height of the titlebar and the width of window borders. For windows in normal sizemodes, the size is set using window.resizeTo, specifically to avoid this bug. When the window is first opened in maximized mode, we set the size using the 'height' and 'width' attributes: https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/browser/components/nsBrowserGlue.js#349-355

Emilio, any idea of how we could avoid this problem when setting the size of a maximized window?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So, this is related to bug 1444525. The sizes persisted are outer window sizes. Height and width are the inner window size. So as long as you use the width and height attributes you should subtract the window size difference.

I don't think there's an easy way to add it from javascript, but should be easy to add an API that sets them on XULWindow size accounting for the outer size, or an API that exposes the inner-to-outer-size difference so that the frontend can do whatever it pleases with it.

Actually, looks like that API already exists:

  https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/browser/components/sessionstore/SessionStore.jsm#4467

So that code should probably just use it?

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

Thanks Emilio! Yes, using that API solves the problem.

Comment 4

[Florian Quèze [:florian]]

The patch is simple enough that I think we should uplift at least to beta.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9008372 [details] [diff] [review]
Patch

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

Wow, I didn't know those getters existed. Very handy! Yeah, this looks good. Thanks florian and emilio!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/690dfc98faff
convert the size from outer to inner window dimensions when setting the width and height to use when the user leaves the maximized mode, r=mconley.

Comment 7

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/690dfc98faff

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 9008372 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: probably bug 1444525, but the regression only became user visible when enabling the early blank window in bug 1473142.
[User impact if declined]: browser window size incorrectly restored when the window was maximized at the time the browser was closed.
[Is this code covered by automated tests?]: no, it's unfortunately very hard to cover things that involve a restart with automated tests.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, please. Steps in comment 0.
[List of other uplifts needed for the feature/fix]: I'll request uplift in bug 1489214 too and it would make sense to QA both at the same time, but technically they could land separately.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Self contained fix that only affects the behavior in the edge case that was broken.
[String changes made/needed]: none.

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9008372 [details] [diff] [review]
Patch

Low risk patch fixing a regression, uplift approved for 63 beta 6, thanks.

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0daf051f7fc7

Comment 11

[Daniel Bodea [:danibodea]]

I can confirm the reproduction of the issue in Nightly v64.0a1 (2018-09-05) and I can confirm the fix in Nightly v64.0a1 (2018-09-13). 
Awaiting beta build for uplift verification. Thank you.

Comment 12

[Daniel Bodea [:danibodea]]

This issue is now fixed in Beta v63.0b6 on Windows 10. 
Uplift successful. Thank you!

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9008372 [details] [diff] [review]
Patch

approved for 62.0.2 based on bug 1489214 comment 21

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-release/rev/003695979048

Comment 15

[Daniel Bodea [:danibodea]]

Putting the qe-verify+ flag back for verification process.

Comment 16

[Julien Cristau [:jcristau] (back April 22)]

Backed out from mozilla-release for bustage:
https://hg.mozilla.org/releases/mozilla-release/rev/da3cd3cea8c0

Florian, should we also backout bug 1489214 too or are we ok with just that one?

Comment 17

[Mike de Boer [:mikedeboer]]

Julien, what made you feel certain that this patch is the one causing the bustage? I couldn't spot it from the logs after a bit of examination, so I'm hoping you can tell us more? :-)

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

RyanVM ran a set try pushes with various backouts and this one ended up green.

Comment 19

[Florian Quèze [:florian]]

The new API that this patch uses landed in 61 (in bug 1446343) so it's not the problem. My current guess is that the bustage comes from a dependency on the changes from bug 1446940 that landed in 63.

If my guess is correct, .docShell.treeOwner (on 63+) needs to be .QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsIDocShellTreeItem).treeOwner on 62.

(In reply to Julien Cristau [:jcristau] from comment #16)
> Backed out from mozilla-release for bustage:

Sorry :-(.

> Florian, should we also backout bug 1489214 too or are we ok with just that
> one?

Please keep bug 1489214, that's the one that was causing the most support requests. The reason I requested that we uplift both patches was to have a consistent behavior across all of 62-64 to reduce confusion when dealing with (potential) future bug reports. I'm fine with wontfix here if that's the direction you want to take. That said, RyanVM offered to run the rebased patch through try, so we may have a fixed patch soon.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Looks like that modification works!
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=d95f1020f0f38a96f03bbb1764a97f2bef800ab7

Comment 21

[Florian Quèze [:florian]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Looks like that modification works!
> https://treeherder.mozilla.org/#/
> jobs?repo=try&group_state=expanded&revision=d95f1020f0f38a96f03bbb1764a97f2be
> f800ab7

Julien, should this rebased version land asap, or do you prefer to wontfix this for 62 at this point? (either is fine with me)

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9008372 [details] [diff] [review]
Patch

I'll land the rebased fix, thanks.

Comment 23

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-release/rev/df0865bc3084

Comment 24

[Daniel Bodea [:danibodea]]

I have reproduced the issue on Firefox Release v62.0 and I have verified the fix in Firefox Release Candidate v62.0.2 on Windows 10.
Uplift successful. Thank you.