Closed Bug 1557587 Opened 2 years ago Closed 1 year ago

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

Categories

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

x86_64
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox69 --- wontfix
firefox73 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

Details

Attachments

(2 files, 3 obsolete files)

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.

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.

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.

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.

Component: Window Management → Session Restore
Product: Core → Firefox

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.)

(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.

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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(mdeboer)

(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/

Perhaps, DOM Core is the right component.

Component: Session Restore → DOM: Core & HTML
Flags: needinfo?(mdeboer)
Product: Firefox → Core
Attachment #9071874 - Attachment is obsolete: true
Priority: -- → P3

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.)

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.

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.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

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.

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

Attachment #9076711 - Attachment is obsolete: true
Attachment #9076709 - Attachment is obsolete: true

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

Attachment #9110581 - Attachment description: Bug 1557587 - part 2: Make `nsGlobalWindowOuter` compute devices pixels with `nsIBaseWindow` r=jfkthame! → Bug 1557587 - part 2: Make `nsGlobalWindowOuter` convert between devices pixels and CSS pixels with scale information stored in `nsIBaseWindow` r=jfkthame!
Pushed by masayuki@d-toybox.com:
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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
QA Whiteboard: [qa-73b-p2]
You need to log in before you can comment on or make changes to this bug.