Merge lp:~vanvugt/mir/fix-1401488 into lp:mir

[Daniel van Vugt]

Avoid heap corruption and general instability caused by GSource destruction from another thread while the GMainLoop is iterating.
(LP: #1401488)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2191
http://jenkins.qa.ubuntu.com/job/mir-ci/2548/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/695
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/695
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/669
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/545
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/545/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/669
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/669/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3802
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16781

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2548/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

8 +#ifndef GLIB_HAS_FIXED_LP_1401488

OK, we don't actually define the macro, but shouldn't it start with "MIR_"?

[Daniel van Vugt (vanvugt)]

GLIB_HAS_FIXED_LP_1401488 is more a matter of documentation than a functional macro. Instead of ever defining it we would probably just delete the workaround introduced here once (if) glib gets fixed. Also, it's local to the .cpp file so doesn't need any MIR_ prefix for namespace protection.

[Alan Griffiths (alan-griffiths)]

The "namespace protection" would be against some other project (glib) defining GLIB_XXX. Although the chances of XXX==HAS_FIXED_LP_1401488 is clearly remote.

[Daniel van Vugt (vanvugt)]

The chances are incredibly remote, but also useful... If GLib did define that macro then I would take it as a strong indication that it means exactly what it sounds like. So that would be a useful definition and not a conflicting definition.

[Alan Griffiths (alan-griffiths)]

The change looks plausible but I don't have a reliable way to reproduce the problem it purports to fix. (I also don't like the possibility of accidentally compiling a variant version of the code, but won't block in the risk of that happening.)

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

I tried the test case in the bug report but my machine got frozen on the server VT either way. Are you sure this is fixing it?

It does nevertheless look like the right thing to do, so approving.

review: Approve

[Kevin DuBois (kdub)]

given how macro-free we've been able to keep the mir code, I'd rather have the comment explaining that we cannot use g_source_unref(gsource) like we'd like to, and only have the longer fix there.

I guess if this is a critical bug though, will abstain.

review: Abstain

[Robert Carr (robertcarr)]

Ok

review: Approve

[Daniel van Vugt (vanvugt)]

Yes I did make sure I had reliable test cases for reproducing the issue before attempting this fix. Actually one is missing from the bug. I'll add it...

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/server/glib_main_loop_sources.cpp'
--- src/server/glib_main_loop_sources.cpp	2014-11-14 19:34:33 +0000
+++ src/server/glib_main_loop_sources.cpp	2014-12-30 09:45:05 +0000
@@ -51,6 +51,21 @@
     GSource* gsource;
 };
 
+#ifndef GLIB_HAS_FIXED_LP_1401488
+gboolean idle_callback(gpointer)
+{
+    return G_SOURCE_REMOVE; // Remove the idle source. Only needed once.
+}
+
+void destroy_idler(gpointer user_data)
+{
+    // idle_callback is never totally guaranteed to be called. However this
+    // function is, so we do the unref here...
+    auto gsource = static_cast<GSource*>(user_data);
+    g_source_unref(gsource);
+}
+#endif
+
 }
 
 /*****************
@@ -91,7 +106,25 @@
     {
         pre_destruction_hook(gsource);
         g_source_destroy(gsource);
+
+#ifdef GLIB_HAS_FIXED_LP_1401488
         g_source_unref(gsource);
+#else
+        /*
+         * https://bugs.launchpad.net/mir/+bug/1401488
+         * We would like to g_source_unref(gsource); here but it's unsafe
+         * to do so. The reason being we could be in some arbitrary thread,
+         * and so the main loop might be mid-iteration of the same source
+         * making callbacks. And glib lacks protection to prevent sources
+         * getting fully free()'d mid-callback (TODO: fix glib?). So we defer
+         * the final unref of the source to a point in the loop when it's safe:
+         */
+        auto main_context = g_source_get_context(gsource);
+        auto idler = g_idle_source_new();
+        g_source_set_callback(idler, idle_callback, gsource, destroy_idler);
+        g_source_attach(idler, main_context);
+        g_source_unref(idler);
+#endif
     }
 }