Created attachment 308342 [details] [review]
gapplication: Acquire the main context before running

Otherwise, we'll acquire it on every loop iteration, which can leave us
vulnerable to racing another thread for the acquisition of the main
context.

This can break methods like g_main_context_invoke, which try to acquire
a context to figure out if it can invoke the method synchronously or
need to defer to an idle. In these cases, it isn't guaranteed that the
invocation function will be invoked in the default main context,
e.g. the one that GApplication is holding.

This also matches what GMainLoop is doing.

Shouldn't you acquire it earlier, before the command-line handling callbacks to be safe?

Sure, we should probably do that too.

What would that be 'safer' against ?

Because g_application_register() will happen earlier in g_application_run() and that will do all kind of gdbus stuff with threads. Also user could be doing thread stuff from "handle-local-options" callback.

IMO it would make sense for g_application_run() to acquire the NULL context before calling back any user code, I always assumed it did, tbh.

Review of attachment 308342 [details] [review]:

I think this patch should come with a documentation update that clearly states what assumptions one can make about main context ownership wrt to g_application_run

Created attachment 308821 [details] [review]
gapplication: Acquire the main context before running

Otherwise, we'll acquire it on every loop iteration, which can leave us
vulnerable to racing another thread for the acquisition of the main
context.

This can break methods like g_main_context_invoke, which try to acquire
a context to figure out if it can invoke the method synchronously or
need to defer to an idle. In these cases, it isn't guaranteed that the
invocation function will be invoked in the default main context,
e.g. the one that GApplication is holding.

This also matches what GMainLoop is doing.

I agree that this seems nice in practice, but first a question: why were you being harmed by the behaviour of g_main_context_invoke()?

We were doing work from a separate thread and using g_main_context_invoke to invoke a callback on the main application thread. This callback is a GJS callback, so it absolutely needed to be on the same thread as the application, otherwise, GJS started printing wacky errors and eventually crashed.

Oh, while reading again g_application_run() I noticed that it pass NULL to g_main_context_iterate() for every iteration, which cause calling g_main_context_default() each time.

That function starts with a comment:
/* Slow, but safe */

So maybe we want to call it only once in g_application_run(). That match even more g_main_loop_run() because g_main_loop_new(NULL) will call call g_main_context_default() only once as well.

Ping on this again?

FWIW we've been shipping with this patch for months on Endless and we haven't seen any negative consequence.

Review of attachment 308821 [details] [review]:

ok, its been long enough. I'll make the executive decision.

Attachment 308821 [details] pushed as a379a0a - gapplication: Acquire the main context before running

Created attachment 317519 [details] [review]
GApplication: Avoid getting the default context repeatidly

This avoids getting a global lock on every main loop iteration.

Review of attachment 317519 [details] [review]:

Unrelated to this bug, opened bug #759554 for it.