GNOME Bugzilla – Bug 705289
Crash when removing an individual with 2 aggregators instantiated
Last modified: 2013-08-05 12:49:57 UTC
See the attached unit test. It works fine if I don't instantiate _aggregator2 and call _unlink_individuals() right away. I don't think I'm doing anything wrong here so I suspect Folks doesn't play nice when we have more than once aggregator instantiated. (/home/cassidy/dev/tizen-pc/folks/tests/eds/.libs/lt-unlink-double-aggregator:16838): folks-WARNING **: Link map contains invalid mapping: test:pas-id-51FA65C200000000 → e1c80aab7c00163f1a50f2c2ca922187995ddaca (0x1cd45a0)
+ Trace 232323
Created attachment 250621 [details] [review] add unit test for bgo#705289 Change-Id: I48adbd4acc7cf25a6daff3c61a4357873b8df398
Note that if we remove the individual instead of unlinking we trigger another crash. - yield this._aggregator.unlink_individual(individuals[0]); + yield this._aggregator.remove_individual(individuals[0]); folks:ERROR:/home/cassidy/dev/tizen-pc/folks/folks/individual.vala:2476:_folks_individual_set_personas: assertion failed: (replacement_individual == null || replacement_individual != this)
+ Trace 232325
Review of attachment 250621 [details] [review]: Looks useful. Please commit after fixing the minor comments below. I’ll look into the crash itself later. ::: tests/eds/unlink-double-aggregator.vala @@ +65,3 @@ + try + { + yield aggregator.prepare(); Missing space before ‘()’, and in a few other places in the file. @@ +69,3 @@ + catch (GLib.Error e) + { + GLib.warning ("Error when calling prepare: %s\n", e.message); This should be a GLib.error(). @@ +78,3 @@ + + this._aggregator.notify["is-quiescent"].connect (this._link_individuals); + _prepare_aggregator(this._aggregator); ‘this._prepare_aggregator (this._aggregator)’. @@ +98,3 @@ + catch (GLib.Error e) + { + GLib.warning ("link_personas: %s\n", e.message); This should be a GLib.error() as well. @@ +122,3 @@ + catch (GLib.Error e) + { + GLib.warning ("unlink_individual: %s\n", e.message); This should also be a GLib.error().
(In reply to comment #3) > Review of attachment 250621 [details] [review]: > > Looks useful. Please commit after fixing the minor comments below. I’ll look > into the crash itself later. Should I really commit it without the fix? That will break "make check". > ::: tests/eds/unlink-double-aggregator.vala > @@ +65,3 @@ > + try > + { > + yield aggregator.prepare(); > > Missing space before ‘()’, and in a few other places in the file. Fixed. > @@ +69,3 @@ > + catch (GLib.Error e) > + { > + GLib.warning ("Error when calling prepare: %s\n", e.message); > > This should be a GLib.error(). done. > @@ +78,3 @@ > + > + this._aggregator.notify["is-quiescent"].connect > (this._link_individuals); > + _prepare_aggregator(this._aggregator); > > ‘this._prepare_aggregator (this._aggregator)’. done. > @@ +98,3 @@ > + catch (GLib.Error e) > + { > + GLib.warning ("link_personas: %s\n", e.message); > > This should be a GLib.error() as well. > done. > @@ +122,3 @@ > + catch (GLib.Error e) > + { > + GLib.warning ("unlink_individual: %s\n", e.message); > > This should also be a GLib.error(). done.
Created attachment 250679 [details] [review] add unit test for bgo#705289 https://bugzilla.gnome.org/show_bug.cgi?id=705289 Change-Id: Ief7bef8651ae1df8b2a927ae22fd5fb162e43466
For some reason I can no longer reproduce the 'unlink error', even using the old test?! Anyway, here is an updated test reproducing the 'remove' error (which was the one I was trying to reproduce).
Review of attachment 250679 [details] [review]: Looks good. Please commit with the change below. Thanks. ::: tests/eds/Makefile.am @@ +84,3 @@ set-is-favourite \ perf \ + double-aggregator \ Move this a bit higher up TESTS, since they’re ordered by increasing complexity (i.e. increasing likelihood of failure).
Created attachment 250680 [details] [review] add unit test for bgo#705289 https://bugzilla.gnome.org/show_bug.cgi?id=705289 Change-Id: Ief7bef8651ae1df8b2a927ae22fd5fb162e43466
Comment on attachment 250680 [details] [review] add unit test for bgo#705289 Attachment 250680 [details] pushed as 1b7e8d8 - add unit test for bgo#705289
Created attachment 250704 [details] Debug log Log generated using: FOLKS_DEBUG=all G_MESSAGES_DEBUG=all make check TESTS=double-aggregator CHECK_VERBOSE=1
As mentioned on IRC, I think this is because the two IA instances create separate Individuals which use the same Personas (because the IAs share PersonaStore instances). The IAs can then get cross-over between their sets of Individuals by following the Persona.individual pointers, which could result in one IA using Individuals created by the other in its aggregation calculations, and hence exploding. Unless you’ve got a really good use-case for using multiple IA instances in a single process, I’m very inclined to explicitly document this as unsupported, and require that IndividualAggregator be a singleton. Without having looked too deeply into it, I suspect that supporting multiple IAs in a single process will be quite complex, and the aggregator code’s complex enough already.
Yeah that was my conclusion from my investigations as well. I think it's fine to claim that multiple IA aren't supported but then we should enforce this behaviour but forcing the IA implementation as a singleton (_new() would return a ref on an existing instance if any). Would you be ok with such change? Btw, I'm not supposed to have multiple IA instance either, but I end up with 2 because of JS/Seed's garbage collector being weird/buggy, leading me to this crash.
(In reply to comment #12) > Yeah that was my conclusion from my investigations as well. > > I think it's fine to claim that multiple IA aren't supported but then we should > enforce this behaviour but forcing the IA implementation as a singleton (_new() > would return a ref on an existing instance if any). Would you be ok with such > change? Sounds good to me. The documentation would need updating as well, and your test case commit would need reverting. > Btw, I'm not supposed to have multiple IA instance either, but I end up with 2 > because of JS/Seed's garbage collector being weird/buggy, leading me to this > crash. Do you think that’s potentially a bug in folks, or just JS?
Created attachment 250839 [details] [review] aggregator: add dup() method returning a singleton As discussed on bgo#705289 we don't support having different aggregator instances as they would share the same set of personas. Adding this singleton makes things easier for client applications.
Created attachment 250840 [details] [review] Revert "add unit test for bgo#705289" It doesn't make sense any more as multiple aggregators are not supported any more. This reverts commit 1b7e8d83bb7488aac7bcf2d59df3a7b35f316ae2.
Review of attachment 250839 [details] [review]: ::: folks/individual-aggregator.vala @@ +372,3 @@ + * instantiated at the same time. So it's recommended to use + * {@link IndividualAggregator.dup} if you need to instantiate it more than + * once. This constructor should be deprecated (and that should be mentioned in NEWS). I think you should also deprecate .with_backend_store() and instead add a new static .dup_with_backend_store() which has the behaviour we discussed on IRC (i.e. it creates a new IA with the given backend store if (_instance == null), but returns null if (_instance != null && _instance.backend_store != backend_store)). Because of the deprecations, all the folks tests should be ported to use the .dup() constructor. That can be a separate commit.
Review of attachment 250840 [details] [review]: Please commit to master immediately. :-)
Created attachment 250863 [details] [review] aggregator: add dup() methods returning a singleton As discussed on bgo#705289 we don't support having different aggregator instances as they would share the same set of personas. Adding this singleton makes things easier for client applications.
Created attachment 250865 [details] [review] tests: use IndividualAggregator.dup () The default constructor has been deprecated.
Review of attachment 250863 [details] [review]: Looks good. Don’t forget to mention this bug under “Bugs fixed” in NEWS. Thanks!
Created attachment 250866 [details] [review] use IndividualAggregator.dup () in tests and tools The default constructor has been deprecated.
Review of attachment 250866 [details] [review]: Go go go!
Attachment 250863 [details] pushed as ed8e377 - aggregator: add dup() methods returning a singleton Attachment 250866 [details] pushed as 4322477 - use IndividualAggregator.dup () in tests and tools