1874766 - [KDE/wayland] Window rules to position PiP window broke after bug 1838045

Status: RESOLVED FIXED

(Toolkit :: Picture-in-Picture, defect, P3)

Description

[Emilio Cobos Álvarez [:emilio]]

Attachment: Rule to show PiP at the top left of my secondary monitor.

As described in bug 1767414 comment 3 and co, it seems to be a regression. I can repro with a rule like the attached one.

The first window gets positioned correctly. The next doesn't. Given the regressing bug, it's likely because:

• The window is shown right after the window load event.
• Bug 1767414 affects the load event timing for cached stylesheets, probably making the window show up sooner.
• Probably, the PiP window tries to resize itself, now after the load event, resetting the position.

I'm not sure (yet) whether this is a bug in the style system, or just something that the PiP code happened to rely on.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

It might be that KWin shouldn't reset the window position for wayland windows like this... Vlad, do you have any thoughts there?

Comment 2

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

Set release status flags based on info from the regressing bug 1838045

Comment 3

[Vlad Zahorodnii [:zzag]]

Does "Force" rule work? I suspect that the title might have been set after the initial surface commit. Remember and Apply Initially apply only when the window is unmapped, so it's important that kwin can identify windows when the pip surface is committed for the first time.

Comment 4

[Vlad Zahorodnii [:zzag]]

I can confirm that Apply Initially doesn't work

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Yes, "Force" does work. Yeah looks like setting the title is async, so my patch which made the load fire faster broke this in a sense.

Comment 6

[nf.pereira]

:emilio I suppose you don't need my window rules anymore now that you've managed to reproduce it.

Great to see some progress being made to fix this! Specially now that Firefox uses Wayland by default and window rules is the only way to manage the PiP window.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1874766 - Make title change events block onload in chrome docshells. r=smaug

See the comment as for why.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/830a664759c7
Make title change events block onload in chrome docshells. r=smaug

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2bfe7d3bd10
Fix build with -WError.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab8632fd1236
Fix win32 build.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6caaf7b98e6
Add a missing null-check.

Comment 12

[Cosmin Sabou [:CosminS]]

Backed out for causing frequent crashes @nsBaseWidget::UpdateZoomConstraints.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&fromchange=97df4d5cb5420f8a928bcb9a1c7fc4c69823df4e&searchStr=android%2C7.0%2Cx86-64%2Cwebrender%2Cdebug%2Cmochitests%2Ctest-android-em-7.0-x86_64-qr%2Fdebug-geckoview-mochitest-plain%2C10&group_state=expanded&selectedTaskRun=J-WmpPTCRXWzJnd08SUG-Q.0&tochange=c57d859580d036c47f9c0140e247b012211f6475

Failure log: https://treeherder.mozilla.org/logviewer?job_id=443510138&repo=autoland&lineNumber=2517

Backout link: https://hg.mozilla.org/integration/autoland/rev/7ecaaae315a8601ddbe0d1c53adf1f8f410ebd40

If it helps debugging a similar failure happened in https://bugzilla.mozilla.org/show_bug.cgi?id=1824886#c23 after https://hg.mozilla.org/integration/autoland/rev/4c734e252630 landed.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Hiro / botond, do you know what that assertion means and why delaying the load event a bit breaks it? Bug 1824886 comment 23 and following have a similar example.

It seems the assertion is harmless tho, as we are just overriding the previous zoom constraints (https://searchfox.org/mozilla-central/rev/1aa61dcd48e128a8cbfbe59b7ba43d31bd3c248a/widget/nsBaseWidget.cpp#1056-1068)

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1874766 - Relax assertion in nsBaseWidget::UpdateZoomConstraints. r=botond,hiro

It doesn't hold if the load event is delayed a bit, but it breaking
shouldn't change any behavior / shouldn't matter in practice.

Comment 15

[Botond Ballo [:botond]]

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

Hiro / botond, do you know what that assertion means and why delaying the load event a bit breaks it?

I think the idea behind APZ including the pres shell id in the key that it uses to look up objects like ZoomConstraints is that this is how it detects e.g. a new document being loaded which might have different zoom constraints.

I'm a bit fuzzy on what other conditions can cause the pres shell to be reconstructed (and therefore the pres shell id to be updated), but as mentioned in the review, looking at how this value is used later on, the change should be harmless.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I am not so familiar with the presshellID's restriction in the ZoomConstraints, so I am debugging what's going on there with D198684. Anyways, in short the assertion happens "about:blank" -> "about:blank" transition for some reasons, and more over the ViewID is also different so D198785 will not solve.

I will have a meeting right now, I will debug it further later.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e3e7f97caf3
Relax assertion in nsBaseWidget::UpdateZoomConstraints. r=botond

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Okay, for what's it's worth I am going to comment what I've found.

So it looks that the assertions are to make sure each valid ZoomConstrains in nsBaseWidget is reset before being updated with the new one. So for example, without D198684 nsBaseWidget::UpdateZoomConstraints gets called in the following order;

1. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, a valid ZoomConstraints)
2. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, Nothing())
3. UpdateZoomConstraints(aPresShellId = 14, aViewId = 8, a valid ZoomConstraints)

But with D198684;

1. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, a valid ZoomConstraints)
2. UpdateZoomConstraints(aPresShellId = 14, aViewId = 8, a valid ZoomConstraints)
3. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, Nothing())

I am not sure whether the assertion happens only on about:blank or not, if it happens on normal documents, we need the assertions there.

Comment 19

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)

But with D198684;

1. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, a valid ZoomConstraints)
2. UpdateZoomConstraints(aPresShellId = 14, aViewId = 8, a valid ZoomConstraints)
3. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, Nothing())

Huh, that's interesting, I would have thought the calls (2) and (3) come from here and so the Nothing() call comes first.

Anyways, like you said the patch retains the ViewID assertion so if this scenario happens in CI, it should still be caught.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

I've already removed the debug build locally, so I can't check it right now, I guess there are two different ZoomConstraintsClient instances.

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6e3e7f97caf3

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f61c77fafa8a
Make title change events block onload in chrome docshells. r=smaug

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f61c77fafa8a