-
-
libpurple/purplenotificationmanager.c (Diff revision 1) I can't figure out why but at this location there is an invalid read showing up in valgrind when this function is called a 2nd time in
test_purple_notification_manager_double_remove
.==13780== Invalid read of size 1 ==13780== at 0x49F72FA: g_str_hash (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x49F5E97: g_hash_table_lookup (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x48D08AB: purple_notification_manager_remove (purplenotificationmanager.c:334) ==13780== by 0x109BA8: test_purple_notification_manager_double_remove (test_notification_manager.c:158) ==13780== by 0x4A32694: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A323BA: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A32B98: g_test_run_suite (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A32C0C: g_test_run (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x10A481: main (test_notification_manager.c:250) ==13780== Address 0x10d340d0 is 0 bytes inside a block of size 37 free'd ==13780== at 0x4840EB7: free (vg_replace_malloc.c:872) ==13780== by 0x4A0F177: g_free (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x48CF5AD: purple_notification_finalize (purplenotification.c:227) ==13780== by 0x496ABE2: g_object_unref (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2) ==13780== by 0x48D09C7: purple_notification_manager_remove (purplenotificationmanager.c:365) ==13780== by 0x109B6B: test_purple_notification_manager_double_remove (test_notification_manager.c:156) ==13780== by 0x4A32694: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A323BA: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A32B98: g_test_run_suite (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A32C0C: g_test_run (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x10A481: main (test_notification_manager.c:250) ==13780== Block was alloc'd at ==13780== at 0x483E670: malloc (vg_replace_malloc.c:381) ==13780== by 0x4B7F618: __vasprintf_internal (vasprintf.c:71) ==13780== by 0x4A59035: g_vasprintf (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A29B44: g_strdup_vprintf (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A29B8F: g_strdup_printf (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x4A480FD: g_uuid_string_random (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2) ==13780== by 0x48CEEFA: purple_notification_set_id (purplenotification.c:66) ==13780== by 0x48CF6CE: purple_notification_init (purplenotification.c:243) ==13780== by 0x4986887: g_type_create_instance (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2) ==13780== by 0x496B16D: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2) ==13780== by 0x496CA56: g_object_new_valist (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2) ==13780== by 0x496CE78: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2)
Phase 1 of the Notifications API
Review Request #1502 — Created June 10, 2022 and submitted
* Created PurpleNotification with unit tests. * Created PurpleNotificationManager with unit tests.
Ran the unit tests and ran Pidgin in the devenv.
Summary | ID |
---|---|
34edd95f2ab1afd7332c9cdae22a73a72904814b |
Description | From | Last Updated |
---|---|---|
Should there be a check that interactive actually changed, like it is done in purple_notification_set_read? |
ivanhoe | |
I can't figure out why but at this location there is an invalid read showing up in valgrind when this … |
ivanhoe | |
I'm encountering these curly braces on the same line as the function name so often that I start to wonder … |
ivanhoe | |
Is this correct? The notifications hash table is created with g_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_object_unref); so my understanding is that the … |
ivanhoe |
-
-
libpurple/purplenotification.c (Diff revision 1) I think that curly brace at the end should get a new line :)
-
libpurple/purplenotification.c (Diff revision 1) I think that curly brace at the end should get a new line. Same for
purple_notification_init
andpurple_notification_class_init
. -
libpurple/purplenotification.c (Diff revision 1) I think it would read better if it were
This **is** always represented ...
-
-
-
-
-
-
libpurple/purplenotification.c (Diff revision 1) Should there be a check that interactive actually changed, like it is done in
purple_notification_set_read
? -
-
-
libpurple/tests/test_notification.c (Diff revision 1) I'm encountering these curly braces on the same line as the function name so often that I start to wonder if I remember your style preferences correctly. They are all over the place here too, but there are also instances where they are on their own line so one or the other should be fixed.
Change Summary:
fix the invalid read and only notify when the interactive property actually changes.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+3430) |
-
-
libpurple/tests/test_notification_manager.c (Diff revisions 1 - 2) Is this correct? The
notifications
hash table is created withg_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_object_unref);
so my understanding is that the id is not freed when the entry is removed from the hash table.
If I understand your comment correctly wouldn't an alternative solution be to strdup the id? I tried that and it does not solve the invalid read.And regardless of me understanding this comment, is this a well usable API? I mean in this test case it is simple but when used in real application code you don't know in advance that an id is attempted to be removed a 2nd time. Would you then always acquire an additional reference, just in case? In
test_purple_notification_manager_add_remove
for example, the additional reference is not acquired, would we do that in real application code?