Bug 284807 – [WPE][GTK] WebKit::allDataStores release assertion at shutdown

• Status: NEW
• Component: WPE WebKit

[Guillaume Desmottes]

To reproduce:
- build this gstreamer branch https://gitlab.freedesktop.org/philn/gstreamer/-/tree/wip/wpe-ng?ref_type=heads
- I add to change this to it picks the right device

```patch
-- a/subprojects/gst-plugins-bad/ext/wpe2/gstwpedisplay.cpp
+++ b/subprojects/gst-plugins-bad/ext/wpe2/gstwpedisplay.cpp
@@ -107,7 +107,7 @@ wpe_display_gstreamer_connect (WPEDisplay * display, GError ** error)
     // FIXME: This hack is needed when using gtkglsink. glimagesink somehow works as expected.
     GST_WARNING
         ("'EGL_EXT_device_drm' not available, hardcoding render node and hoping for the best");
-    self->drm_render_node = g_strdup ("/dev/dri/renderD128");
+    self->drm_render_node = g_strdup ("/dev/dri/card1");
     return TRUE;
   }
```

- `WEBKIT_DISABLE_SANDBOX_THIS_IS_DANGEROUS=1 G_DEBUG=fatal-warnings GST_DEBUG=2 gst-launch-1.0 wpevideosrc2 location=https://gstreamer.freedesktop.org/ ! queue ! gtk4paintablesink`

```
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1  0x00007ffff7b0ed03 in __pthread_kill_internal (threadid=<optimized out>, signo=6) at pthread_kill.c:78
#2  0x00007ffff7ab5d1e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007ffff7a9d942 in __GI_abort () at abort.c:79
#4  0x00007fffe37a556f in WTFCrashWithInfo () at WTF/Headers/wtf/Assertions.h:864
#5  0x00007fffe3dd14ea in WebKit::allDataStores () at /usr/src/debug/wpewebkit-2.46.4-1.fc41.x86_64/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:118
#6  WebKit::WebsiteDataStore::~WebsiteDataStore (this=0x7fff0a050400) at /usr/src/debug/wpewebkit-2.46.4-1.fc41.x86_64/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:194
#7  0x00007fffe3dd1692 in WebKit::WebsiteDataStore::~WebsiteDataStore (this=0x70b8) at /usr/src/debug/wpewebkit-2.46.4-1.fc41.x86_64/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:183
#8  0x00007fffe3d549c6 in WTF::ThreadSafeRefCounted<API::Object, (WTF::DestructionThread)0>::deref() const::{lambda()#1}::operator()() const (this=<optimized out>) at WTF/Headers/wtf/ThreadSafeRefCounted.h:144
#9  WTF::ThreadSafeRefCounted<API::Object, (WTF::DestructionThread)0>::deref (this=0x70c0) at WTF/Headers/wtf/ThreadSafeRefCounted.h:156
#10 WTF::DefaultRefDerefTraits<WebKit::WebsiteDataStore>::derefIfNotNull (ptr=0x70b8) at WTF/Headers/wtf/Ref.h:62
#11 WTF::RefPtr<WebKit::WebsiteDataStore, WTF::RawPtrTraits<WebKit::WebsiteDataStore>, WTF::DefaultRefDerefTraits<WebKit::WebsiteDataStore> >::~RefPtr (this=0x7ffd3800ce00) at WTF/Headers/wtf/RefPtr.h:60
#12 _WebKitWebsiteDataManagerPrivate::~_WebKitWebsiteDataManagerPrivate (this=0x7ffd3800ce00) at /usr/src/debug/wpewebkit-2.46.4-1.fc41.x86_64/Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:93
#13 webkit_website_data_manager_finalize (object=0x7ffd3800ce30 [WebKitWebsiteDataManager]) at /usr/src/debug/wpewebkit-2.46.4-1.fc41.x86_64/Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:123
#14 0x00007ffff7ca6d2a in g_object_unref (_object=0x7ffd3800ce30) at ../gobject/gobject.c:4486
#15 0x00007fffe3d1535d in WTF::derefGPtr<_WebKitWebsiteDataManager> (ptr=0x70b8) at WTF/Headers/wtf/glib/GRefPtr.h:281
#16 WTF::GRefPtr<_WebKitWebsiteDataManager>::~GRefPtr (this=0x7ffd3800c000) at WTF/Headers/wtf/glib/GRefPtr.h:91
#17 _WebKitNetworkSessionPrivate::~_WebKitNetworkSessionPrivate (this=0x7ffd3800c000) at /usr/src/debug/wpewebkit-2.46.4-1.fc41.x86_64/Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.cpp:78
#18 webkit_network_session_finalize (object=0x7ffd3800c080 [WebKitNetworkSession]) at /usr/src/debug/wpewebkit-2.46.4-1.fc41.x86_64/Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.cpp:98
#19 0x00007ffff7ca6d2a in g_object_unref (_object=0x7ffd3800c080) at ../gobject/gobject.c:4486
#20 0x00007ffff7ab8441 in __run_exit_handlers (status=1, listp=0x7ffff7c86680 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at exit.c:108
#21 0x00007ffff7ab850e in __GI_exit (status=<optimized out>) at exit.c:138
#22 0x00007ffff7a9f24f in __libc_start_call_main (main=main@entry=0x4018e0 <main>, argc=argc@entry=7, argv=argv@entry=0x7fffffff66d8) at ../sysdeps/nptl/libc_start_call_main.h:74
#23 0x00007ffff7a9f30b in __libc_start_main_impl (main=0x4018e0 <main>, argc=7, argv=0x7fffffff66d8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffff66c8) at ../csu/libc-start.c:360
#24 0x0000000000401915 in _start ()
```

Running in a F41 toolbox in Silverblue 41 with a nvidia RTX 4080.

[Guillaume Desmottes]

It's actually easy to reproduce with 2.48.1 : gst-launch-1.0 wpevideosrc2 location=https://gstreamer.freedesktop.org/ ! fakesink

The hit Ctrl-C once the pipeline is playing.

I think the problem here is the atexit callback destroying WebsiteDataStore is called from the application main thread which is different from the WebKit main thread which is the thread that initialized WebKit, see https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/generic/MainThreadGeneric.cpp#L44

[Guillaume Desmottes]

I'm still not sure which atexit handler is triggering this. 

My best guess so far is the static reference on WebKitNetworkSession is dropped when exiting and GRefPtr then calls g_object_unref. Does that make sense?
https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.cpp#L229

If that's actually the case this API cannot work if WebKit has been initialized from a thread different than the application one.

[Michael Catanzaro]

So this is bug #243401, somehow come back to haunt us....

[Michael Catanzaro]

Well, not really. The problem in bug #243401 is the isMainThread() function was itself broken. Here we just have stuff happening on the wrong thread.

> My best guess so far is the static reference on WebKitNetworkSession is dropped when exiting and GRefPtr then calls g_object_unref. Does that make sense?
https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.cpp#L229

Yes. To avoid destroying the WebKitNetworkSession in an exit handler, we would need to use NeverDestroyed there. Exit handlers are scary and frequent sources of crashes.

Normally using NeverDestroyed is usually safe, and it's mandatory in cross-platform code. But we often omit it in Linux-specific code. And it's not always safe to leak a global object. In the past, we've had cookies not saved to disk when the actual SoupSession in the network process was leaked, for example. That's not comparable to this situation, though, because here we are in the UI process, but it wouldn't be too surprising if leaking the WebKitNetworkSession were to cause some sort of unexpected bad stuff to happen.

> If that's actually the case this API cannot work if WebKit has been initialized from a thread different than the application one.

Right, but this is supposed to be safe. I'm not sure what to do. I guess we have to either (a) use NeverDestroyed, or (b) stop using a static variable here.

But also, we follow this same pattern in many other places, and they're likely all broken. E.g. PlatformDisplay::sharedDisplay will have the same problem. Global data like this is only supposed to be accessed from the thread that initializes WebKit, but exit handlers will always run on the OS main thread, not the WebKit main thread. Looks like we have a flaw in the thread-safety model.

The only good news is that I've checked GLib, GTK, and GStreamer and I don't think they have the same problem, because they don't use atexit except for tests or debug or demos. So this problem is really specific to WebKit static variables.

I guess we need to make NeverDestroyed mandatory to use. That would solve a few other bugs: bug #116672, bug #260856, and bug #278685. The discussion in bug #116672 is particularly interesting. Looks like I used to know that static objects are unsafe, but forgot.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> In the past, we've had cookies not
> saved to disk when the actual SoupSession in the network process was leaked,
> for example.

That was bug #166029. https://bugs.webkit.org/show_bug.cgi?id=166029#c3 is interesting.

[Michael Catanzaro]

We have a Tools/Scripts/check-for-exit-time-destructors, but it is broken and nothing actually calls it. :(

Clang's -Wexit-time-destructors warning might help.

[Guillaume Desmottes]

> Right, but this is supposed to be safe. I'm not sure what to do. I guess we have to either (a) use NeverDestroyed, or (b) stop using a static variable here.

Another option could be to use a weak pointer instead, so the object is destroyed when no longer used. But then the object could be re-created/destroyed multiple times during the application lifetime; not sure if that actually would be a problem or not.

[Michael Catanzaro]

That would probably be a problem.