Commit 58dba5a8 authored by Philip Withnall's avatar Philip Withnall
Browse files

backends: Tidy up prepare() and unprepare() methods’ mutual exclusion

As discovered in bgo#665728, all our prepare() (and unprepare()) methods are
currently vulnerable to being run multiple times concurrently from a single
thread. Add a _pending_prepare flag to all of them to prevent this. It
doesn't need to be locked, since it should only ever be accessed from a
single thread (since only a single thread can get through the lock{}
recursive mutex at once).

Helps: bgo#665728
parent 8bdeef85
......@@ -37,6 +37,7 @@ public class Folks.Backends.Eds.Backend : Folks.Backend
private static const string _use_address_books =
"FOLKS_BACKEND_EDS_USE_ADDRESS_BOOKS";
private bool _is_prepared = false;
private bool _prepare_pending = false; /* used for unprepare() too */
private bool _is_quiescent = false;
private HashMap<string, PersonaStore> _persona_stores;
private Map<string, PersonaStore> _persona_stores_ro;
......@@ -95,8 +96,15 @@ public class Folks.Backends.Eds.Backend : Folks.Backend
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._prepare_pending = true;
this._create_avatars_cache_dir ();
E.BookClient.get_sources (out this._ab_sources);
......@@ -110,6 +118,10 @@ public class Folks.Backends.Eds.Backend : Folks.Backend
this._is_quiescent = true;
this.notify_property ("is-quiescent");
}
finally
{
this._prepare_pending = false;
}
}
}
......@@ -120,8 +132,15 @@ public class Folks.Backends.Eds.Backend : Folks.Backend
{
lock (this._is_prepared)
{
if (this._is_prepared)
if (!this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._prepare_pending = true;
foreach (var persona_store in this._persona_stores.values)
{
this._remove_address_book (persona_store);
......@@ -136,6 +155,10 @@ public class Folks.Backends.Eds.Backend : Folks.Backend
this._is_prepared = false;
this.notify_property ("is-prepared");
}
finally
{
this._prepare_pending = false;
}
}
}
......
......@@ -37,6 +37,7 @@ public class Edsf.PersonaStore : Folks.PersonaStore
private HashMap<string, Persona> _personas;
private Map<string, Persona> _personas_ro;
private bool _is_prepared = false;
private bool _prepare_pending = false;
private bool _is_quiescent = false;
private E.BookClient _addressbook;
private E.BookClientView _ebookview;
......@@ -533,11 +534,13 @@ public class Edsf.PersonaStore : Folks.PersonaStore
/* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652637 */
lock (this._is_prepared)
{
if (this._is_prepared)
if (this._is_prepared == true || this._prepare_pending == true)
{
return;
}
this._prepare_pending = true;
try
{
/* Listen for removal signals for the address book. There's no
......@@ -625,11 +628,16 @@ public class Edsf.PersonaStore : Folks.PersonaStore
* and the second is an error message. */
_("Couldn't open address book ‘%s’: %s"), this.id, e1.message);
}
finally
{
this._prepare_pending = false;
}
if (this._addressbook.is_opened () == false)
{
/* Remove the persona store on error */
this.removed ();
this._prepare_pending = false;
throw new PersonaStoreError.INVALID_ARGUMENT (
/* Translators: the parameter is an address book URI. */
......@@ -697,6 +705,10 @@ public class Edsf.PersonaStore : Folks.PersonaStore
/* Translators: the parameteter is an error message. */
_("Couldn't get address book capabilities: %s"), e2.message);
}
finally
{
this._prepare_pending = false;
}
/* Get the set of capabilities supported by the address book.
* Specifically, we're looking for do-initial-query, which signifies
......@@ -724,6 +736,10 @@ public class Edsf.PersonaStore : Folks.PersonaStore
/* Translators: the parameteter is an error message. */
_("Couldn't get address book capabilities: %s"), e4.message);
}
finally
{
this._prepare_pending = false;
}
bool got_view = false;
try
......@@ -815,8 +831,13 @@ public class Edsf.PersonaStore : Folks.PersonaStore
_("Couldn't get view for address book ‘%s’: %s"),
this.id, e3.message);
}
finally
{
this._prepare_pending = false;
}
this._is_prepared = true;
this._prepare_pending = false;
this.notify_property ("is-prepared");
/* If the address book isn't going to do an initial query (i.e.
......
......@@ -35,6 +35,7 @@ extern const string BACKEND_NAME;
public class Folks.Backends.Kf.Backend : Folks.Backend
{
private bool _is_prepared = false;
private bool _prepare_pending = false; /* used for unprepare() too */
private bool _is_quiescent = false;
private HashMap<string, PersonaStore> _persona_stores;
private Map<string, PersonaStore> _persona_stores_ro;
......@@ -92,8 +93,15 @@ public class Folks.Backends.Kf.Backend : Folks.Backend
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._prepare_pending = true;
File file;
unowned string path = Environment.get_variable (
"FOLKS_BACKEND_KEY_FILE_PATH");
......@@ -130,6 +138,10 @@ public class Folks.Backends.Kf.Backend : Folks.Backend
this._is_quiescent = true;
this.notify_property ("is-quiescent");
}
finally
{
this._prepare_pending = false;
}
}
}
......@@ -138,19 +150,36 @@ public class Folks.Backends.Kf.Backend : Folks.Backend
*/
public override async void unprepare () throws GLib.Error
{
foreach (var persona_store in this._persona_stores.values)
lock (this._is_prepared)
{
this.persona_store_removed (persona_store);
}
if (!this._is_prepared || this._prepare_pending == true)
{
return;
}
this._persona_stores.clear ();
this.notify_property ("persona-stores");
try
{
this._prepare_pending = true;
this._is_quiescent = false;
this.notify_property ("is-quiescent");
foreach (var persona_store in this._persona_stores.values)
{
this.persona_store_removed (persona_store);
}
this._persona_stores.clear ();
this.notify_property ("persona-stores");
this._is_quiescent = false;
this.notify_property ("is-quiescent");
this._is_prepared = false;
this.notify_property ("is-prepared");
this._is_prepared = false;
this.notify_property ("is-prepared");
}
finally
{
this._prepare_pending = false;
}
}
}
private void _store_removed_cb (Folks.PersonaStore store)
......
......@@ -38,6 +38,7 @@ public class Folks.Backends.Kf.PersonaStore : Folks.PersonaStore
private GLib.KeyFile _key_file;
private unowned Cancellable _save_key_file_cancellable = null;
private bool _is_prepared = false;
private bool _prepare_pending = false;
private bool _is_quiescent = false;
private const string[] _always_writeable_properties =
......@@ -168,8 +169,15 @@ public class Folks.Backends.Kf.PersonaStore : Folks.PersonaStore
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._prepare_pending = true;
var filename = this._file.get_path ();
this._key_file = new GLib.KeyFile ();
......@@ -283,6 +291,10 @@ public class Folks.Backends.Kf.PersonaStore : Folks.PersonaStore
this._is_quiescent = true;
this.notify_property ("is-quiescent");
}
finally
{
this._prepare_pending = false;
}
}
}
......
......@@ -37,6 +37,7 @@ public class Swf.PersonaStore : Folks.PersonaStore
private HashMap<string, Persona> _personas;
private Map<string, Persona> _personas_ro;
private bool _is_prepared = false;
private bool _prepare_pending = false;
private bool _is_quiescent = false;
private ClientService _service;
private ClientContactView _contact_view;
......@@ -210,8 +211,10 @@ public class Swf.PersonaStore : Folks.PersonaStore
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (!this._is_prepared && !this._prepare_pending)
{
this._prepare_pending = true;
/* Take a reference to the PersonaStore while waiting for the
* async call to return. See: bgo#665039. */
this.ref ();
......@@ -222,6 +225,7 @@ public class Swf.PersonaStore : Folks.PersonaStore
if (caps == null)
{
this.unref ();
this._prepare_pending = false;
return;
}
......@@ -230,6 +234,7 @@ public class Swf.PersonaStore : Folks.PersonaStore
if (!has_contacts)
{
this.unref ();
this._prepare_pending = false;
return;
}
......@@ -247,6 +252,7 @@ public class Swf.PersonaStore : Folks.PersonaStore
if (contact_view == null)
{
this.unref ();
this._prepare_pending = false;
return;
}
......@@ -259,6 +265,7 @@ public class Swf.PersonaStore : Folks.PersonaStore
this._contact_view = contact_view;
this._is_prepared = true;
this._prepare_pending = false;
this.notify_property ("is-prepared");
/* FIXME: for lsw Stores with 0 contacts or badly
......
......@@ -34,6 +34,7 @@ extern const string BACKEND_NAME;
public class Folks.Backends.Sw.Backend : Folks.Backend
{
private bool _is_prepared = false;
private bool _prepare_pending = false;
private bool _is_quiescent = false;
private Client _client;
private HashMap<string, PersonaStore> _persona_stores;
......@@ -91,8 +92,10 @@ public class Folks.Backends.Sw.Backend : Folks.Backend
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (!this._is_prepared && !this._prepare_pending)
{
this._prepare_pending = true;
/* Hold a ref. on the Backend while we wait for the callback from
* this._client.get_services() to prevent the Backend being
* destroyed in the mean time. See: bgo#665039. */
......@@ -105,6 +108,7 @@ public class Folks.Backends.Sw.Backend : Folks.Backend
this.add_service (service_name);
this._is_prepared = true;
this._prepare_pending = false;
this.notify_property ("is-prepared");
this._is_quiescent = true;
......@@ -121,22 +125,33 @@ public class Folks.Backends.Sw.Backend : Folks.Backend
*/
public override async void unprepare () throws GLib.Error
{
foreach (var store in this._persona_stores.values)
lock (this._is_prepared)
{
store.removed.disconnect (this.store_removed_cb);
this.persona_store_removed (store);
}
if (!this._is_prepared || this._prepare_pending)
{
return;
}
this._client = null;
this._prepare_pending = true;
this._persona_stores.clear ();
this.notify_property ("persona-stores");
foreach (var store in this._persona_stores.values)
{
store.removed.disconnect (this.store_removed_cb);
this.persona_store_removed (store);
}
this._client = null;
this._is_quiescent = false;
this.notify_property ("is-quiescent");
this._persona_stores.clear ();
this.notify_property ("persona-stores");
this._is_prepared = false;
this.notify_property ("is-prepared");
this._is_quiescent = false;
this.notify_property ("is-quiescent");
this._is_prepared = false;
this._prepare_pending = false;
this.notify_property ("is-prepared");
}
}
private void add_service (string service_name)
......
......@@ -562,7 +562,12 @@ public class Tpf.PersonaStore : Folks.PersonaStore
{
lock (this._is_prepared)
{
if (!this._is_prepared && !this._prepare_pending)
if (this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._prepare_pending = true;
......@@ -629,9 +634,12 @@ public class Tpf.PersonaStore : Folks.PersonaStore
}
this._is_prepared = true;
this._prepare_pending = false;
this.notify_property ("is-prepared");
}
finally
{
this._prepare_pending = false;
}
}
}
......
......@@ -34,6 +34,7 @@ public class Folks.Backends.Tp.Backend : Folks.Backend
{
private AccountManager _account_manager;
private bool _is_prepared = false;
private bool _prepare_pending = false; /* used by unprepare() too */
private bool _is_quiescent = false;
private HashMap<string, PersonaStore> _persona_stores;
private Map<string, PersonaStore> _persona_stores_ro;
......@@ -91,7 +92,12 @@ public class Folks.Backends.Tp.Backend : Folks.Backend
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._account_manager = AccountManager.dup ();
yield this._account_manager.prepare_async (null);
......@@ -113,6 +119,10 @@ public class Folks.Backends.Tp.Backend : Folks.Backend
this._is_quiescent = true;
this.notify_property ("is-quiescent");
}
finally
{
this._prepare_pending = false;
}
}
}
......@@ -121,28 +131,40 @@ public class Folks.Backends.Tp.Backend : Folks.Backend
*/
public override async void unprepare () throws GLib.Error
{
if (!this._is_prepared)
return;
lock (this._is_prepared)
{
if (!this._is_prepared || this._prepare_pending)
{
return;
}
this._account_manager.account_enabled.disconnect (
this._account_enabled_cb);
this._account_manager.account_validity_changed.disconnect (
this._account_validity_changed_cb);
this._account_manager = null;
try
{
this._account_manager.account_enabled.disconnect (
this._account_enabled_cb);
this._account_manager.account_validity_changed.disconnect (
this._account_validity_changed_cb);
this._account_manager = null;
foreach (var persona_store in this._persona_stores.values)
{
this.persona_store_removed (persona_store);
}
foreach (var persona_store in this._persona_stores.values)
{
this.persona_store_removed (persona_store);
}
this._persona_stores.clear ();
this.notify_property ("persona-stores");
this._persona_stores.clear ();
this.notify_property ("persona-stores");
this._is_quiescent = false;
this.notify_property ("is-quiescent");
this._is_quiescent = false;
this.notify_property ("is-quiescent");
this._is_prepared = false;
this.notify_property ("is-prepared");
this._is_prepared = false;
this.notify_property ("is-prepared");
}
finally
{
this._prepare_pending = false;
}
}
}
private void _account_validity_changed_cb (Account account, bool valid)
......
......@@ -166,6 +166,7 @@ public class Trf.PersonaStore : Folks.PersonaStore
private HashMap<string, Persona> _personas;
private Map<string, Persona> _personas_ro;
private bool _is_prepared = false;
private bool _prepare_pending = false;
private bool _is_quiescent = false;
private static const int _default_timeout = 100;
private Resources _resources_object;
......@@ -1056,8 +1057,15 @@ public class Trf.PersonaStore : Folks.PersonaStore
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._prepare_pending = true;
try
{
this._connection =
......@@ -1113,6 +1121,10 @@ public class Trf.PersonaStore : Folks.PersonaStore
throw new PersonaStoreError.INVALID_ARGUMENT (e3.message);
}
}
finally
{
this._prepare_pending = false;
}
}
}
......
......@@ -33,6 +33,7 @@ extern const string BACKEND_NAME;
public class Folks.Backends.Tr.Backend : Folks.Backend
{
private bool _is_prepared = false;
private bool _prepare_pending = false; /* used by unprepare() too */
private bool _is_quiescent = false;
private HashMap<string, PersonaStore> _persona_stores;
private Map<string, PersonaStore> _persona_stores_ro;
......@@ -89,8 +90,15 @@ public class Folks.Backends.Tr.Backend : Folks.Backend
{
lock (this._is_prepared)
{
if (!this._is_prepared)
if (this._is_prepared || this._prepare_pending)
{
return;
}
try
{
this._prepare_pending = true;
this._add_default_persona_store ();
this._is_prepared = true;
......@@ -99,6 +107,10 @@ public class Folks.Backends.Tr.Backend : Folks.Backend
this._is_quiescent = true;
this.notify_property ("is-quiescent");
}
finally
{
this._prepare_pending = false;
}
}
}
......@@ -107,19 +119,36 @@ public class Folks.Backends.Tr.Backend : Folks.Backend
*/
public override async void unprepare () throws GLib.Error
{
foreach (var persona_store in this._persona_stores.values)
lock (this._is_prepared)
{
this.persona_store_removed (persona_store);
}
if (!this._is_prepared || this._prepare_pending)
{
return;
}
this._persona_stores.clear ();
this.notify_property ("persona-stores");
try
{
this._prepare_pending = true;
foreach (var persona_store in this._persona_stores.values)
{
this.persona_store_removed (persona_store);
}
this._is_quiescent = false;
this.notify_property ("is-quiescent");
this._persona_stores.clear ();
this.notify_property ("persona-stores");
this._is_quiescent = false;
this.notify_property ("is-quiescent");
this._is_prepared = false;
this.notify_property ("is-prepared");
this._is_prepared = false;
this.notify_property ("is-prepared");
}
finally
{
this._prepare_pending = false;
}
}
}
/**
......
......@@ -116,6 +116,12 @@ public abstract class Folks.Backend : Object
*
* This function is guaranteed to be idempotent (since version 0.3.0).
*
* Concurrent calls to this function from different threads will block until
* preparation has completed. However, concurrent calls to this function from
* a single thread might not, i.e. the first call will block but subsequent