1557587 - When I open 2 or more windows in different DPI monitors, window size in non-primary monitors are not restored correctly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

STR

1. Launch Firefox from taskbar in the primary display whose scale is over 100%
2. Make sure "Restore previous session" of "Startup" settings is checked
3. Open new window and move it into another display whose scale is 100%
4. Quit Firefox from menu
5. Launch Firefox from taskbar in the primary display

AR
The display in another display is restored bigger.

ER
The display in another display should be restored with the previous size.

If windows in other displays are snapped to screen edges, they are restored as expected sizes.

In my environment, the primary display is 200%, the other displays are 100%. So, the windows are restored twice bigger.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, it's odd. I realized that this bug is not reproduced only specific windows are in non-primary displays. I'll investigate what's different from no-problem windows.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, perhaps, I got it. If the left most window in taskbar is in non-primary display, it's restored in the primary display. Then, it'll be moved to correct display without size adjustment.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

According to my investigation, looks like that this is a bug of Session Restore.

When the first window is created, nsIWidget::Resize(double aWidth, double aHeight, bool aRepaint) is called immediately after creation.

Then, it'll be moved with nsIWidget::Resize(double aX, double aY, double aWidth, double aHeight, bool aRepaint). aX, aY, aWidth and aHeight are values which assume 1.0 as the scale (i.e., they are in logical pixels).

Finally, it'll be resized with nsIWidget::Resize(double aWidth, double aHeight, bool aRepaint). At this time, aWidth and aHeight computed with the display's scale which includes large area of the window.

So, the last call is redundant and called with wrong scaled values. When this is called, the stack is:

#01: nsGlobalWindowInner::ResizeTo (m:\src\dom\base\nsGlobalWindowInner.cpp:3523)
#02: mozilla::dom::Window_Binding::resizeTo (m:\fx64-dbg\dom\bindings\WindowBinding.cpp:4297)
#03: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (m:\src\dom\bindings\BindingUtils.cpp:3173)
#04: CallJSNative (m:\src\js\src\vm\Interpreter.cpp:448)
#05: js::InternalCallOrConstruct (m:\src\js\src\vm\Interpreter.cpp:540)
#06: InternalCall (m:\src\js\src\vm\Interpreter.cpp:595)
#07: Interpret (m:\src\js\src\vm\Interpreter.cpp:3087)
#08: js::RunScript (m:\src\js\src\vm\Interpreter.cpp:425)
#09: js::InternalCallOrConstruct (m:\src\js\src\vm\Interpreter.cpp:568)
#10: InternalCall (m:\src\js\src\vm\Interpreter.cpp:595)
#11: js::Call (m:\src\js\src\vm\Interpreter.cpp:611)
#12: JS::Call (m:\src\js\src\jsapi.cpp:2667)
#13: mozilla::dom::Function::Call (m:\fx64-dbg\dom\bindings\FunctionBinding.cpp:41)
#14: mozilla::dom::Function::Call<nsCOMPtr<nsISupports> > (m:\fx64-dbg\dist\include\mozilla\dom\FunctionBinding.h:73)
#15: nsGlobalWindowInner::RunTimeoutHandler (m:\src\dom\base\nsGlobalWindowInner.cpp:5803)
#16: mozilla::dom::TimeoutManager::RunTimeout (m:\src\dom\base\TimeoutManager.cpp:971)
#17: mozilla::dom::TimeoutExecutor::MaybeExecute (m:\src\dom\base\TimeoutExecutor.cpp:179)
#18: mozilla::dom::TimeoutExecutor::Run (m:\src\dom\base\TimeoutExecutor.cpp:236)
#19: nsThread::ProcessNextEvent (m:\src\xpcom\threads\nsThread.cpp:1165)
#20: NS_ProcessNextEvent (m:\src\xpcom\threads\nsThreadUtils.cpp:486)
#21: mozilla::ipc::MessagePump::Run (m:\src\ipc\glue\MessagePump.cpp:88)
#22: MessageLoop::RunHandler (m:\src\ipc\chromium\src\base\message_loop.cc:309)
#23: MessageLoop::Run (m:\src\ipc\chromium\src\base\message_loop.cc:291)
#24: nsBaseAppShell::Run (m:\src\widget\nsBaseAppShell.cpp:139)
#25: nsAppShell::Run (m:\src\widget\windows\nsAppShell.cpp:412)
#26: nsAppStartup::Run (m:\src\toolkit\components\startup\nsAppStartup.cpp:277)
#27: XREMain::XRE_mainRun (m:\src\toolkit\xre\nsAppRunner.cpp:4636)
#28: XREMain::XRE_main (m:\src\toolkit\xre\nsAppRunner.cpp:4775)
#29: XRE_main (m:\src\toolkit\xre\nsAppRunner.cpp:4856)
#30: NS_internal_main (m:\src\browser\app\nsBrowserApp.cpp:295)
#31: wmain (m:\src\toolkit\xre\nsWindowsWMain.cpp:131)
#32: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
#33: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x17974]
#34: RtlUserThreadStart[C:\WINDOWS\SYSTEM32\ntdll.dll +0x6a271]

So, somebody called window.resizeTo() with wrong value in JS.

I guess it's here.

nsGlobalWindowOuter::ResizeToOuter() treats aWidth and aHeight in CSS pixels. So that its caller shouldn't refer device pixels.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1557587 - Make `SessionStore` adjust restoring window size with current screen's resolution

SessionStore moves only first window and resize it after creation. Given
position and size are adjusted for the screen which the window will be moved.
However, Window.moveTo() may be handled asynchronously. I.e., when calling
Window.resizeTo(), the window may still be in different screen. So, when
current screen and destination screen have different device pixel ratio,
SessionStore restores the first window with wrong size.

This patch adjusts the window and height values in the destination screen
with current screen's device pixel ratio.

(I request review to Jonathan Kew since you fixed bug 1259707. I think
that this is what you forgot to fix at that time or new regression after that.)

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #4)

However, Window.moveTo() may be handled asynchronously. I.e., when calling
Window.resizeTo(), the window may still be in different screen.

If a web-page calls moveTo() to move a window to a different monitor and then it calls resizeTo(), the window size might be incorrect? If so, it should be fixed instead of adding a stopgap measure to the session restore module.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I'm not sure. I'll check it after getting back.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:mikedeboer, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masatoshi Kimura [:emk] from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #4)

However, Window.moveTo() may be handled asynchronously. I.e., when calling
Window.resizeTo(), the window may still be in different screen.

If a web-page calls moveTo() to move a window to a different monitor and then it calls resizeTo(), the window size might be incorrect? If so, it should be fixed instead of adding a stopgap measure to the session restore module.

Oh, you're right. A testcase is here (change the position for your environment and "Run" before clicking the button).
https://jsfiddle.net/d_toybox/hj816d2p/4/

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Perhaps, DOM Core is the right component.

Comment 10

[Masatoshi Kimura [:emk]]

Since UIResolutionChanged is async, resizeTo wouldn't see the DPI change yet:
https://searchfox.org/mozilla-central/rev/29f8e6aebd2b2660b9c76858ccb0c6eaf35dd7aa/layout/base/nsPresContext.cpp#1357

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, the comment 8's case is different bug... SessionRestore runs in the main process, but the testcase is caused by a bug of the communication between the processes. (Anyway, I have a patch for the latter, I'll request review for it in this bug.)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1557587 - part 1: Make `BrowserParent::RecvSetDimensions()` adjust given value with current scale

If nsGlobalWindowOuter::ResizeTo(), etc are called before receiving
BrowserChild::RecvUIResolutionChanged(), the aX, aY, aCx and aCy
values have been computed with old scale.

This patch makes BrowserChild::SendSetDimensions() set scale in the
remote process and BrowserParent::RecvSetDimensions() recompute each
value with current scale.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1557587 - part 2: Make `nsGlobalWindowOuter` compute devices pixels with `nsIBaseWindow`

Currently, nsGlobalWindowOuter computes device pixels from CSS pixels with
using PresContext::CSSPixelsToDevPixels() for calling nsIBaseWindow methods.
However, this may be non-latest information of corresponding widget.
Therefore, it needs to compute device pixels with nsIBaseWindow.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1557587 - part 1: Make `BrowserParent::RecvSetDimensions()` adjust given value with current scale r=jfkthame!

If nsGlobalWindowOuter::ResizeTo() etc is called before receiving
BrowserChild::RecvUIResolutionChanged(), the aX, aY, aCx and aCy
values are computed with old scale.

This patch makes BrowserChild::SendSetDimensions() set scale in the
remote process and BrowserParent::RecvSetDimensions() recompute each
value with current scale.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1557587 - part 2: Make `nsGlobalWindowOuter` convert between devices pixels and CSS pixels with scale information stored in `nsIBaseWindow` r=jfkthame!

Currently, nsGlobalWindowOuter computes devices pixels from CSS pixels with
using PresContext::CSSPixelsToDevPixels() for calling nsIBaseWindow methods.
However, this may be not latest information of the corresponding widget.
Therefore, it needs to compute device pixels with nsIBaseWindow.

Depends on D54150

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The new patches are just updated for the latest mozilla-central, and they still work fine on my environment.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c15fd9b7ccb7
part 1: Make `BrowserParent::RecvSetDimensions()` adjust given value with current scale r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/cd8ecd10ded3
part 2: Make `nsGlobalWindowOuter` convert between devices pixels and CSS pixels with scale information stored in `nsIBaseWindow` r=jfkthame

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c15fd9b7ccb7
https://hg.mozilla.org/mozilla-central/rev/cd8ecd10ded3