1880323 - Moving the last tab of a window to another window hides its contents

Status: VERIFIED FIXED

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

Description

[tgn-ff]

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

Steps to reproduce:

1. Open a URL, eg. https://bugzilla.mozilla.org, in a new window. It should be the only tab of the window.
2. Move its tab to another, pre-existing window.

KDE, X11.

Actual results:

The contents of the tab are not displayed any more.

Expected results:

This was introduced in Bug 1875369.

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0efc1d8b9bd6dc8d8e6947435a0540b229062c57&tochange=039d034ac81d93d29151bc8bf573999eca8d9200

Comment 1

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

:stransky, since you are the author of the regressor, bug 1875369, could you take a look?

For more information, please visit BugBot documentation.

Comment 2

[Alice0775 White]

An error in Browser Console when drop the tab:

InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable AboutNewTabParent.sys.mjs:28
    handleEvent resource:///actors/AboutNewTabParent.sys.mjs:28
    swapDocShells chrome://global/content/elements/browser-custom-element.js:1577
    _swapBrowserDocShells chrome://browser/content/tabbrowser.js:4749
    swapBrowsersAndCloseOther chrome://browser/content/tabbrowser.js:4634
    adoptTab chrome://browser/content/tabbrowser.js:5163
    on_drop chrome://browser/content/tabbrowser-tabs.js:808
    handleEvent chrome://browser/content/tabbrowser-tabs.js:1853

Comment 3

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

I do see that on Wayland too, Thanks.

Comment 4

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

It's a regression from https://phabricator.services.mozilla.com/D199085 will look at it.

Comment 5

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

It's caused by window->DisableRendering(); line at moz_container_unmap(). May we share compositor widget here?

Comment 6

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

Looks like caused by early mCompositorSession->Shutdown(); call on the windows which is going to be closed. If the tab is just copied we don't see such error. I wonder if we shutdown something related to puppet window or IPC channel so the reparented content page rendering is broken.

It doesn't affect in-process rendered pages line about:support and so.

Comment 8

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

Attachment: Bug 1880323 [Linux] Don't destroy layer manager at DisableRendering() r?emilio

Right now we destroy layer managed when nsWindow becomes hidden.
It makes sure that window rendering queue is suspended and invalid wl_surface is not used any more.
But it's also expensive operation and we need to create layer manager again if the window becomes visible again.
It also introduces a regression it tab is reparented and remairing nsWindow is closed.
In such case rendering pipeline is deleted and reparented tab is empty as associated rendering resources are already freed.

In this patch we don't delete layer manager on window hide but rather keep that operation to nsWindow::Destroy() as we do before.
Instead force GL backend to create small fallback EGLSurface for hidden nsWindow and keep potential async rendering running
until the window is deleted.

Comment 9

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

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdcedb217978
[Linux] Don't destroy layer manager at DisableRendering() r=emilio

Comment 11

[pstanciu]

Backout for causing failures in nsWindow.cpp.

• Backout link
• Push with failures
• Failure Log
• Failure line: SUMMARY: ThreadSanitizer: data race /builds/worker/checkouts/gecko/widget/gtk/nsWindow.cpp:9880:16 in nsWindow::DisableRendering()

Comment 12

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

Attachment: Bug 1880323 [Linux] Make nsWindow::GetTitlebarRect() thread safe r?emilio

• Implement nsWindow::SetTitlebarRect() and update titlebar area when nsWindow is resized.
• Make GetTitlebarRect() to only return value calculated at SetTitlebarRect().
• Protect both by mutex to make it thread safe.

Depends on D202179

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e6c8078c017
[Linux] Don't destroy layer manager at DisableRendering() r=emilio
https://hg.mozilla.org/integration/autoland/rev/7346a5d91e93
[Linux] Make nsWindow::GetTitlebarRect() thread safe r=emilio

Comment 14

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

Should be covered now.

Comment 15

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/0e6c8078c017
https://hg.mozilla.org/mozilla-central/rev/7346a5d91e93

Comment 17

[tgn-ff]

The work in Bug 1875369 had fixed Bug 1730819 which has regressed again after the fix in this (1880323) bug. Was either of those intentional?

Comment 18

[Intermittent Failures Robot]

3 failures in 3382 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 3

Platform and build breakdown:

• linux64: 3
  • opt: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1880323&startday=2024-02-19&endday=2024-02-25&tree=all

Comment 19

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

(In reply to tgn-ff from comment #17)

The work in Bug 1875369 had fixed Bug 1730819 which has regressed again after the fix in this (1880323) bug. Was either of those intentional?

Bug 1730819 is regressed by this one, right? Yes, that's expected.
I can create a specific patch to release compositor for popups only but we need something general to go with beta now.

Comment 20

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

Comment on attachment 9380902 [details]
Bug 1880323 [Linux] Don't destroy layer manager at DisableRendering() r?emilio

Beta/Release Uplift Approval Request

• User impact if declined: Missing page content if tab is moved between browsers.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1) Open a URL, eg. https://bugzilla.mozilla.org, in a new window. It should be the only tab of the window.

2. Move its tab to another, pre-existing window.
3. Moved tab should have content and should not be blank.

• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Changes window rendering shutdown sequence and leaves compositor opened until window destroy. It partially backouts Bug 1875369.
• String changes made/needed:
• Is Android affected?: No

Comment 21

[Dianna Smith [:diannaS]]

Comment on attachment 9380902 [details]
Bug 1880323 [Linux] Don't destroy layer manager at DisableRendering() r?emilio

Approved for 124.0b5

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/c40e08be848b
https://hg.mozilla.org/releases/mozilla-beta/rev/ae5b818393b8

Comment 23

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 124.0a1 (2024-02-14) on Ubuntu 23.10 with Wayland using steps from comment 20. The moved tab has blank content.
The issue is verified fixed with Firefox 125.0a1 (2024-02-27) and 124.0b5 treeherder build from comment 22 on Ubuntu 23.10 (Wayland and X11) and Ubuntu 22.04 (Wayland). The moved tab is correctly displayed after following steps from comment 20.