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

VERIFIED FIXED in Firefox 62

Status

()

defect
VERIFIED FIXED
8 months ago
7 months ago

People

(Reporter: alice0775, Assigned: florian)

Tracking

({regression})

62 Branch
Firefox 64
All
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62+ verified, firefox63+ verified, firefox64+ verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
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?
Flags: needinfo?(florian)
Flags: needinfo?(emilio)
(Reporter)

Updated

8 months ago
Summary: Browser size gradually increases every restart if quit browser in maximized mode. → The browser size will increase with each reboot when you exit the browser in maximized mode.
(Assignee)

Comment 1

8 months ago
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?
Flags: needinfo?(florian)
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?
Flags: needinfo?(emilio)
(Assignee)

Comment 3

8 months ago
Posted patch PatchSplinter Review
Thanks Emilio! Yes, using that API solves the problem.
Attachment #9008372 - Flags: review?(mconley)
(Assignee)

Updated

8 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 4

8 months ago
The patch is simple enough that I think we should uplift at least to beta.
Hardware: x86_64 → All
Version: 61 Branch → 62 Branch
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!
Attachment #9008372 - Flags: review?(mconley) → review+

Comment 6

7 months ago
Pushed by florian@queze.net:
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.
(Assignee)

Updated

7 months ago
Flags: qe-verify+

Comment 7

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/690dfc98faff
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Assignee)

Comment 8

7 months ago
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.
Attachment #9008372 - Flags: approval-mozilla-beta?
Comment on attachment 9008372 [details] [diff] [review]
Patch

Low risk patch fixing a regression, uplift approved for 63 beta 6, thanks.
Attachment #9008372 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
This issue is now fixed in Beta v63.0b6 on Windows 10. 
Uplift successful. Thank you!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 9008372 [details] [diff] [review]
Patch

approved for 62.0.2 based on bug 1489214 comment 21
Attachment #9008372 - Flags: approval-mozilla-release+
Putting the qe-verify+ flag back for verification process.
Flags: qe-verify+
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?
Flags: needinfo?(florian)
Attachment #9008372 - Flags: approval-mozilla-release+
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? :-)
Flags: needinfo?(jcristau)
RyanVM ran a set try pushes with various backouts and this one ended up green.
Flags: needinfo?(jcristau)
(Assignee)

Comment 19

7 months ago
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.
Flags: needinfo?(florian)
(Assignee)

Comment 21

7 months ago
(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)
(Assignee)

Updated

7 months ago
Flags: needinfo?(jcristau)
Comment on attachment 9008372 [details] [diff] [review]
Patch

I'll land the rebased fix, thanks.
Flags: needinfo?(jcristau)
Attachment #9008372 - Flags: approval-mozilla-release+
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.