[Nepomuk] Bug 292996

[Vishesh Handa]

On Tue, Dec 4, 2012 at 8:06 AM, Simeon Bird <bladud at gmail.com> wrote:

> Hi everyone,
>
> I think I figured out Bug 292996 (crash in Nepomuk2::~Resource).
>
> So the problem is that kontact has a class,
> NepomukResourceRetrieverRunnable, which inherits from QRunnable, and
> creates a Resource on the stack of it's QThread.
>
> But, unknown to it, it is actually getting a reference to a Resource
> stored by the ResourceManager, which has QApplication as a parent.
>
> When kontact exists, it deletes ResourceManager.
>
> But at this point the QRunnable has not yet been stopped,
> because the caller assumes that they have allocated
> Nepomuk::Resource on the QThread stack, and are not aware
> that it is secretly shared with the other threads via
> the singleton class in ResourceManager.
>
> The QRunnable persists, but now resources don't point to anything.
>
> Boom! Crash.
>

You're a genius! :)


>
> Ok, so that's the crash.
>
> There are two possible fixes:
>
> The first is just to have the ResourceManager destructor check the
> ref-count for all its resources in a loop and not go away anything until
> they are all zero. This is fairly non-invasive but ugly.


Ugly. I might have a better solution.


>
> The second is: do we really need to share the Resources in a singleton
> class anyway? So far as I can see, it's only there to speed up multiple
> accesses to the same resource, which I would imagine is quite rare in
> practice (most people will want multiple different resources). If most of
> the time the reference count on the resources is 1, it will be faster just
> to allocate them directly because we won't be doing the whole locking dance
> around ResourceManager, nor will we need to search all previously allocated
> Resources.
>

Historically, the ResourceManager never used to delete its reference to the
ResourceData, even when all the Resources containing that ResourceData had
been deleted. I removed that for 4.9, cause it used to cause a ton of
problems. Also, I was planning on using shared memory in the ResourceData
class. I've postponed that decision to focus on more important stuff for
now. Maybe we should introduce it again (checkout
ResourceManager::shouldBeDeleted)

Here is why I would like to keep the cache in the ResourceManager -

1. It makes copying resources very cheap. Arguably one can achieve the same
thing by just using QSharedData or just reference counting.
2. It makes maintaining an up to date cache a lot easier. We already need
to hook up the ResourceManager in order to keep the Resource cache up to
date across difference applications. Without the ResourceManager, we would
have to use some manager to do the same across multiple Resource classes.

I'm not sure how often multiple accesses happen in practise, but it would
be unwise to change the Resource's accepted behaviour.

>
> Someone who uses kmail (I don't) could check this fairly easily by adding
> debug statements that print the maximal reference count of each Resource a
> few times...
>
> Or am I missing some other feature of keeping the Resources in
> ResourceManager?
>

Initializing the Resource class the first time is very expensive. The
ResourceData::load() method is quite slow (about ~100ms), we want to avoid
having to reload the ResourceData as much as we can.


> Even if the second fix is a good idea, it seems too big a change to
> implement this close to release, so I was thinking the first fix for 4.10,
> and the second for 4.11. What do you think?
>

I don't particularly like the idea of the second fix. Sorry.

I have another solution -

When destroying the ResourceManager, we could iterate over all the
ResourceData*, and set m_rm = 0. Then in Resource::~Resource we could check
if m_rm != 0. Or we could simply set the Resource::m_data = 0. That way
these Resource classes will just contain a null pointer. Let me prepare a
patch.


>
> Simeon
>
> _______________________________________________
> Nepomuk mailing list
> Nepomuk at kde.org
> https://mail.kde.org/mailman/listinfo/nepomuk
>
>


-- 
Vishesh Handa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20121204/f65295e3/attachment.html>