Bug 209345 – [GTK][PSON] Crash in WaylandCompositor::Surface destructor with USE_WPE_RENDERER disabled

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Дилян Палаузов]

With WebKitGTK 2.28 running under Epiphany, the latter crashes with this backtrace.

Moved from https://gitlab.gnome.org/GNOME/epiphany/issues/1132 .

Thread 1 (Thread 0x7f205dcdb280 (LWP 22706)):
#0  WebKit::WebPageProxy::viewWidget() (this=<optimized out>) at /src/gnome/webkitgtk-2.28.0/Source/WebKit/UIProcess/API/gtk/PageClientImpl.h:59
#1  0x00007f206b7f70fc in WebKit::WaylandCompositor::Surface::setWebPage(WebKit::WebPageProxy*) (this=this@entry=0x7f204eeeb480, webPage=webPage@entry=0x0) at /src/gnome/webkitgtk-2.28.0/Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:180
#2  0x00007f206b7f75b2 in WebKit::WaylandCompositor::Surface::~Surface() (this=0x7f204eeeb480, __in_chrg=<optimized out>) at /src/gnome/webkitgtk-2.28.0/Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:158
        pendingList = <optimized out>
        list = <optimized out>
#3  0x00007f206b7f7706 in WebKit::<lambda(wl_client*, wl_resource*, uint32_t)>::<lambda(wl_resource*)>::operator() (__closure=0x0, resource=<optimized out>) at /src/gnome/webkitgtk-2.28.0/Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:372
        surface = 0x7f204eeeb480
#4  WebKit::<lambda(wl_client*, wl_resource*, uint32_t)>::<lambda(wl_resource*)>::_FUN(wl_resource *) () at /src/gnome/webkitgtk-2.28.0/Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:373
#5  0x00007f2061bfd1df in destroy_resource (element=0x568ae40, data=<optimized out>, flags=0) at ../src/wayland-server.c:724
        resource = 0x568ae40
#6  0x00007f2061c01631 in for_each_helper (func=func@entry=0x7f2061bfd180 <destroy_resource>, data=data@entry=0x7ffdc5d131bc, entries=<optimized out>, entries=<optimized out>) at ../src/wayland-util.c:372
        start = <optimized out>
        end = 0x2475ef0
        p = 0x2475ec8
        ret = WL_ITERATOR_CONTINUE
#7  0x00007f2061c01b10 in wl_map_for_each (map=map@entry=0x56c1030, func=func@entry=0x7f2061bfd180 <destroy_resource>, data=data@entry=0x7ffdc5d131bc) at ../src/wayland-util.c:385
        ret = <optimized out>
#8  0x00007f2061bfd35e in wl_client_destroy (client=client@entry=0x56c1000) at ../src/wayland-server.c:883
        serial = 0
#9  0x00007f2061bfd413 in destroy_client_with_error (reason=<optimized out>, client=<optimized out>) at ../src/wayland-server.c:319
        client = 0x56c1000
        connection = <optimized out>
        resource = <optimized out>
        object = <optimized out>
        closure = <optimized out>
        message = <optimized out>
        p = {1857958179, 32544}
        resource_flags = <optimized out>
        opcode = <optimized out>
        size = <optimized out>
        since = <optimized out>
        len = <optimized out>
#10 wl_client_connection_data (fd=<optimized out>, mask=<optimized out>, data=0x56c1000) at ../src/wayland-server.c:342
        client = 0x56c1000
        connection = <optimized out>
        resource = <optimized out>
        object = <optimized out>
        closure = <optimized out>
        message = <optimized out>
        p = {1857958179, 32544}
        resource_flags = <optimized out>
        opcode = <optimized out>
        size = <optimized out>
        since = <optimized out>
        len = <optimized out>
#11 0x00007f2061bff0a2 in wl_event_loop_dispatch (loop=0x22e7610, timeout=timeout@entry=0) at ../src/event-loop.c:1027
        ep = {{events = 25, data = {ptr = 0x3b3fae0, fd = 62126816, u32 = 62126816, u64 = 62126816}}, {events = 0, data = {ptr = 0x7ffdc5d131f0, fd = -976145936, u32 = 3318821360, u64 = 140727922274800}}, {events = 80, data = {ptr = 0x0, fd = 0, u32 = 0, u64 = 0}}, {events = 0, data = {ptr = 0x22b7e30, fd = 36404784, u32 = 36404784, u64 = 36404784}}, {events = 3318821688, data = {ptr = 0xc5d1333c00007ffd, fd = 32765, u32 = 32765, u64 = 14254230628395417597}}, {events = 32765, data = {ptr = 0x7ffdc5d13340, fd = -976145600, u32 = 3318821696, u64 = 140727922275136}}, {events = 4294967295, data = {ptr = 0xc5d1333000000000, fd = 0, u32 = 0, u64 = 14254230576855777280}}, {events = 32765, data = {ptr = 0x22b7fc0, fd = 36405184, u32 = 36405184, u64 = 36405184}}, {events = 36405192, data = {ptr = 0x22b7fc000000000, fd = 0, u32 = 0, u64 = 156359074684862464}}, {events = 11, data = {ptr = 0x7ffdc5d13360, fd = -976145568, u32 = 3318821728, u64 = 140727922275168}}, {events = 1601089651, data = {ptr = 0x7f20, fd = 32544, u32 = 32544, u64 = 32544}}, {events = 13893632, data = {ptr = 0x7f205f6f94d8 <_dbus_platform_rmutex_unlock+24>, fd = 1601148120, u32 = 1601148120, u64 = 139777016829144}}, {events = 3318821728, data = {ptr = 0xc5d1338800007ffd, fd = 32765, u32 = 32765, u64 = 14254230954812932093}}, {events = 32765, data = {ptr = 0x7ffdc5d13360, fd = -976145568, u32 = 3318821728, u64 = 140727922275168}}, {events = 1601106952, data = {ptr = 0xc5d1336000007f20, fd = 32544, u32 = 32544, u64 = 14254230783014240032}}, {events = 32765, data = {ptr = 0x7ffdc5d13388, fd = -976145528, u32 = 3318821768, u64 = 140727922275208}}, {events = 3318821792, data = {ptr = 0x5f6fcd5f00007ffd, fd = 32765, u32 = 32765, u64 = 6876940963923656701}}, {events = 32544, data = {ptr = 0x300000002, fd = 2, u32 = 2, u64 = 12884901890}}, {events = 36406400, data = {ptr = 0xc5d133a000000000, fd = 0, u32 = 0, u64 = 14254231057892114432}}, {events = 32765, data = {ptr = 0x7f205f6cb94e <dbus_connection_unref+157>, fd = 1600960846, u32 = 1600960846, u64 = 139777016641870}}, {events = 36405104, data = {ptr = 0x22b848000000000, fd = 0, u32 = 0, u64 = 156364297365094400}}, {events = 0, data = {ptr = 0xc5d133bc, fd = -976145476, u32 = 3318821820, u64 = 3318821820}}, {events = 0, data = {ptr = 0xc5d133e000000003, fd = 3, u32 = 3, u64 = 14254231332770021379}}, {events = 32765, data = {ptr = 0x7f205f6cd409 <_dbus_connection_update_dispatch_status_and_unlock+257>, fd = 1600967689, u32 = 1600967689, u64 = 139777016648713}}, {events = 3318821856, data = {ptr = 0x224402000000000, fd = 0, u32 = 0, u64 = 154318793420570624}}, {events = 0, data = {ptr = 0x22ba040, fd = 36413504, u32 = 36413504, u64 = 36413504}}, {events = 1863211885, data = {ptr = 0x7f20, fd = 32544, u32 = 32544, u64 = 32544}}, {events = 0, data = {ptr = 0x1022b8480, fd = 36406400, u32 = 36406400, u64 = 4331373696}}, {events = 3318821920, data = {ptr = 0x6ebd507c00007ffd, fd = 32765, u32 = 32765, u64 = 7979622608276586493}}, {events = 32544, data = {ptr = 0x7ffdc5d13410, fd = -976145392, u32 = 3318821904, u64 = 140727922275344}}, {events = 8, data = {ptr = 0x220974000000000, fd = 0, u32 = 0, u64 = 153288688464297984}}, {events = 0, data = {ptr = 0x7ffdc5d13428, fd = -976145368, u32 = 3318821928, u64 = 140727922275368}}}
        source = <optimized out>
        i = <optimized out>
        count = <optimized out>
        has_timers = <optimized out>
#12 0x00007f206b7f69c3 in WebKit::<lambda(GSource*, GSourceFunc, gpointer)>::operator() (__closure=0x0, callback=<optimized out>, userData=<optimized out>, source=0x2630980) at /src/gnome/webkitgtk-2.28.0/Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:474
        wlLoopSource = 0x2630980
        events = <optimized out>
#13 WebKit::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer) () at /src/gnome/webkitgtk-2.28.0/Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:476
#14 0x00007f206f0c8bad in g_main_dispatch (context=0x2244020) at ../glib/gmain.c:3179
        dispatch = 0x7f206b7f6980 <WebKit::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer)>
        prev_source = 0x0
        was_in_call = 0
        user_data = 0x0
        callback = 0x0
        cb_funcs = 0x0
        cb_data = <optimized out>
        need_destroy = <optimized out>
        source = 0x2630980
        current = 0x2244110
        i = 2
        __FUNCTION__ = "g_main_dispatch"
#15 g_main_context_dispatch (context=context@entry=0x2244020) at ../glib/gmain.c:3844
#16 0x00007f206f0c8e00 in g_main_context_iterate (context=context@entry=0x2244020, block=block@entry=0, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3917
        max_priority = 0
        timeout = 0
        some_ready = 1
        nfds = <optimized out>
        allocated_nfds = <optimized out>
        fds = 0x2352db0
#17 0x00007f206f0c8e8f in g_main_context_iteration (context=context@entry=0x2244020, may_block=may_block@entry=0) at ../glib/gmain.c:3978
        retval = <optimized out>
#18 0x00007f206f2cbdca in g_application_run (application=0x223a720 [EphyShell], argc=<optimized out>, argv=<optimized out>) at ../gio/gapplication.c:2583
        arguments = 0x238a810
        status = 0
        context = 0x2244020
        acquired_context = 1
        __FUNCTION__ = "g_application_run"
#19 0x0000000000404b0e in main (argc=1, argv=0x7ffdc5d13768) at ../src/ephy-main.c:427
        option_context = 0x2220dc0
        option_group = 0x2220e40
        error = 0x0
        user_time = 0
        arbitrary_url = 0
        ctx = 0x2355220
        mode = EPHY_EMBED_SHELL_MODE_INCOGNITO
        status = 0
        flags = (EPHY_FILE_HELPERS_PRIVATE_PROFILE | EPHY_FILE_HELPERS_ENSURE_EXISTS | EPHY_FILE_HELPERS_STEAL_DATA)
        desktop_info = 0x0

[Michael Catanzaro]

I suspect that WaylandCompositor is incompatible with PSON, but I'm having trouble reproducing the crash. If anyone has a reproducer, that would be lovely.

[Дилян Палаузов]

I can provide whatever information you want, but I do not know by myself what information to provide.

Epiphany continues crashing occasionally after a while.

I have do add, that I have only Waylang/Weston without Gnome Shell, without XWayland (the latter also stopped magically working nearly at the same time when I upgraded to WebKitGTK 2.28).  And I ofter use Ctrl+Alt+F2 to exit the GUI and then come back.  In the past these forth and back switches lead to Epiphany crashing, but then this was fixed.  Could WebKit be more likely to crash, if it is moved in the background (leave GUI completely, change tab, or change application) during the webpage rendering?

[Guilaume Ayoub]

(In reply to Дилян Палаузов from comment #2)
> I can provide whatever information you want, but I do not know by myself
> what information to provide.
> 
> Epiphany continues crashing occasionally after a while.
> 
> I have do add, that I have only Waylang/Weston without Gnome Shell, without
> XWayland (the latter also stopped magically working nearly at the same time
> when I upgraded to WebKitGTK 2.28).  And I ofter use Ctrl+Alt+F2 to exit the
> GUI and then come back.  In the past these forth and back switches lead to
> Epiphany crashing, but then this was fixed.  Could WebKit be more likely to
> crash, if it is moved in the background (leave GUI completely, change tab,
> or change application) during the webpage rendering?

I regularly (+10 times a day) get the crash when I close a tab. Unfortunately, I can’t find a way to reproduce the bug.

[Michael Catanzaro]

OK, so with some help from Jan-Michael, I've managed to reproduce. The error occurs when closing a browser tab that was using AC mode and has had at least one process swap. It only happens when closing a single tab, not when closing the entire browser. 

To reproduce:

 * Build with -DUSE_WPE_RENDERER=OFF (workaround for bug #209118)
 * Open two browser tabs in Epiphany: one www.duckduckgo.com and one www.example.com.
 * Load https://webkit.org/blog-files/3d-transforms/poster-circle.html in the www.duckduckgo.com tab
 * Close the poster circle tab
 * UI process crashes

The example.com tab is just here to ensure we have two tabs and can therefore close the other tab without closing the entire browser.

It seems to require that the process has swapped from one using AC mode to another using AC mode. The reproducer still works if you swap duckduckgo.com with the poster circle page, but using example.com to replace either one of them does not work.

It actually only requires one website using AC mode. You can also reproduce by transitioning the tab from poster circle -> example.com -> back to poster circle -> close the poster circle tab.

[Michael Catanzaro]

Debugging:

$ jhbuild run epiphany
_WebKitWebViewBasePrivate::_WebKitWebViewBasePrivate(): creating this=0x173d160
WebKit::AcceleratedBackingStoreWayland::AcceleratedBackingStoreWayland(WebKit::WebPageProxy&): creating this=0x7fb8725eeb10
void WebKit::WaylandCompositor::registerWebPage(WebKit::WebPageProxy&): webPage=0x7fb8172e6000
void WebKit::WaylandCompositor::bindWebPage(WebKit::WebPageProxy&): webPage=0x7fb8172e6000

<<< example.com tab, unrelated
_WebKitWebViewBasePrivate::_WebKitWebViewBasePrivate(): creating this=0x121a8a0
WebKit::AcceleratedBackingStoreWayland::AcceleratedBackingStoreWayland(WebKit::WebPageProxy&): creating this=0x7fb8725f3390
void WebKit::WaylandCompositor::registerWebPage(WebKit::WebPageProxy&): webPage=0x7fb8172e2a00
void WebKit::WaylandCompositor::bindWebPage(WebKit::WebPageProxy&): webPage=0x7fb8172e2a00
>>>

WebKit::WaylandCompositor::Surface::Surface(): Creating this=0x7fb872590a80
void WebKit::WaylandCompositor::Surface::setWebPage(WebKit::WebPageProxy*): this=0x7fb872590a80 webPage=0x7fb8172e6000 m_webPage=(nil)
WebKit::WaylandCompositor::Surface::Surface(): Creating this=0x7fb87258fd20
void WebKit::WaylandCompositor::Surface::setWebPage(WebKit::WebPageProxy*): this=0x7fb87258fd20 webPage=0x7fb8172e6000 m_webPage=(nil)

So far so good. Now:

void WebKit::WaylandCompositor::unbindWebPage(WebKit::WebPageProxy&): webPage=0x7fb8172e6000
void WebKit::WaylandCompositor::Surface::setWebPage(WebKit::WebPageProxy*): this=0x7fb87258fd20 webPage=(nil) m_webPage=0x7fb8172e6000
virtual 

We unbound only one of the two Surfaces. Surface 0x7fb87258fd20 (the surface for the second page, poster circle) is OK but Surface 0x7fb872590a80 (the surface for DuckDuckGo) got missed.

WebKit::AcceleratedBackingStoreWayland::~AcceleratedBackingStoreWayland(): destroying this=0x7fb8725eeb10
void WebKit::WaylandCompositor::unregisterWebPage(WebKit::WebPageProxy&): webPage=0x7fb8172e6000
void WebKit::WaylandCompositor::Surface::setWebPage(WebKit::WebPageProxy*): this=0x7fb87258fd20 webPage=(nil) m_webPage=(nil)

Here, we again unregistered only one of the two Surfaces. The DuckDuckGo surface is left with a dangling pointer to m_webPage that is no longer valid.

_WebKitWebViewBasePrivate::~_WebKitWebViewBasePrivate(): destroying this=0x173d160
WebKit::WaylandCompositor::Surface::~Surface(): Destroying this=0x7fb87258fd20
void WebKit::WaylandCompositor::Surface::setWebPage(WebKit::WebPageProxy*): this=0x7fb87258fd20 webPage=(nil) m_webPage=(nil)
WebKit::WaylandCompositor::Surface::~Surface(): Destroying this=0x7fb872590a80
void WebKit::WaylandCompositor::Surface::setWebPage(WebKit::WebPageProxy*): this=0x7fb872590a80 webPage=(nil) m_webPage=0x7fb8172e6000
Segmentation fault (core dumped)

Now the Surfaces get deleted by the lambda passed to wl_resource_set_implementation() (inside: static const struct wl_compositor_interface compositorInterface). m_webPage is dangling, so Surface::setWebPages tries to use it, and we die.

So problem is that both WaylandCompositor::unbindWebPage and WaylandCompositor::unregisterWebPage failed to call surface->setWebPage(nullptr) for the second Surface. They both should have done that. Will continue to debug.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> So problem is that both WaylandCompositor::unbindWebPage and
> WaylandCompositor::unregisterWebPage failed to call
> surface->setWebPage(nullptr) for the second Surface. They both should have
> done that. Will continue to debug.

Oh shit, it's very simple. We are not using a loop there. We just take the first matching Surface from m_pageMap, assuming that Surface objects never share the same WebPageProxy. Since PSON, that is no longer correct.

[Michael Catanzaro]

Created attachment 395898 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Well this turned out to be more complicated than I expected.

I uploaded a WIP patch as proof of concept. It fixes the crash, but introduces a new regression: exit from AC mode no longer works, the non-accelerated page does not get rendered. To reproduce, using just one tab: load gnome.org (AC mode) and then example.com (exits AC mode). I've tried pretty hard to ensure that no accelerated surface is rendering at the time (at least, that we enter the eglDestroyImage/glDeleteTextures codepath inside WaylandCompositor::Surface::setWebPage) and I haven't made progress on this for a little while now, so I'm going to stop and just upload what I have and maybe come back to this tomorrow. The patch is peppered with debug statements to try to figure out what's going wrong with exiting AC mode.

[Michael Catanzaro]

Comment on attachment 395898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395898&action=review

> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:284
> +// FIXME: this check isn't working
> +    if (m_acceleratedCompositingModeEnabled)
> +        WaylandCompositor::singleton().bindWebPage(m_webPage);

BTW I should have rephrased this confusing debug comment before uploading. The check itself is working properly. It's just not accomplishing its goal, which is to ensure that exiting AC mode works. (Without this check, bindWebPage would display the accelerated surface, so it needs to be here.)

[Michael Catanzaro]

OK, sorry, I've failed to debug what's going wrong with the rendering, and I'm out of time for further debugging. My fallback plan is to disable PSON when building without WPE_RENDERER enabled, bug #210287. That should be an OK workaround for now.

[Carlos Garcia Campos]

Created attachment 396075 [details]
Patch

[Michael Catanzaro]

Comment on attachment 396075 [details]
Patch

Smart. And much nicer than my patch.

But problem is that the Surface previously bound to the WebPageProxy is still alive until the web process is closed, but now the Surface is no longer tracked anywhere by the WaylandCompositor class. Right? Unbinding it here removes it from m_pageMap in favor of this new Surface. Now what happens when we process swap back to the original Surface (e.g. using back/forward list)? Won't the call to bindWebPage fail? It has no way to find the correct Surface anymore. So I think your change is good, but not sufficient to fully fix WaylandCompositor. Do you agree?

I think a separate list of all Surfaces is needed. And identifying the proper Surface from that list in bindWebPage requires the surface to know the WebCore::PageIdentifier that it was originally associated with. Yes?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #13)
> Comment on attachment 396075 [details]
> Patch
> 
> Smart. And much nicer than my patch.
> 
> But problem is that the Surface previously bound to the WebPageProxy is
> still alive until the web process is closed,

This is not true. The Surface object will be deleted when wayland compositor destroys the wayland surface resource.

            wl_resource_set_implementation(surfaceResource, &surfaceInterface, new WaylandCompositor::Surface(),
                [](struct wl_resource* resource) {
                    auto* surface = static_cast<WaylandCompositor::Surface*>(wl_resource_get_user_data(resource));
                    delete surface;
                });


> but now the Surface is no
> longer tracked anywhere by the WaylandCompositor class. Right?

Not true either. The Surface is not tracked by the page map, that only tracks the relationship between page and surface, but not its life time, what we store in the map is indeed a weak pointer to the Surface object. The Surface lifetime is tracked by the Wayland compositor, when the wayland surface is destroyed, the Surface object is deleted.

> Unbinding it
> here removes it from m_pageMap in favor of this new Surface.

This is right.

> Now what
> happens when we process swap back to the original Surface (e.g. using
> back/forward list)?

The web process creates a new wayland surface (which destroys the previous one, and then the Surface object too) and bindSurfaceToWebPage message is sent for the new surface.

> Won't the call to bindWebPage fail?

No, it's called from bindSurfaceToWebPage.

> It has no way to
> find the correct Surface anymore.

The right surface has been added to the map in bindSurfaceToWebPage.

> So I think your change is good, but not
> sufficient to fully fix WaylandCompositor. Do you agree?

No.

> I think a separate list of all Surfaces is needed. And identifying the
> proper Surface from that list in bindWebPage requires the surface to know
> the WebCore::PageIdentifier that it was originally associated with. Yes?

No.

[Michael Catanzaro]

> Not true either. The Surface is not tracked by the page map, that only
> tracks the relationship between page and surface, but not its life time,
> what we store in the map is indeed a weak pointer to the Surface object. The
> Surface lifetime is tracked by the Wayland compositor, when the wayland
> surface is destroyed, the Surface object is deleted.

I know, because I made it use the WeakPtr. :)
 
> > Now what
> > happens when we process swap back to the original Surface (e.g. using
> > back/forward list)?
> 
> The web process creates a new wayland surface (which destroys the previous
> one, and then the Surface object too) and bindSurfaceToWebPage message is
> sent for the new surface.

Excellent. I just assumed it was not doing this. If I had added more prints, I would have seen that what I was trying to do wasn't needed at all.

[Guilaume Ayoub]

No more crashes for me with the patch, thanks!

[Carlos Garcia Campos]

Committed r259942: <https://trac.webkit.org/changeset/259942>