Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fixed: Crash in GUIPicturesWindow caused by PR2890 (fixes #14500) #2998

Merged
merged 1 commit into from Aug 1, 2013

Conversation

Projects
None yet
4 participants
Member

arnova commented Jul 23, 2013

This should fix #14500, haven't tried to reproduce it myself but the problem seems pretty obvious.

Member

arnova commented Jul 23, 2013

@jmarshallnz : Please review. Perhaps there's a better way to handle (ab)uses like this? Perhaps revert SetCachedImage/GetCachedImage to being static again? Or use locks?

Member

jimfcarroll commented Jul 23, 2013

This doesn't look like it will do anything to address the problem. I think we need to serialize access to sqlite.

Member

jmarshallnz commented Jul 23, 2013

I'd serialise access to the shared database object (TextureDatabase) in the loader if we expect public functions to use private members of a threaded object.

Member

jimfcarroll commented Jul 23, 2013

I was going to serialize all sqlite DB calls. Given that we don't know which "mode" sqlite is compiled in (see http://www.sqlite.org/threadsafe.html ), I'm not sure what else to do. Had a brief discussion on IRC. Any other suggestions?

Member

jmarshallnz commented Jul 23, 2013

At the very least we're assuming multithread, right? So one presumes it can't be compiled singlethreaded, so you should be able to choose threadsafe at runtime. Other than mistakes such as this one, we don't (to my knowledge) use the same db object across threads.

Member

jimfcarroll commented Jul 24, 2013

I'm not really sure we can assume anything. Let me know if you think otherwise. On Linux it's just pulled in. The safest thing to do is assume it's compiled "singlethreaded" and account for it in our code. That's what I was going to do. Let me know if you think that makes sense.

Member

jmarshallnz commented Jul 24, 2013

If that were the case, how would we not be crashing all over the place? We use sqlite databases from multiple threads concurrently all the time. We just don't use shared instances (except in places where we have bugs such as this one).

Member

jimfcarroll commented Jul 24, 2013

I don't know but that sound convincing to me. If it's compiled for multi-threaded mode then each connection is supposed to be able to operate independently. I'll make that assumption and put a PR out there.

Member

jimfcarroll commented Jul 24, 2013

This should fix it (or at least come closer): #3000

Member

arnova commented Jul 24, 2013

Like @jmarshallnz said: we use concurrent db access in my other places too so those should trigger crashes then as well. My idea was that this specific crash was caused by accessing the texturedb-member in the m_thumbloader object from both the background-loader thread and GUIWindowPictures that's why I split those to (hopefully) fix this, which in a fact gives them both their own thumbloader-object with their own (pirvate) texturedb-member. @jmarshallnz : Any pointers on how to serialise this properly?

Member

jimfcarroll commented Jul 24, 2013

If PR3000 is taken then that should mean this one wont be needed since it (supposedly) solves the root problem rather than just this one.

Member

arnova commented Jul 24, 2013

@jimfcarroll : Of course, just wanted to make sure you understood the issue/fix in this PR correctly.

Member

jimfcarroll commented Jul 24, 2013

Ah thanks. I do now. :-)

Owner

MartijnKaijser commented Jul 30, 2013

from EricV in trac:

Comment(by EricV):
Tested the temporary fix and indeed I see no more crash. Thanks.

Haven't tested it myself. Hopefully i will get to it tonight.

Member

arnova commented Jul 30, 2013

I think it is better to do it like this anyway. IMO we shouldn't mix threaded and non-threaded uses of the same object: that's asking for trouble. Ideally we should perhaps even seperate the ThumbLoader class somehow so you don't even have the option to do so..

Owner

MartijnKaijser commented Jul 30, 2013

Confirmed working.

Owner

MartijnKaijser commented Jul 31, 2013

Merge this so it fixes the monthly build?

Member

arnova commented Aug 1, 2013

I'd say +1. Dunno what @jmarshallnz thinks about it...

MartijnKaijser added a commit that referenced this pull request Aug 1, 2013

Merge pull request #2998 from arnova/pictures_crash_fix
fixed: Crash in GUIPicturesWindow caused by PR2890 (fixes #14500)

@MartijnKaijser MartijnKaijser merged commit 30f4a79 into xbmc:master Aug 1, 2013

Owner

MartijnKaijser commented Aug 1, 2013

I've merged this as it at least makes xbmc usable again. Perhaps there needs to be a more thorough fix but that will take some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment