[poppler] XRef::fetch mutex lock

Albert Astals Cid aacid at kde.org
Wed Apr 3 10:32:23 PDT 2013 

El Dimecres, 3 d'abril de 2013, a les 08:00:34, Thomas Freitag va escriure:
> Am 03.04.2013 00:01, schrieb Albert Astals Cid:
> > El Dimarts, 2 d'abril de 2013, a les 10:36:53, Thomas Freitag va escriure:
> >> Am 02.04.2013 09:22, schrieb Thomas Freitag:
> >>> Hi Albert!
> >>> 
> >>> Am 27.03.2013 12:29, schrieb Thomas Freitag:
> >>>> Sorry, pushed the wrong button, here my answer to the list:
> >>>> 
> >>>> Am 27.03.2013 11:41, schrieb Albert Astals Cid:
> >>>>> Why do we pass around the recursion integer around?
> >>>> 
> >>>> No idea: The recursive integer wasn't introduced by the thread safe
> >>>> patch, it was already there. I just used it and extend some functions
> >>>> which incorrectly ignore that parameter instead of passing it to the
> >>>> called functions. Probably I missed some more than only the one in
> >>>> DCTStream::DCTStream.
> >>> 
> >>> I just ran a regression test over my PDF files, but I didn't get any
> >>> dead locks. So if You want me to solve that problem, You should
> >>> provide me that PDF. (Regardless if You prefer to use recursive mutex:
> >>> the endless loop detection need to pass the recursive integer
> >>> everywhere!)
> >> 
> >> Attached a patch which corrects it. I made it in my sleep, 'cause the
> >> dead lock was probably caused by my patch for bug 61994 (even if that
> >> PDF doesn't cause any dead locks). This time I also respect the
> >> DCTStream usage if LIBJPEG is disabled :-)
> > 
> > Sure, this fixes the issue (and we should commit it to help with the
> > infinite recursion issue), but what's your opinion on making our mutexes
> > recursive?
> I think, a quite good discussion about recursive and non recursive mutes
> was here:
> http://stackoverflow.com/questions/187761/recursive-lock-mutex-vs-non-recurs
> ive-lock-mutex. This convinced me to vote for a usage of recursive mutex in
> poppler: 1. xpdf and therfore poppler wasn't really designed for the usage
> of threads. We have it now working, but if we cherish on the usage of non
> recursive mutex we will always run into dead lock problems with futur
> patches again, so testing patches will become more expensive.
> 2. Since poppler nowhere uses emergency exits with "throw", we don't
> have the problem that we have to keep track that a thread releases a
> mutex so often it aquires it.
> 3. The only (small) problem I see is the sentence "Not all systems
> supporting pthreads also support recursive mutexes, but if they want to
> be POSIX conform, they have to
> <http://www.opengroup.org/onlinepubs/009695399/functions/pthread_mutexattr_g
> ettype.html>."

To be honest i don't think we should care much about the minority of systems 
there that don't support recursive mutexes.

Cheers,
  Albert

> 
> Cheers,
> Thomas
> 
> > It should help us making the code simpler since we could just drop all the
> > DoNotLockMutex cases, no?
> > 
> > Cheers,
> > 
> >    Albert
> >> 
> >> Cheers,
> >> Thomas
> >> 
> >>> Cheers,
> >>> Thomas
> >>> 
> >>>>> Wouldn't a real recursive mutex be enough?
> >>>> 
> >>>> No idea: I used in the thread safe patch just the pthread_lock which
> >>>> was defined. I'm not knowing what happens when we change it globally
> >>>> i.e. if it affects other usages of the GooMutex. Just feel free to
> >>>> change it, You should have the better overview of usages than I have.
> >>>> 
> >>>> Cheers,
> >>>> Thomas
> >>>> 
> >>>>> I'm asking because i have a document here (that sadly i can't share)
> >>>>> that is deadlocking
> >>>>> itself there because we are not passing the recursion integer
> >>>>> everywhere (we lose it in DCTStream::DCTStream)
> >>>>> as shown in this backtrace
> >>>>> 
> >>>>> #0  __lll_lock_wait () at
> >>>>> ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> >>>>> #1  0x00007ffff599217c in _L_lock_982 () from
> >>>>> /lib/x86_64-linux-gnu/libpthread.so.0
> >>>>> #2  0x00007ffff5991fcb in __GI___pthread_mutex_lock (mutex=0x71dc10)
> >>>>> at pthread_mutex_lock.c:64
> >>>>> #3  0x00007ffff5f02f73 in MutexLocker::MutexLocker
> >>>>> (this=0x7fffffffcfb0, mutexA=0x71dc10, modeA=DoLockMutex) at
> >>>>> /home/tsdgeos/devel/poppler/goo/GooMutex.h:72
> >>>>> #4  0x00007ffff5fac8e2 in XRef::fetch (this=0x71db50, num=5, gen=0,
> >>>>> obj=0x7fffffffd0b0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1137
> >>>>> #5  0x00007ffff5f8a013 in Object::fetch (this=0x71fe08,
> >>>>> xref=0x71db50, obj=0x7fffffffd0b0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122
> >>>>> #6  0x00007ffff5f19de4 in Dict::lookup (this=0x71fb60,
> >>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256
> >>>>> #7  0x00007ffff5f032e9 in Object::dictLookup (this=0x7fffffffd5a0,
> >>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315
> >>>>> #8  0x00007ffff601f8a3 in DCTStream::DCTStream (this=0x7832b0,
> >>>>> strA=0x720780, colorXformA=-1, dict=0x7fffffffd5a0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/DCTStream.cc:72
> >>>>> #9  0x00007ffff5f9bf6d in Stream::makeFilter (this=0x720780,
> >>>>> name=0x71fef0 "DCTDecode", str=0x720780, params=0x7fffffffd1e0,
> >>>>> recursion=1, dict=0x7fffffffd5a0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:311
> >>>>> #10 0x00007ffff5f9b71a in Stream::addFilters (this=0x720780,
> >>>>> dict=0x7fffffffd5a0, recursion=1) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:184
> >>>>> #11 0x00007ffff5f9204b in Parser::makeStream (this=0x71fcd0,
> >>>>> dict=0x7fffffffd5a0, fileKey=0x0, encAlgorithm=cryptRC4,
> >>>>> keyLength=1146103040, objNum=3, objGen=0, recursion=1, strict=false)
> >>>>> at /home/tsdgeos/devel/poppler/poppler/Parser.cc:280
> >>>>> #12 0x00007ffff5f918e6 in Parser::getObj (this=0x71fcd0,
> >>>>> obj=0x7fffffffd5a0, simpleOnly=false, fileKey=0x0,
> >>>>> encAlgorithm=cryptRC4, keyLength=1146103040, objNum=3, objGen=0,
> >>>>> recursion=0, strict=false) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Parser.cc:131
> >>>>> #13 0x00007ffff5facdd3 in XRef::fetch (this=0x71db50, num=3, gen=0,
> >>>>> obj=0x7fffffffd5a0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1197
> >>>>> #14 0x00007ffff5f8a013 in Object::fetch (this=0x71e128,
> >>>>> xref=0x71db50, obj=0x7fffffffd5a0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122
> >>>>> #15 0x00007ffff5f19de4 in Dict::lookup (this=0x71e660, key=0x7206e0
> >>>>> "Im1", obj=0x7fffffffd5a0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256
> >>>>> #16 0x00007ffff5f032e9 in Object::dictLookup (this=0x71e598,
> >>>>> key=0x7206e0 "Im1", obj=0x7fffffffd5a0, recursion=0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315
> >>>>> #17 0x00007ffff5f2a647 in GfxResources::lookupXObject
> >>>>> (this=0x71e590, name=0x7206e0 "Im1", obj=0x7fffffffd5a0) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:411
> >>>>> #18 0x00007ffff5f3f41e in Gfx::opXObject (this=0x71e440,
> >>>>> args=0x7fffffffd720, numArgs=1) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:4114
> >>>>> #19 0x00007ffff5f2bfd6 in Gfx::execOp (this=0x71e440,
> >>>>> cmd=0x7fffffffd6e0, args=0x7fffffffd720, numArgs=1) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:858
> >>>>> #20 0x00007ffff5f2b82b in Gfx::go (this=0x71e440, topLevel=true) at
> >>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:717
> >>>>> 
> >>>>> 
> >>>>> Cheers,
> >>>>> 
> >>>>>     Albert
> >>>>> 
> >>>>> _______________________________________________
> >>>>> poppler mailing list
> >>>>> poppler at lists.freedesktop.org
> >>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>> 
> >>>>> .
> >>>> 
> >>>> _______________________________________________
> >>>> poppler mailing list
> >>>> poppler at lists.freedesktop.org
> >>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>> 
> >>>> .
> >>> 
> >>> _______________________________________________
> >>> poppler mailing list
> >>> poppler at lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>> 
> >>> .
> > 
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler
> > 
> > .

More information about the poppler mailing list