[GTK4] Ensure that clipboard read operations work synchronously

[csaavedra]

ac10a38

[GTK4] Ensure that clipboard read operations work synchronously
https://bugs.webkit.org/show_bug.cgi?id=265088

Reviewed by NOBODY (OOPS!).

Often clipboard operations are triggered as a result of a
button press event, which is handled with synchronous
messages between web and UI process. However, GTK4 clipboard
reads are asynchronous, and this can easily cause deadlocks
when the UI process is both waiting for the press event to be
handled by the web process before returning to the main loop
and the GTK4 clipboard is waiting for the GAsyncReadyCallback
to read from its stream at the same time, before allowing
WebKit's pasteboard to finish and let the press event to finish
being handled.

This explains why many of the GTK4 pasteboard and clipboard
tests are currently timing out -- the UI process is in
a deadlock state.

This deadlock can be prevented by using a GMainLoop to make
sure that the GTK Clipboard will call the GAsyncReadyCallback
for the clipboard operation in time, without deadlocking.

* Source/WebKit/UIProcess/gtk/ClipboardGtk4.cpp:
(WebKit::ReadTextAsyncData::ReadTextAsyncData):
(WebKit::Clipboard::readText):
(WebKit::ReadBufferAsyncData::ReadBufferAsyncData):
(WebKit::Clipboard::readBuffer):

ac10a38

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on current version of this PR (hash ac10a38)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[mcatanzaro]

Hm, nested main loops are inherently very risky and error-prone. For example:

do_thing_async(async_cb);
do_another_thing_that_triggers_clipboard_read(); // <-- now async_cb may be surprisingly called here!
do_thing_required_for_async_cb_to_not_break(); // looks safe, but isn't

Programmers don't generally expect callbacks to be invoked before the next iteration of the current main loop. This is why gtk_dialog_run() was removed from GTK. Here's a recent example where this same pattern used in Epiphany caused a real bug.

This is probably fixable though. If you create your own temporary GMainContext here and push it thread default, you can run a main loop that handles only the clipboard operation and won't execute other code. Then after the call to g_main_loop_run(), pop thread default and unref the GMainContext. The question is which main context the gdk_clipboard_read_text_async() callback will be invoked on. It should hopefully be the main context that was thread-default at the time the operation started, in which case this strategy will work fine. If not, we have a problem and might need to change GdkClipboard.

[csaavedra]

This is probably fixable though. If you create your own temporary GMainContext here and push it thread default, you can run a main loop that handles only the clipboard operation and won't execute other code. Then after the call to g_main_loop_run(), pop thread default and unref the GMainContext. The question is which main context the gdk_clipboard_read_text_async() callback will be invoked on. It should hopefully be the main context that was thread-default at the time the operation started, in which case this strategy will work fine. If not, we have a problem and might need to change GdkClipboard.

I tried this and the outcome is that most tests deadlock. I didn't have time to debug that yet though (update: silly me, I forgot to use this new main context for the temporary main loop 🤦).

Maybe I should explain a bit more extensively what the problem is in the bug report, but in a nutshell the original problem happens because

GdkContentProviderBytes::write_mime_type_async() is calling g_output_stream_write_all_async() (https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gdk/gdkcontentproviderimpl.c#L537), which does all the writing work in a separate thread (https://gitlab.gnome.org/GNOME/glib/-/blob/0af274faa43a59dd2b8e8a2cb75cb39a84d24641/gio/goutputstream.c#L1225), in that thread g_output_stream_write_all() blocks while writing. This is all OK.

The problem is on the other side of the operation (reading from that stream). This would happen in the clipboard async callback. But the callback is not called quite yet. g_task_return() decides not to call it right away but to just wait (https://gitlab.gnome.org/GNOME/glib/-/blob/0af274faa43a59dd2b8e8a2cb75cb39a84d24641/gio/gtask.c#L1265). But WebKit's UI process at this point is not iterating the main loop, it goes back to waiting for the synchronous mouse press event message to be replied from the web process, because this originated the clipboard operation to begin with and the web process has not yet replied (as it's, in turn, waiting for the result of the clipboard read message it sent to the UI process).

[mcatanzaro]

I tried this and the outcome is that most tests deadlock. I didn't have time to debug that yet though (update: silly me, I forgot to use this new main context for the temporary main loop 🤦).

So, are we good then?

[mcatanzaro]

BTW there is some documentation here that explains the danger I'm warning about, in the section "Using GMainContext in a Library":

Never iterate a context created outside the library, including the global-default or thread-default contexts. Otherwise, sources created in the application may be dispatched when the application is not expecting it, causing re-entrancy problems for the application code.

[csaavedra]

I tried this and the outcome is that most tests deadlock. I didn't have time to debug that yet though (update: silly me, I forgot to use this new main context for the temporary main loop 🤦).

So, are we good then?

Not quite, other tests block at a later stage (during splicing of the returned stream into a memory buffer stream).