1607088 - [Wayland] Frequent crashes under gdk_wayland_window_ref_cairo_surface when spawning the inspector on a different window

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

See https://crash-stats.mozilla.org/report/index/1380b5ff-358d-4f46-a851-e36cb0200105 for example.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is with WebRender, fwiw.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This is the assert: assert (CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&surface->ref_count));, so either we're reffing the surface too late, or too early. May be a gdk bug...

Comment 3

[Val Packett]

I've been seeing a different cairo assert, not related to the inspector or webrender, just sometimes when launching. Only after recent updates.

(firefox:14792): Gdk-WARNING **: 20:28:20.829: (gdkwindow-wayland.c:810):buffer_release_callback: runtime check failed: (impl->staging_cairo_surface != cairo_surface)
Assertion failed: (status == cr->status), function _cairo_create_in_error, file cairo.c, line 412.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I managed to catch this one under rr, so tentatively taking.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(rr) p/x surface->ref_count.ref_count
$4 = 0xe5e5e5e5

Yeah that's not good.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

This is failing: https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-24/gdk/wayland/gdkwindow-wayland.c#L810

And we're freeing a surface too early, I think.

Comment 7

[Jan Alexander Steffens [:heftig]]

I think this might be a regression caused by the wl_surface_commit added by bug 1606776, perhaps in combination with the wl_surface_commit added by bug 1542808, which possibly runs on another thread. GTK and Wayland are not thread-safe.

Comment 8

[Jan Alexander Steffens [:heftig]]

I see asserts from the line Emilio pointed out (but with the crash signature from bug 1607033) doing various things, like trying to dock the inspector, opening the page info dialog, or opening an external link.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jan Alexander Steffens [:heftig] from comment #7)

I think this might be a regression caused by the wl_surface_commit added by bug 1606776, perhaps in combination with the wl_surface_commit added by bug 1542808, which possibly runs on another thread. GTK and Wayland are not thread-safe.

I do think it's because of that bug, but I don't think it's a threading issue. At least I don't see anything relevant going on off the main thread. What I see is that we're hitting this after that wl_surface_commit runs, which confuses gtk.

So I think that commit added by bug 1606776 is just messing up with the gdk state (at least when gdkwindow's state is in pending_buffer_attached state), making wayland send a wl_release_buffer that gdk thinks it is due to its own commit but it's not.

Now what the right thing to do is, I don't know. It may be that we just need to addref the cairo surface so that gdk releases it on their buffer_release_callback, or it may be a deeper problem...

Comment 10

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

We need to force Gtk to re-commit GdkWindow wl_surface to compositor. We may use frame begin/end or request frame clock repaint, whatever it takes. I'll look at it.

Comment 11

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

Attachment: Bug 1607088 [Wayland] Request mShell window repaint instead of direct commit to wl_surface, r?heftig

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3aaf0cc32ae9
[Wayland] Request mShell window repaint instead of direct commit to wl_surface, r=heftig

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3aaf0cc32ae9

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval if necessary.

Comment 17

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

Comment on attachment 9118971 [details]
Bug 1607088 [Wayland] Request mShell window repaint instead of direct commit to wl_surface, r?heftig

Beta/Release Uplift Approval Request

• User impact if declined: Crashes on Wayland backend caused by commit to wl_surface owned by Gtk.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1606751
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Linux/Wayland only.
• String changes made/needed: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9118971 [details]
Bug 1607088 [Wayland] Request mShell window repaint instead of direct commit to wl_surface, r?heftig

Wayland crash fix. Approved for 73.0b3.

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/74b15b3a6591