[epiphany/gnome-3-22] history-service: Fix multiple initialization race conditions

[Michael Catanzaro <mcatanzaro src gnome org>]

commit feae9a35c59bb5abd6ab6708dac82bcaf9c605ef
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 7e09ddd..51b4705 100644
--- a/lib/history/ephy-history-service-private.h
+++ b/lib/history/ephy-history-service-private.h
@@ -26,6 +26,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 6c71af7..3e15749 100644
--- a/lib/history/ephy-history-service.c
+++ b/lib/history/ephy-history-service.c
@@ -152,6 +152,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)
 {
@@ -178,6 +208,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;
 
@@ -265,14 +296,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 *
@@ -494,6 +517,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.
@@ -502,9 +526,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 5ee6181..ef2d465 100644
--- a/tests/ephy-history-test.c
+++ b/tests/ephy-history-test.c
@@ -97,11 +97,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);