<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Dec 4, 2012 at 8:06 AM, Simeon Bird <span dir="ltr"><<a href="mailto:bladud@gmail.com" target="_blank">bladud@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi everyone, <br><br>I think I figured out Bug 292996 (crash in Nepomuk2::~Resource).<br><br>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. <br>

<br>But, unknown to it, it is actually getting a reference to a Resource stored by the ResourceManager, which has QApplication as a parent. <br><br>When kontact exists, it deletes ResourceManager.<br><br>But at this point the QRunnable has not yet been stopped, <br>

because the caller assumes that they have allocated <br>Nepomuk::Resource on the QThread stack, and are not aware <br>that it is secretly shared with the other threads via <br>the singleton class in ResourceManager.<br><br>

The QRunnable persists, but now resources don't point to anything.<br><br>Boom! Crash.<br></blockquote><div><br>You're a genius! :)<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>Ok, so that's the crash. <br><br>There are two possible fixes: <br><br>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.</blockquote>
<div><br>Ugly. I might have a better solution.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
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. <br>
</blockquote><div><br>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)<br>
<br>Here is why I would like to keep the cache in the ResourceManager -<br><br>1. It makes copying resources very cheap. Arguably one can achieve the same thing by just using QSharedData or just reference counting.<br>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.<br>
<br>I'm not sure how often multiple accesses happen in practise, but it would be unwise to change the Resource's accepted behaviour.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>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...<br><br>Or am I missing some other feature of keeping the Resources in ResourceManager? <br>
</blockquote><div><br>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.<br>
 <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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?<span class="HOEnZb"><font color="#888888"><br>
</font></span></blockquote><div><br>I don't particularly like the idea of the second fix. Sorry.<br><br>I have another solution -<br><br>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.<br>
 <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888"><br>Simeon<br>
</font></span><br>_______________________________________________<br>
Nepomuk mailing list<br>
<a href="mailto:Nepomuk@kde.org">Nepomuk@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/nepomuk" target="_blank">https://mail.kde.org/mailman/listinfo/nepomuk</a><br>
<br></blockquote></div><br><br clear="all"><br>-- <br><span style="color:rgb(192,192,192)">Vishesh Handa</span><br><br>
</div>