Bug 161481 – [GTK] Use GTestDBus instead of dbus-launch in WebKitTestBus.cpp

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alberto Garcia]

This is not a priority, but I'm leaving it here so it can be tracker.

Debian is moving towards making dbus-x11 optional.

WebKitGTK+ uses dbus-launch in one of its tests. Using GTestDBus instead (as stated in the FIXME comment in the very source file) should solve this problem.

More details here:

https://lists.debian.org/debian-devel/2016/08/msg00554.html

[Carlos Garcia Campos]

Created attachment 293416 [details]
Patch

[Michael Catanzaro]

Didn't we have a similar patch recently? From Gustavo or Emanuele? What happened to that?

[Carlos Garcia Campos]

I don't know I haven't seen anything in my bug mail, and this is the first patch in this bug.

[Carlos Garcia Campos]

Ah, I guess you mean bug #161135. That doesn't change the use of dbus-launch, both patches are compatible, it's not the same thing

[Michael Catanzaro]

Comment on attachment 293416 [details]
Patch

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

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebKitTestBus.cpp:38
> +    g_setenv("DISPLAY", display.data(), FALSE);

So the problem here is it's too late to be doing setenv; we've already initialized threads. Some other thread could call getenv at the same time -- there are getenv calls in many many places -- and crash. It's not a theoretical issue, I've seen multiple cases of this crashing gnome-session.

[Michael Catanzaro]

Although I do see the existing code was doing this already, I would prefer finding a way to fix it.

[Carlos Garcia Campos]

Comment on attachment 293416 [details]
Patch

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

>> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebKitTestBus.cpp:38
>> +    g_setenv("DISPLAY", display.data(), FALSE);
> 
> So the problem here is it's too late to be doing setenv; we've already initialized threads. Some other thread could call getenv at the same time -- there are getenv calls in many many places -- and crash. It's not a theoretical issue, I've seen multiple cases of this crashing gnome-session.

It's not that I want to se the display here, DISPLAY has already been set, but g_test_dbus_up() unsets it unconditionally, so I'm just restoring it because otherwise nothing works (the web process needs DISPLAY to be set to work). This has nothing to do with gnome-session, this only affects the tests.

[Michael Catanzaro]

Comment on attachment 293416 [details]
Patch

The problem is that if you do that, the test will randomly crash; we don't need more flaky tests. In fact, since g_test_dbus_up() calls setenv() itself, it suffers from the same problem. We need to be careful to only call such functions (a) at the very top of main(), or (b) when we're really really sure that it's before the first secondary thread has been created, otherwise the test will be flaky. And it's impossible for WebKitTestBus to guarantee where it's used from, so it really needs to not be setting environment variables.

Is it even possible to create a GObject without starting secondary threads?

[Gustavo Noronha (kov)]

While the patch from bug 161135 applies along with this one, it does disable this one which I think is undesirable. The result of having both applied would be dbus-run-session be used every time it is available. This solution is a better one, so I'd recommend just going with it, if we can fix the threading concerns.

[Carlos Garcia Campos]

(In reply to comment #8)
> Comment on attachment 293416 [details]
> Patch
> 
> The problem is that if you do that, the test will randomly crash; we don't
> need more flaky tests.

Why? Could you show a bt of one of such crashes? Because our current code is calling setenv in the same place, this patch is only replacing the code that GTestDBus with GTestDBus.

> In fact, since g_test_dbus_up() calls setenv()
> itself, it suffers from the same problem.

Exactly, it's not a problem of this patch, but an existing problem (if any) and I have never seen random crashes in the tests using WebKitTestBus.

> We need to be careful to only call
> such functions (a) at the very top of main(),

Which is exactly what happens. This is not public API, this is an internal helper class designed to be used by tests that should do this at the very beginning of beforeAll():

bus = new WebKitTestBus();
if (!bus->run())
    return;

And beforeAll() is called by main() right after setting up the environment and registering gresources. There isn't any other code before that, nor any other secondary thread created.

> or (b) when we're really
> really sure that it's before the first secondary thread has been created,
> otherwise the test will be flaky. And it's impossible for WebKitTestBus to
> guarantee where it's used from, so it really needs to not be setting
> environment variables.

We are already ensuring both a) and b), and we don't need WebKitTestBus to ensure anything, we just need to use it as it's designed to be used, at the very beginning of beforeAll(). Another option would be to do it unconditionally in main() for all tests, but that could slow down the tests that don't need dbus at all.

> Is it even possible to create a GObject without starting secondary threads?

There isn't any thread nor GObject created in this case when WebKitTestBus::run is called.

[Carlos Garcia Campos]

(In reply to comment #9)
> While the patch from bug 161135 applies along with this one, it does disable
> this one which I think is undesirable. The result of having both applied
> would be dbus-run-session be used every time it is available. This solution
> is a better one, so I'd recommend just going with it, if we can fix the
> threading concerns.

I'm not sure this patch solves the same problem. The bus started by WebKitTestBus::run() will be leaked if the test crashes, because ~WebKitTestBus() won't be called in that case. So, we can leave this code as fallback when dbus-run-session is not present, or when tests are run manually (without using run-gtk-tests), something that I do a lot. When dbus-run-session is present we simply set m_address, I don't think the patches are mutually exclusive.

[Michael Catanzaro]

(In reply to comment #10)
> Why? Could you show a bt of one of such crashes? Because our current code is
> calling setenv in the same place, this patch is only replacing the code that
> GTestDBus with GTestDBus.

Here's one example of such a crash, with a backtrace:

https://bugzilla.gnome.org/show_bug.cgi?id=754951

We had a second case in some important GNOME component recently, but I honestly don't remember where and can't find it now. And we have third one in Endless's private bug tracker right now.

> > We need to be careful to only call
> > such functions (a) at the very top of main(),
> 
> Which is exactly what happens. This is not public API, this is an internal
> helper class designed to be used by tests that should do this at the very
> beginning of beforeAll():
> 
> bus = new WebKitTestBus();
> if (!bus->run())
>     return;
> 
> And beforeAll() is called by main() right after setting up the environment
> and registering gresources. There isn't any other code before that, nor any
> other secondary thread created.

OK, then it should be OK, but it is so easy to misuse that I would prefer that you add an assertion to guarantee this. Unfortunately the best example I found requires reading /proc:

http://stackoverflow.com/a/13993822/1120203

but it could be done inside #ifndef NDEBUG. Then if that check passes, we can be confident that GLib isn't creating any secondary thread somewhere before the setenv.

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #10)
> > Why? Could you show a bt of one of such crashes? Because our current code is
> > calling setenv in the same place, this patch is only replacing the code that
> > GTestDBus with GTestDBus.
> 
> Here's one example of such a crash, with a backtrace:
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=754951
> 
> We had a second case in some important GNOME component recently, but I
> honestly don't remember where and can't find it now. And we have third one
> in Endless's private bug tracker right now.

I don't care about other applications crashing, this bug is about our unit tests, we have done this forever and I've never seen a crash, so I don't understand why you say that this patch is going to be the source of crashes and flaky tests.

> > > We need to be careful to only call
> > > such functions (a) at the very top of main(),
> > 
> > Which is exactly what happens. This is not public API, this is an internal
> > helper class designed to be used by tests that should do this at the very
> > beginning of beforeAll():
> > 
> > bus = new WebKitTestBus();
> > if (!bus->run())
> >     return;
> > 
> > And beforeAll() is called by main() right after setting up the environment
> > and registering gresources. There isn't any other code before that, nor any
> > other secondary thread created.
> 
> OK, then it should be OK, but it is so easy to misuse that I would prefer
> that you add an assertion to guarantee this. Unfortunately the best example
> I found requires reading /proc:
> 
> http://stackoverflow.com/a/13993822/1120203
> 
> but it could be done inside #ifndef NDEBUG. Then if that check passes, we
> can be confident that GLib isn't creating any secondary thread somewhere
> before the setenv.

This is private API for unit tests, I don't think we need to complicate things to fix a problem that doesn't exist.

[Gustavo Noronha (kov)]

(In reply to comment #11)
> (In reply to comment #9)
> > While the patch from bug 161135 applies along with this one, it does disable
> > this one which I think is undesirable. The result of having both applied
> > would be dbus-run-session be used every time it is available. This solution
> > is a better one, so I'd recommend just going with it, if we can fix the
> > threading concerns.
> 
> I'm not sure this patch solves the same problem. The bus started by
> WebKitTestBus::run() will be leaked if the test crashes, because
> ~WebKitTestBus() won't be called in that case. So, we can leave this code as
> fallback when dbus-run-session is not present, or when tests are run
> manually (without using run-gtk-tests), something that I do a lot. When
> dbus-run-session is present we simply set m_address, I don't think the
> patches are mutually exclusive.

I thought GTestDBus handled cleaning up the dbus daemon process even on crashes? But if you think it's useful to have both and ok to have dbus-run-session used when present I'll go ahead and land it =)

[Michael Catanzaro]

Comment on attachment 293416 [details]
Patch

I'm going to give you r+ since you've convinced me that the current code is safe, but I really do think this is fragile and that it would be better to add an assertion, even if the assertion is not straightforward to write.

[Michael Catanzaro]

(to catch future incorrect use of this class, and to verify that we're right in thinking there is only one thread when this code is executed)

[Carlos Garcia Campos]

(In reply to comment #14)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > While the patch from bug 161135 applies along with this one, it does disable
> > > this one which I think is undesirable. The result of having both applied
> > > would be dbus-run-session be used every time it is available. This solution
> > > is a better one, so I'd recommend just going with it, if we can fix the
> > > threading concerns.
> > 
> > I'm not sure this patch solves the same problem. The bus started by
> > WebKitTestBus::run() will be leaked if the test crashes, because
> > ~WebKitTestBus() won't be called in that case. So, we can leave this code as
> > fallback when dbus-run-session is not present, or when tests are run
> > manually (without using run-gtk-tests), something that I do a lot. When
> > dbus-run-session is present we simply set m_address, I don't think the
> > patches are mutually exclusive.
> 
> I thought GTestDBus handled cleaning up the dbus daemon process even on
> crashes? But if you think it's useful to have both and ok to have
> dbus-run-session used when present I'll go ahead and land it =)

You are right, it uses a global watcher that forks a child that watches the parent and kills spawned processes in that case. So, I agree this solution is better, because it also works when tests are run directly without using run-gtk-tests

[Carlos Garcia Campos]

Committed r208284: <http://trac.webkit.org/changeset/208284>