[epiphany] history-service: Fix multiple initialization race conditions

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 2cb839ce0e385335f6253ffeca5e4c57073b3daa
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Sun Feb 19 09:45:32 2017 -0600

    history-service: Fix multiple initialization race conditions
    
    This started out as a project to fix the read-only service test I just
    added. Initializing two history service objects in a row was racy,
    because I needed the first history service to be initialized before
    creating the second one, but there was no way to ensure that. This was
    only an issue for this one test, though; real Epiphany browser mode of
    course only creates one history service, so I assumed it was not a big
    problem.
    
    Fix this first issue using a condition variable to ensure the GObject
    initialization doesn't complete until after the history service has
    actually created the SQLite database.
    
    In doing this, I discovered a second bug. The use of the condition
    variable altered the timing slightly, and caused the history filename
    property to not be set in time when entering the history service thread.
    In fact, it's kind of amazing that the history service ever worked at
    all, because there is absolutely nothing here to guarantee that the
    filename and read-only properties have been initialized prior to
    starting the history service thread. So the database filename could be
    NULL when opening the database, which is a great way to lose all your
    history. Also, it could also be in read-only mode here even if it is
    supposed to be read/write mode, which is going to cause failures after
    today's commits. Fix this by adding a constructed function and starting
    the history thread from there, instead of doing it in init. This means
    that the history thread will not be started until after properties have
    been set. Note that, while I could not reproduce this bug on my machine
    until after adding the condition variable to fix the first bug, that was
    just due to timing and luck; it was already broken before.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=778649

 lib/history/ephy-history-service-private.h |    2 +
 lib/history/ephy-history-service.c         |   47 ++++++++++++++++++++++-----
 tests/ephy-history-test.c                  |    5 ---
 3 files changed, 40 insertions(+), 14 deletions(-)
---
diff --git a/lib/history/ephy-history-service-private.h b/lib/history/ephy-history-service-private.h
index f581e94..08adefe 100644
--- a/lib/history/ephy-history-service-private.h
+++ b/lib/history/ephy-history-service-private.h
@@ -29,6 +29,8 @@ struct _EphyHistoryService {
   char *history_filename;
   EphySQLiteConnection *history_database;
   GMutex history_thread_mutex;
+  gboolean history_thread_initialized;
+  GCond history_thread_initialized_condition;
   GThread *history_thread;
   GAsyncQueue *queue;
   gboolean scheduled_to_quit;
diff --git a/lib/history/ephy-history-service.c b/lib/history/ephy-history-service.c
index 8770190..e67bf96 100644
--- a/lib/history/ephy-history-service.c
+++ b/lib/history/ephy-history-service.c
@@ -155,6 +155,36 @@ ephy_history_service_dispose (GObject *object)
   G_OBJECT_CLASS (ephy_history_service_parent_class)->dispose (object);
 }
 
+static void
+ephy_history_service_constructed (GObject *object)
+{
+  EphyHistoryService *self = EPHY_HISTORY_SERVICE (object);
+
+  G_OBJECT_CLASS (ephy_history_service_parent_class)->constructed (object);
+
+  self->queue = g_async_queue_new ();
+
+  /* This value is checked in several functions to verify that they are only
+   * ever run on the history thread. Accordingly, we'd better be sure it's set
+   * before it is checked for the first time. That requires a lock here. */
+  g_mutex_lock (&self->history_thread_mutex);
+  self->history_thread = g_thread_new ("EphyHistoryService", (GThreadFunc)run_history_service_thread, self);
+
+  /* Additionally, make sure the SQLite connection has really been opened before
+   * returning. We need this so that we can test that using a read-only service
+   * at the same time as a read/write service does not cause the read/write
+   * service to break. This delay is required because we need to be sure the
+   * read/write service has completed initialization before attempting to open
+   * the read-only service, or initializing the read-only service will fail.
+   * This isn't needed except in test mode, because only tests might run
+   * multiple history services, but it's harmless and cleaner to do always.
+   */
+  while (!self->history_thread_initialized)
+    g_cond_wait (&self->history_thread_initialized_condition, &self->history_thread_mutex);
+
+  g_mutex_unlock (&self->history_thread_mutex);
+}
+
 static gboolean
 emit_urls_visited (EphyHistoryService *self)
 {
@@ -181,6 +211,7 @@ ephy_history_service_class_init (EphyHistoryServiceClass *klass)
 
   gobject_class->finalize = ephy_history_service_finalize;
   gobject_class->dispose = ephy_history_service_dispose;
+  gobject_class->constructed = ephy_history_service_constructed;
   gobject_class->get_property = ephy_history_service_get_property;
   gobject_class->set_property = ephy_history_service_set_property;
 
@@ -268,14 +299,6 @@ ephy_history_service_class_init (EphyHistoryServiceClass *klass)
 static void
 ephy_history_service_init (EphyHistoryService *self)
 {
-  self->queue = g_async_queue_new ();
-
-  /* This value is checked in several functions to verify that they are only
-   * ever run on the history thread. Accordingly, we'd better be sure it's set
-   * before it is checked for the first time. That requires a lock here. */
-  g_mutex_lock (&self->history_thread_mutex);
-  self->history_thread = g_thread_new ("EphyHistoryService", (GThreadFunc)run_history_service_thread, self);
-  g_mutex_unlock (&self->history_thread_mutex);
 }
 
 EphyHistoryService *
@@ -497,6 +520,7 @@ static gpointer
 run_history_service_thread (EphyHistoryService *self)
 {
   EphyHistoryServiceMessage *message;
+  gboolean success;
 
   /* Note that self->history_thread is only written once, and that's guaranteed
    * to have occurred before we enter this critical section due to this mutex.
@@ -505,9 +529,14 @@ run_history_service_thread (EphyHistoryService *self)
    */
   g_mutex_lock (&self->history_thread_mutex);
   g_assert (self->history_thread == g_thread_self ());
+
+  success = ephy_history_service_open_database_connections (self);
+
+  self->history_thread_initialized = TRUE;
+  g_cond_signal (&self->history_thread_initialized_condition);
   g_mutex_unlock (&self->history_thread_mutex);
 
-  if (ephy_history_service_open_database_connections (self) == FALSE)
+  if (!success)
     return NULL;
 
   do {
diff --git a/tests/ephy-history-test.c b/tests/ephy-history-test.c
index ce951cd..c1eef95 100644
--- a/tests/ephy-history-test.c
+++ b/tests/ephy-history-test.c
@@ -95,11 +95,6 @@ static void
 test_readonly_mode (void)
 {
   gchar *temporary_file = g_build_filename (g_get_tmp_dir (), "epiphany-history-test.db", NULL);
-  /* FIXME: This is racy, since we need to be sure the history thread of the
-   * first EphyHistoryService object has created the database before starting
-   * the history thread of the second EphyHistoryService object. This is a test
-   * bug, not an application bug.
-   */
   EphyHistoryService *service = ensure_empty_history (temporary_file);
   EphyHistoryService *readonly_service = ephy_history_service_new (temporary_file, 
EPHY_SQLITE_CONNECTION_MODE_READ_ONLY);