1844543 - nsWindow::SetEGLNativeWindowSize() and nsWindow::GetNativeData(NS_NATIVE_EGL_WINDOW) is not thread safe

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Martin Stránský [:stransky] (ni? me)]

nsWindow::SetEGLNativeWindowSize() and nsWindow::GetNativeData(NS_NATIVE_EGL_WINDOW) may be called from Renderer thread and need sync with Main thread.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Wayland] Add more scale changes logging r?emilio

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Wayland] Implement EGL size changes in MozContainer r?emilio

Move EGLWindow resize code from nsWindow to MozContainerWayland. It allows to use container lock and make the operation thread safe.

Depends on D184121

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Wayland] Make nsWindow::GdkCeiledScaleFactor() thread safe r?emilio

If GdkCeiledScaleFactor() is called from non-main thread (compositor, renderer etc.) just return recent scale factor and don't update it. If it's changed it will be updated from main thread later.

Depends on D184122

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Wayland] Pass window scale to moz_container_wayland_set_scale_factor_locked() directly r?emilio

Remove hidden GdkCeiledScaleFactor() call from moz_container_wayland_set_scale_factor_locked() and pass it directly there.
It reduces number of GdkCeiledScaleFactor() calls and make sure scale is not changed surprisingly.

G: changed widget/gtk/WindowSurfaceWaylandMultiBuffer.cpp

Depends on D184123

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Wayland] Pass window scale to moz_container_wayland_set_scale_factor_locked() directly r?emilio

Remove hidden GdkCeiledScaleFactor() call from moz_container_wayland_set_scale_factor_locked() and pass it directly there.
It reduces number of GdkCeiledScaleFactor() calls and make sure scale is not changed surprisingly.

Depends on D184122

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Wayland] Use nsWindow::GetCachedCeiledScaleFactor() to get scale factor from renderer/conpositor thread r?emilio

GdkCeiledScaleFactor() can't be called from non-main thread (compositor, renderer etc.).
In such case just return recent scale factor and don't update it.
If it's changed it will be updated from main thread later.

Depends on D184170

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Linux] Use mutex to ensure thread safe nsWindow destroy r?emilio

Depends on D184171

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/989758821905
[Wayland] Add more scale changes logging r=emilio

Comment 10

[Norisz Fay [:noriszfay]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/114fe5b2c6dd
[Wayland] Implement EGL size changes in MozContainer r=emilio
https://hg.mozilla.org/integration/autoland/rev/6c1f22c273b9
[Wayland] Pass window scale to moz_container_wayland_set_scale_factor_locked() directly  r=emilio
https://hg.mozilla.org/integration/autoland/rev/f19933e3f455
[Wayland] Use nsWindow::GetCachedCeiledScaleFactor() to get scale factor from renderer/conpositor thread r=emilio
https://hg.mozilla.org/integration/autoland/rev/8dbafcdb0be7
[Linux] Use mutex to ensure thread safe nsWindow destroy r=emilio

Comment 12

[Noemi Erli[:noemi_erli]]

Backed out 4 changesets (Bug 1844543) for causing bustages in nsWindow.h CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=Q--Q2U0xQsyR3sHrHR8FGg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=8dbafcdb0be71b032ffdb6e3fffc2f101636d5f5

Log: https://treeherder.mozilla.org/logviewer?job_id=424486558&repo=autoland&lineNumber=9287

Backout: https://hg.mozilla.org/integration/autoland/rev/1e3e1a5b6d6cf0c3011616048f77f721114381db

There are also this failures which appeared later on: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Ctsan%2Copt%2Creftest&tochange=b83cfea645752706c643567095d49b349cb9e610&fromchange=360e8f490c101dcdbf0a832d28a1da47c8b04ac1&selectedTaskRun=RiLDGt5KQmCpBWFuCfvKoA.0

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91e48d264a62
[Wayland] Implement EGL size changes in MozContainer r=emilio
https://hg.mozilla.org/integration/autoland/rev/3b42c79afd36
[Wayland] Pass window scale to moz_container_wayland_set_scale_factor_locked() directly  r=emilio
https://hg.mozilla.org/integration/autoland/rev/1a4518e0602b
[Wayland] Use nsWindow::GetCachedCeiledScaleFactor() to get scale factor from renderer/conpositor thread r=emilio
https://hg.mozilla.org/integration/autoland/rev/4921cebd7db4
[Linux] Use mutex to ensure thread safe nsWindow destroy r=emilio

Comment 14

[Cristian Tuns]

Backed out for causing multiple failures on Linux WebRender

• Backout link
• Push with failures
• Failure Log

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8552ae037335
[Wayland] Implement EGL size changes in MozContainer r=emilio
https://hg.mozilla.org/integration/autoland/rev/d37d5b5058f2
[Wayland] Pass window scale to moz_container_wayland_set_scale_factor_locked() directly  r=emilio
https://hg.mozilla.org/integration/autoland/rev/aa3cdac16780
[Wayland] Use nsWindow::GetCachedCeiledScaleFactor() to get scale factor from renderer/conpositor thread r=emilio

Comment 17

[Martin Stránský [:stransky] (ni? me)]

We need to use test&set atomic operation and not mutex.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Linux] Make nsWindow::mIsDestroyed atomic r?emilio

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Linux] Don't access EGLWindow of destroyed nsWindow r?emilio

Depends on D187027

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8552ae037335
https://hg.mozilla.org/mozilla-central/rev/d37d5b5058f2
https://hg.mozilla.org/mozilla-central/rev/aa3cdac16780

Comment 21

[msirringhaus]

https://hg.mozilla.org/mozilla-central/rev/aa3cdac16780 is causing Assertion failure: NS_IsMainThread(), at mozilla-central/widget/gtk/nsWindow.cpp:8900 for me on startup on sway.
So I can't start devel-builds right now with this change.

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1844543 [Linux] Use mutex to ensure thread safe nsWindow destroy r?emilio

Comment 23

[Martin Stránský [:stransky] (ni? me)]

try: https://treeherder.mozilla.org/jobs?repo=try&revision=d5c280e405cb1f2513e7eea14bb7bf473ebc7d7e

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5378e223b13e
[Linux] Use mutex to ensure thread safe nsWindow destroy r=emilio

Comment 25

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5378e223b13e

Comment 26

[Martin Stránský [:stransky] (ni? me)]

Last patches landed, closing.

Comment 27

[Tom Schuster (MoCo) | OOO back 1. Dec]

[Tracking Requested - why for this release]: Could we consider backporting this to ESR115? Especially the thread-safety fix for nsWindow.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Sounds like a question for Martin. It'd need a rebased patch if yes.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Tom, could you clarify why for comment 27? Not a lot of context there :)

Comment 30

[Martin Stránský [:stransky] (ni? me)]

(In reply to Tom Schuster (MoCo) from comment #27)

[Tracking Requested - why for this release]: Could we consider backporting this to ESR115? Especially the thread-safety fix for nsWindow.

We'd need to backport all the patches here. Not sure how much work is that.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

From what I can see, the patches in this bug do not all graft cleanly. I wouldn't be opposed to taking a rebased set of patches for ESR if the belief is that they help with some crashes tracked in other bugs, but we'll need those to be posted with approval requests for that to be considered.