1446343 - Session restore does not preserve dimensions of minimized windows

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Benjamin Mullins]

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

Steps to reproduce:

1. Starting with at least two windows open, minimize one.
2. Exit Firefox from the menu.
3. Launch Firefox. Restore the previous session.


Actual results:

The minimized window is restored at a smaller size than it was when Firefox was closed.


Expected results:

The minimized window should be restored with the original dimensions it had.

---

I can reproduce this bug on the beta (60b) and nightly (61a) branches on Windows 10. I was not able to reproduce this bug on stable (59). I was not able to reproduce this bug on any version of Firefox in Linux.

I noticed the window that I sometimes have minimized started exhibiting this behavior perhaps a couple weeks ago, but didn't work out the exact nature of the bug until now. For most of the 60a release cycle of Nightly this did not occur.

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Firefox: 61.0a1, Build ID: 20180321102906

I have managed to reproduce this issue on latest Beta (60.0b5)and latest Nightly (60.a1) build. The issue is also reproducible if you restart the browser using the "restart" command in Developer Toolbar (F12). 
I have managed to reproduce the issue on Windows 7 x64, Windows 10 x64 and Mac 10.12 but I haven't manage to reproduce it on Arch Linux.

Indeed the issue is not reproducible on latest Firefox (59.0.1) release. Considering this I have used the mozregression tools and I have found the regression range. Here are the results:

Last good revision: 5ceb1365ae756eabe0763de42cb64fd9dc3e79dc
First bad revision: 7d40106bafebdd868ca7f6d188b5f0e30cb6f0e3
Pushlog: https://goo.gl/dk4VGv

From the provided pushlog, looks like bug 1442521 introduce the issue.
Xidorn can you please take a look at this?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

It is expected that bug 1442521 doesn't affect linux because on linux the outer size and inner size are identical (we don't count window decoration in the outer size, unlike on other systems).

Sounds like the inner sizes are incorrectly applied as outer size somehow.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Since widget code is very tricky to fix (as shown with my recent experience), I'd actually prefer we just backout bug 1442521 from 60beta, and we can continue fixing the regressions from that in 61.

Given that we decided not to uplift bug 1439875 to 60, there is no need for that patch to stay there.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Fixed by backout for Fx60.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

I can reproduce this on Windows. This is probably that there is some inconsistency in session store component.

I inspected a sessionstore file, and it turns out it saves different size for window minimized. Snippets below:
> "windows": [
>   {
>     // ...
>     "busy": false,
>     "width": 1296,
>     "height": 1079,
>     "screenX": 4,
>     "screenY": 40,
>     "sizemode": "normal"
>   },
>   {
>     // ...
>     "busy": false,
>     "width": "1280",
>     "height": "1040",
>     "screenX": "8",
>     "screenY": "41",
>     "sizemode": "minimized"
>   }
> ],

These two windows ought to have the same size, but they are serialized to different sizes. Interestingly, they use different types (normal uses numbers, while minimized one uses strings).

So this is probably related to where session store reads the information from.

Leave the ni? and I'll continue investigating.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

So... the related code is SessionStoreInternal._getWindowDimension[1]. It tries to query the window outer size, but instead uses attributes on document element when the sizemode is not normal.

And this is regressed by bug 1442521 because the meaning of attributes on document element was changed there.

We've handled similar situation in bug 1444525 for XUL persistence, and probably we need to do something alike for session store.


[1] https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/browser/components/sessionstore/SessionStore.jsm#4333-4363

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

I have some idea.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1446343 part 1 - Expose GetOuterToInner{Width,Height}DifferenceInCSSPixels to script.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1446343 part 2 - Convert size attributes to outer size for sessionstore.

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8965557 [details]
Bug 1446343 part 1 - Expose GetOuterToInner{Width,Height}DifferenceInCSSPixels to script.

https://reviewboard.mozilla.org/r/234350/#review239972

::: xpfe/appshell/nsIXULWindow.idl:73
(Diff revision 1)
> -  [noscript,notxpcom] uint32_t getOuterToInnerHeightDifferenceInCSSPixels();
> -  [noscript,notxpcom] uint32_t getOuterToInnerWidthDifferenceInCSSPixels();
> +  uint32_t getOuterToInnerHeightDifferenceInCSSPixels();
> +  uint32_t getOuterToInnerWidthDifferenceInCSSPixels();

If you make these [infallible] readonly attributes, you can at least keep calling the nicer signatures in C++...

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Seems macOS will fail the screen{X,Y} test... Let's just check width and height for now :/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965557 [details]
Bug 1446343 part 1 - Expose GetOuterToInner{Width,Height}DifferenceInCSSPixels to script.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965558 [details]
Bug 1446343 part 2 - Convert size attributes to outer size for sessionstore.

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

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8965558 [details]
Bug 1446343 part 2 - Convert size attributes to outer size for sessionstore.

https://reviewboard.mozilla.org/r/234352/#review240052

Very close, hence the r- :) Thanks for fixing this!

This new test is failing on OSX, so you probably want to check out why that is. (On OSX, 'maximize' is basically Fullscreen mode on OSX with a transition and 'minimize' transitions as well, which might influence the values after the 'sizemodechange' event fires.)

::: browser/components/sessionstore/SessionStore.jsm:4347
(Diff revision 2)
>          return "normal";
>        }
>      }
>  
> -    var dimension;
> +    // If the sizemode is not normal, their dimension may be misleading,
> +    // so try to read from the attribute first.

nit: Can you spend a few words here to explain _why_ it may be misleading, or point to a bug/ documentation that'd explain it in more detail. Because I'm having difficulty understand the _why_ we need to jump through hoops like this in frontend code already :-(

::: browser/components/sessionstore/SessionStore.jsm:4355
(Diff revision 2)
> +      let attr = parseInt(docElem.getAttribute(aAttribute), 10);
> +      if (attr) {
> +        if (aAttribute != "width" && aAttribute != "height") {
> +          return attr;
> +        }
> +        // width and height attribute are inner size, but we want to

nit: "Width and height attributes report the inner size,"

::: browser/components/sessionstore/SessionStore.jsm:4361
(Diff revision 2)
> +        // store the outer size, so add the difference.
> +        let xulWin = aWindow.getInterface(Ci.nsIDocShell)
> +                            .treeOwner
> +                            .QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIXULWindow);
> +        let diff = aAttribute == "width"

Perhaps it'd be nice to shorten this to:
```js
return attr + xulWin[`outerToInner${aAttribute.charAt(0).toUpperCase() + aAttribute.substr(1)}DifferenceInCSSPixels`];
```

or

```js
return attr + xulWin[`outerToInner${aAttribute == "width" ? "Width" : "Height"}DifferenceInCSSPixels`];
```
?

::: browser/components/sessionstore/SessionStore.jsm:4371
(Diff revision 2)
> +    }
> +
> +    let dimension;
>      switch (aAttribute) {
>      case "width":
>        dimension = aWindow.outerWidth;

We might as return the values from inside the `switch` statement and skip storing it in a temp variable.

::: browser/components/sessionstore/test/browser_1446343-windowsize.js:4
(Diff revision 2)
> +add_task(async function test() {
> +  const win = await BrowserTestUtils.openNewBrowserWindow();
> +  registerCleanupFunction(function() {
> +    BrowserTestUtils.closeWindow(win);

Instead of using `registerCleanupFunction`, can you move `await BrowserTestUtils.closeWindow(win)` to the end of this function?

::: browser/components/sessionstore/test/browser_1446343-windowsize.js:21
(Diff revision 2)
> +  const {outerWidth, outerHeight} = win;
> +  function checkCurrentState(sizemode) {
> +    let state = JSON.parse(ss.getWindowState(win));
> +    let winState = state.windows[0];
> +    let msgSuffix = ` should match on ${sizemode} mode`;
> +    is(winState.width, outerWidth, "width" + msgSuffix);

nit: Can you use `Assert.equal` here?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965558 [details]
Bug 1446343 part 2 - Convert size attributes to outer size for sessionstore.

https://reviewboard.mozilla.org/r/234352/#review240052

That may be done in a separate bug, because I don't think they are related to this bug. If you want, I can keep the test and make them conditionally expected fail for macOS.

> nit: Can you spend a few words here to explain _why_ it may be misleading, or point to a bug/ documentation that'd explain it in more detail. Because I'm having difficulty understand the _why_ we need to jump through hoops like this in frontend code already :-(

This is existing logic, which I just expand a bit...

Basically, we want to persist the size / position in normal state, because even if the window is maximized / minimized, user may still want to restore them to the size it was previously.

The attributes on window object are for the current state, and thus not the size / position we want the window to restore to. In that case, we assume that the corresponding attributes on the root element has the values we want.

> Perhaps it'd be nice to shorten this to:
> ```js
> return attr + xulWin[`outerToInner${aAttribute.charAt(0).toUpperCase() + aAttribute.substr(1)}DifferenceInCSSPixels`];
> ```
> 
> or
> 
> ```js
> return attr + xulWin[`outerToInner${aAttribute == "width" ? "Width" : "Height"}DifferenceInCSSPixels`];
> ```
> ?

I... don't really think it is more readable than a simple ternary expression...

> Instead of using `registerCleanupFunction`, can you move `await BrowserTestUtils.closeWindow(win)` to the end of this function?

I believe `registerCleanupFunction` is more robust, in the sense that even if there is any exception happens inside the test, the window would still be closed properly.

Also, having the cleanup code closer to the initialization code may make stuff easier to reason about.

> nit: Can you use `Assert.equal` here?

How is this different from `is`? How can I express `todo_is` with `Assert` (for macOS `screenX` and `screenY` in case you want)?

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> That may be done in a separate bug, because I don't think they are related
> to this bug. If you want, I can keep the test and make them conditionally
> expected fail for macOS.

Sure, but since you're introducing the test file in this bug and land it whilst perma-failing on OSX, you'll get backed out for sure... that's basically why I made the remark. Please feel free to skip the assertions on OSX.

> This is existing logic, which I just expand a bit...
> 
> Basically, we want to persist the size / position in normal state, because
> even if the window is maximized / minimized, user may still want to restore
> them to the size it was previously.
> 
> The attributes on window object are for the current state, and thus not the
> size / position we want the window to restore to. In that case, we assume
> that the corresponding attributes on the root element has the values we want.

I understand now :) It'd be real nice to have this explanation near the code as well!

> I... don't really think it is more readable than a simple ternary
> expression...

Up to you, really, that's why I noted it as a nit (I believe).

> I believe `registerCleanupFunction` is more robust, in the sense that even
> if there is any exception happens inside the test, the window would still be
> closed properly.
> 
> Also, having the cleanup code closer to the initialization code may make
> stuff easier to reason about.

If there's an exception thrown inside the test, we'd be in trouble anyways, so a leaking window will be the least of our worries.
In new tests we basically shy away from `registerCleanupFunction` and use the more linear approach, i.e. without the branching with callbacks. The async...await style has simplified the code flow in tests considerably that they're much more readable, thus having a cleanup phase at the end is quite easy to reason about.

> How is this different from `is`? How can I express `todo_is` with `Assert`
> (for macOS `screenX` and `screenY` in case you want)?

It's not different at all, only stylistic. This means that it's up to you what you prefer. I made it so that `is()` uses Assert.jsm under the hood anyway.
There's no 'todo' functionality in the Assert namespace, so you won't be able to. But I'm not a fan of todo's in code regardless, because they tend to never get resolved. In these cases I prefer to handle exceptions as:
```js
if (AppConstants.platform != "macosx") {
  //...
}
```

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Mike de Boer [:mikedeboer] (Back! Processing backlog...) from comment #16)
> > How is this different from `is`? How can I express `todo_is` with `Assert`
> > (for macOS `screenX` and `screenY` in case you want)?
> 
> It's not different at all, only stylistic. This means that it's up to you
> what you prefer. I made it so that `is()` uses Assert.jsm under the hood
> anyway.
> There's no 'todo' functionality in the Assert namespace, so you won't be
> able to. But I'm not a fan of todo's in code regardless, because they tend
> to never get resolved. In these cases I prefer to handle exceptions as:

I believe part of the value of 'todo' is that, when it is accidently fixed by someone else, we can catch that on the CI and start enforcing it.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965558 [details]
Bug 1446343 part 2 - Convert size attributes to outer size for sessionstore.

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

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8965558 [details]
Bug 1446343 part 2 - Convert size attributes to outer size for sessionstore.

https://reviewboard.mozilla.org/r/234352/#review240098

Thanks!

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965558 [details]
Bug 1446343 part 2 - Convert size attributes to outer size for sessionstore.

https://reviewboard.mozilla.org/r/234352/#review240098

Thanks for the quick review!

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8965557 [details]
Bug 1446343 part 1 - Expose GetOuterToInner{Width,Height}DifferenceInCSSPixels to script.

https://reviewboard.mozilla.org/r/234350/#review240258

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa7d70282996
part 1 - Expose GetOuterToInner{Width,Height}DifferenceInCSSPixels to script. r=bz
https://hg.mozilla.org/integration/autoland/rev/ba7856d07da4
part 2 - Convert size attributes to outer size for sessionstore. r=mikedeboer

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/aa7d70282996
https://hg.mozilla.org/mozilla-central/rev/ba7856d07da4

Comment 24

[Tom Bannister]

This issue is no longer reproducible on either Firefox 60.0b16 (20180426170554) on Windows 10x64 or on Firefox Nightly 61.0a1 (20180503220110) on Windows 10x64.

[bugday-20180502]

Comment 25

[Bogdan Maris, Desktop Test Engineering]

(In reply to Tom Bannister from comment #24)
> This issue is no longer reproducible on either Firefox 60.0b16
> (20180426170554) on Windows 10x64 or on Firefox Nightly 61.0a1
> (20180503220110) on Windows 10x64.
> 
> [bugday-20180502]

Thanks for verifying this issue. Marking accordingly.