| Summary: | Amarok crashes when searching the Icecast directory | ||
|---|---|---|---|
| Product: | [Applications] amarok | Reporter: | zulurumu <wrcadk> |
| Component: | Services/Podcast Directory | Assignee: | Matěj Laitl <matej> |
| Status: | RESOLVED FIXED | ||
| Severity: | crash | CC: | ralf-engels |
| Priority: | NOR | Keywords: | drkonqi |
| Version: | 2.8.0 | ||
| Target Milestone: | 2.9 | ||
| Platform: | Fedora RPMs | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/amarok/7e9e0710daecdb2c6372a699e9af967f485c66c0 | Version Fixed In: | 2.9 |
| Sentry Crash Report: | |||
|
Description
zulurumu
2014-05-07 18:33:57 UTC
Probably caused by f3e894f9f1385c1c4f20ddf2adfb090e269914e4 Please check: - ScriptableServiceTrack *track = new ScriptableServiceTrack( name ); + KSharedPtr<ScriptableServiceTrack> track( new ScriptableServiceTrack( name ) ); It seems like all the ScriptableService::add* functions assume that they take the ownership of the pointer (a comment would be nice in the .h file to point that out) Specifically addTrack will free the track in several cases. Giving a share pointer is probably a mistake. Matěj please check. it's your commit and I can't see why only track is given as shared pointer. Git commit 7e9e0710daecdb2c6372a699e9af967f485c66c0 by Matěj Laitl. Committed on 02/11/2014 at 10:01. Pushed by laitl into branch 'master'. scriptable service: fix a crash due to double deletion of an object Thanks go to Ralf Engels for pointing out the problematic code. The problem is IMO not directly in insertItem() using KSharedPtr, but in addTrack() which creates a KSharedPtr out of a track pointer, but at the same time calls delete on the track pointer in some circumstances. Once a pointer is KSharedPtr-managed, delete must not be used. I've kept KSharedPtr for track in insertItem(). While using it is a bit inconsistent wrt. surrounding code, it should have no effect (the crash is IMO caused by addTrack() code alone). It feels more correct as it prevents a hypothetical (currently unreachable) memory leak. Interested folks, please test whether this really fixes bug 334479 for you. FIXED-IN: 2.9 M +1 -0 ChangeLog M +0 -3 src/services/scriptable/ScriptableService.cpp http://commits.kde.org/amarok/7e9e0710daecdb2c6372a699e9af967f485c66c0 Hi Matěj, you are right. The shared pointer as such is not bad. I thought about different possibilities to fix the issue. Using shared pointers from the begin on actually seems to be the best solution. But then for all of the add* functions. I actually can't try out the issue since I couldn't reproduce it in the first place. Just spotted the issue. But I assume that making a shared pointer out of a shared pointer will definitely cause an issue. In this case I assume that one of the shared pointers ran out of scope. (In reply to Ralf Engels from comment #3) > I thought about different possibilities to fix the issue. Using shared > pointers from the begin on actually seems to be the best solution. But then > for all of the add* functions. Righto. That would be the second step, I've just picked the low hanging fruit and fixed the crash, which is kinda selfish. ;) > But I assume that making a shared pointer out of a shared pointer will > definitely cause an issue. In this case I assume that one of the shared > pointers ran out of scope. With KSharedPtr (refcount in pointed object) this should be perfectly fine: KSharedPtr<Object> shared1( new Object() ); Object *plain = shared1.data(); { KSharedPtr<Object> shared2( plain ); (...) } Object is destroyed only after shared1 goes out of scope. With QSharedPointer (refcount in pointer), the code would be of course incorrect. (shared2 would prematurely delete Object) |