1786048 - Setting innerWidth/Height multiple times can apply the delta multiple times.

Status: VERIFIED FIXED

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

Description

[matc]

Attachment: set_innerWidth_twice.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

onclick = () => {
  win = open(document.location.href, "_blank", "width=200,height=200");
  win.onload = () => {
    win.innerWidth = 300;
    win.innerWidth = 300;
  };
};

Actual results:

win.innerWidth ends up being 400.

Expected results:

win.innerWidth should end up being 300.

Comment 1

[matc]

This is easier to reproduce on Windows than on Linux. Setting innerWidth causes a child to parent ipc message with the current and the requested size. The parent then changes the window size by the delta between the two sizes. If the window is already resized when the second message is processed, the parent also applies that delta. On Linux the resize only happens synchronously while the window is not yet fully visible.

Comment 2

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[matc]

I was playing around with this problem a bit and noticed other related problems. One is with outer sizes where changing back to the previous size is ignored.

let current = win.outerWidth;
win.outerWidth = current + 10;
win.outerWidth = current;
// outerWidth ends up at current + 10

This is BrowserChild::SetDimensions detecting the second change as an ignored dimension, because the first change has not completed yet. This could be solved making nsGlobalWindowOuter set the flags.

Also on Linux it is a general problem that a resize does not complete immediately. This breaks simple patterns like window.outerWidth = 100; window.outerHeight = 200; in both chrome and content. The second resize simply overwrites the first one.

Comment 4

[matc]

Attachment: Bug 1786048 - Part 1: AppWindow can't skip SetSize calls, HeadlessWidget has to notify when moved. r=emilio

Two minor changes that otherwise might go unnoticed in the following
parts:

• AppWindow can't skip SetSize calls that match the current size. On
  Linux a previous call might not have changed the size yet. If the
  current call is skipped, the previous call can ultimately dictate the
  resulting size.

• BrowserParent should not have to call UpdatePosition when receiving
  new dimensions from BrowserChild. But HeadlessWidget needs to call
  NotifyWindowMoved when moved.

Comment 5

[matc]

Attachment: Bug 1786048 - Part 3: Merge nsIEmbeddingSiteWindow into nsIBaseWindow. r=emilio

BrowserChild might have an outdated view of the current size, which
makes auto detecting the unchanged dimensions inaccurate. E.g.
immediately reverting a change can be ignored when auto detected.

To support setting inner dimensions through nsIEmbeddingSiteWindow, we
have to split nsSiteWindow into a chrome and content part. The portion
of the interface shared with nsIBaseWindow no longer needs interface
specific implementations. The effective implementation for
Get/SetDimensions lives in AppWindow with an additional boolean to
indicate whether the caller is chrome or content. This is also in
preparation to work around Linux specific behaviour.

Depends on D160259

Comment 6

[matc]

Attachment: Bug 1786048 - Part 4: Remove cross process SizeShellTo. r=emilio

Because the parent process lacks information about the current shell
size, the child has to send both the current and the new shell size to
the parent. The parent then applies the delta to the window size. This
can produce different results for calls with the same arguments,
whenever a previous call did not have enough time to update the child
with its new size.

The implementation is replaced by applying the delta in the child.

Depends on D160260

Comment 7

[matc]

Attachment: Bug 1786048 - Part 5: [Linux] Use substitution values explicitly provided by nsIWidget for unchanged dimensions. r=emilio,stransky

This allows to account for outstanding changes on Linux. We can return
either the current or last requested size to be used for unchanged
dimensions. When we see a size-allocate signal that isn't in response to
one of our resizes we use the current size and otherwise the last
requested size.

This has a tendency to return the last requested size in cases where the
current size should be returned. E.g. after the window manager ignores a
request. But we can assume that repeating such a request will result in
the same size. After a 0.5s timeout we fall back to returning the
current size.

For positions we don't have a clear indicator for a completed move, we
rely on using only the timeout.

The only change in behaviour for nsWindow is that it now can be moved to
its current position when that position is not the last requested
position.

Depends on D160261

Comment 8

[matc]

Attachment: Bug 1786048 - Part 2: Use layout device pixels for AppWindow primary content size. r=emilio

The getter used to return CSS pixels, while the setter expected layout
device pixels. The nsIDocShellTreeOwner documentation suggests that
layout device pixels are to be used in both cases.

Depends on D160259

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0750c885a688
Part 1: AppWindow can't skip SetSize calls, HeadlessWidget has to notify when moved. r=emilio
https://hg.mozilla.org/integration/autoland/rev/205f129c648a
Part 2: Use layout device pixels for AppWindow primary content size. r=emilio
https://hg.mozilla.org/integration/autoland/rev/17a9990d6d19
Part 3: Merge nsIEmbeddingSiteWindow into nsIBaseWindow. r=emilio
https://hg.mozilla.org/integration/autoland/rev/318a2d2e6e39
Part 4: Remove cross process SizeShellTo. r=emilio

Comment 10

[Marian-Vasile Laza]

Backed out 4 changesets (Bug 1786048) for causing bc failures on browser_modal_resize.js.
Backout link
Push with failures <--> bc8
Failure Log

Comment 11

[matc]

I filed bug 1803611 for the Linux specific issue. I plan to update Part 5 in that bug.

Part 5: [Linux] Use substitution values explicitly provided by nsIWidget for unchanged dimensions. r=emilio,stransky

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e71e8644b8a9
Part 1: AppWindow can't skip SetSize calls, HeadlessWidget has to notify when moved. r=emilio
https://hg.mozilla.org/integration/autoland/rev/999a18d6f33e
Part 2: Use layout device pixels for AppWindow primary content size. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f505df8a481a
Part 3: Merge nsIEmbeddingSiteWindow into nsIBaseWindow. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ae94135e68ef
Part 4: Remove cross process SizeShellTo. r=emilio

Comment 13

[Sandor Molnar[:smolnar]]

Backed out for causing WD failures in /webdriver/tests/fullscreen_window/fullscreen.py and mochitest failures in widget/tests/test_sizemode_events.xhtml

Backout link: https://hg.mozilla.org/integration/autoland/rev/8b10cc791d9f7719fe71748920465449dfac4fe0

Push with failures

Failure log WD

Failure log mochitest

Failure log bc

Comment 14

[Emilio Cobos Álvarez [:emilio]]

FYI, I'll try to land this on the 12th since we're on the soft freeze and these are not particularly risk-free nor urgent patches, please needinfo me if I forget!

Comment 16

[matc]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #14)

FYI, I'll try to land this on the 12th since we're on the soft freeze and these are not particularly risk-free nor urgent patches, please needinfo me if I forget!

The requested needinfo.

Also added another small change in widget/tests/test_sizemode_events.xhtml to no longer skip the "resize from maximized". The issue was that the test only shrunk the window by 10px.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Thanks, queued!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7306b42464cd
Part 1: AppWindow can't skip SetSize calls, HeadlessWidget has to notify when moved. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6396fa5ba6e7
Part 2: Use layout device pixels for AppWindow primary content size. r=emilio
https://hg.mozilla.org/integration/autoland/rev/249b7e625020
Part 3: Merge nsIEmbeddingSiteWindow into nsIBaseWindow. r=emilio
https://hg.mozilla.org/integration/autoland/rev/43d4de124e22
Part 4: Remove cross process SizeShellTo. r=emilio

Comment 19

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7306b42464cd
https://hg.mozilla.org/mozilla-central/rev/6396fa5ba6e7
https://hg.mozilla.org/mozilla-central/rev/249b7e625020
https://hg.mozilla.org/mozilla-central/rev/43d4de124e22

Comment 20

[Phabricator Automation]

Comment on attachment 9300191 [details]
Bug 1786048 - Part 5: [Linux] Use substitution values explicitly provided by nsIWidget for unchanged dimensions. r=emilio,stransky

Revision D160262 was moved to bug 1803611. Setting attachment 9300191 [details] to obsolete.

Comment 21

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Managed to reproduce on a 2022-12-15 Nightly build on macOS 12.
Verified as fixed on Firefox 110.0b5(build ID: 20230124185837) and Nightly 111.0a1(build ID: 20230125215201) on macOS 12, Windows 10, Ubuntu 22.