Bug 30876 – [GTK] Make timeout callbacks hold the GDK lock

• Component: WebKitGTK
• Status: RESOLVED INVALID

[Xan Lopez]

Timeouts, idles, and other similar things are not executed holding the GDK lock (since they are implemented in glib, which does not have access to that). In multithreaded GTK+ applications only one thread can use GTK+ at any given moment, so if you use timeouts or idles you need to be careful and hold the GDK lock if you are going to call GTK+ APIs. WebKitGTK+ is multithreaded, so this applies to us.

I have identified three timeouts in our codebase, all of them not holding the GDK lock. Two are in the DRT app, and the other one is the timeout used to schedule functions to be called in the main thread. I guess the DRT changes are uncontroversial, but kov fears that making the other timeout hold the lock could be a performance regression in some cases. It seems to me, though, that there are some obvious (?) situations where JS will end up calling GTK+, like window.open, so I if I'm not wrong we have to either a) do this b) identify all the places that can be called from JS and protect them with locks. b) is similar to our current situation with calling webkit_init on demand, which seems to be a healthy source of bugs.

I'm CCing ggaren and posting my proposed patch to kick off some discussion.

[Xan Lopez]

Created attachment 42051 [details]
gdklock.diff

[Holger Freyther]

I have a slightly modified concern. In the long run we want to build a separate JSC so and this might not require a GUI and linking to GDK might seem excessive. From what I see the modified function in JSC will not be executed from within JSC though..

Did you check WebCore/platform/gtk/SharedTimerGtk.cpp? is that safe?

[Xan Lopez]

(In reply to comment #2)
> I have a slightly modified concern. In the long run we want to build a separate
> JSC so and this might not require a GUI and linking to GDK might seem
> excessive. From what I see the modified function in JSC will not be executed
> from within JSC though..

That's right, and that sucks. So maybe we'll have to go the "find all the places that need protection and protect them...".

> Did you check WebCore/platform/gtk/SharedTimerGtk.cpp? is that safe?

No, it's exactly the same. g_timeout_add/g_idle_add don't hold the GDK lock in their callbacks, so if some other thread kicks in when that is being executed, and the callback ends up calling GTK+, you'll have two threads using GTK+ at the same time.

[Gustavo Noronha (kov)]

<kov> if any gdk function is called, we need to hold the lock before doing that, otherwise we'll fail on multithreaded programs
<ggaren> kov: scheduleDispatchFunctionsOnMainThread can be called by anyone, so, yes.
 kov: processWork will call into webkit, so, yes.
<ggaren> kov: waitToDumpWatchdogFired will probably not call into any gui code

So, looks like we do need that; we will now have to figure out how to make this without linking jsc to gdk, if possible =D

[Danilo Šegan]

I wonder if this could be behind https://bugs.webkit.org/show_bug.cgi?id=58789 as well?

[Martin Robinson]

(In reply to comment #5)
> I wonder if this could be behind https://bugs.webkit.org/show_bug.cgi?id=58789 as well?

If that's happening on the main thread and doesn't involve GDK/GTK, probably not, but perhaps.

[Martin Robinson]

This probably isn't an issue with WebKit2 any longer, but feel free to reopen if I'm wrong.