After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 749574 - audioaggregator: Deadlock in gst_object_sync_values()
audioaggregator: Deadlock in gst_object_sync_values()
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-19 10:42 UTC by Nirbheek Chauhan
Modified: 2015-08-16 13:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
An incorrect fix for the issue (1.20 KB, patch)
2015-05-19 10:42 UTC, Nirbheek Chauhan
none Details | Review
Sync pad values before taking the locks and aggregating (7.36 KB, patch)
2015-07-22 16:11 UTC, Nirbheek Chauhan
none Details | Review
Sync pad values before taking the locks and aggregating (7.34 KB, patch)
2015-07-22 18:46 UTC, Nirbheek Chauhan
committed Details | Review

Description Nirbheek Chauhan 2015-05-19 10:42:25 UTC
Created attachment 303584 [details] [review]
An incorrect fix for the issue

audioaggregator calls gst_object_sync_values() inside gst_audio_aggregator_fill_buffer() with the pad and aggregator locks taken. This is incorrect since if there's any changes scheduled using GstController, the sync_values call will deadlock while trying to make those changes (since making those changes requires taking those locks).

The attached patch is completely wrong, but serves to show the location of the issue. Ideally, fill_buffer() should be refactored to call sync_values() right before taking the locks the same way that videoaggregator does.
Comment 1 Tim-Philipp Müller 2015-05-19 10:51:40 UTC
A unit test for the issue would be great :)
Comment 2 Nirbheek Chauhan 2015-05-19 10:53:44 UTC
That crossed my mind too. Will write one. :)
Comment 3 Nirbheek Chauhan 2015-07-22 16:11:57 UTC
Created attachment 307919 [details] [review]
Sync pad values before taking the locks and aggregating

audioaggregator: Sync pad values before aggregating

We need to sync the pad values before taking the aggregator and pad locks
otherwise the element will just deadlock if there's any property changes
scheduled using GstController since that involves taking the aggregator and pad
locks.

Also add a test for this.

---

I've run the test without the patch to verify that the test actually works. The application deadlocks as expected.
Comment 4 Olivier Crête 2015-07-22 16:41:56 UTC
Review of attachment 307919 [details] [review]:

I'm not too happy about how long the object lock is held in audio aggregator, but in the mean time, this patch looks good.
Comment 5 Nirbheek Chauhan 2015-07-22 18:46:14 UTC
Created attachment 307925 [details] [review]
Sync pad values before taking the locks and aggregating

Patch wasn't applying due to your commit from yesterday. This fixes that. :)
Comment 6 Tim-Philipp Müller 2015-07-22 18:59:07 UTC
Thanks, pushed:

commit ad8cb458baf875c4bf77c8831fc7aeb926c41d82
Author: Nirbheek Chauhan <nirbheek@centricular.com>
Date:   Tue May 19 16:08:08 2015 +0530

    audioaggregator: Sync pad values before aggregating
    
    We need to sync the pad values before taking the aggregator and pad locks
    otherwise the element will just deadlock if there's any property changes
    scheduled using GstController since that involves taking the aggregator and pad
    locks.
    
    Also add a test for this.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749574