[KDE/Mac] Review Request 120150: [OS X] prevent another crash after finishing a code difference review
René J.V. Bertin
rjvbertin at gmail.com
Fri Sep 12 12:59:20 UTC 2014
> On Sept. 12, 2014, 12:59 a.m., Milian Wolff wrote:
> > No really, this just shows that something else is wrong here. Maybe fixed the bug in Qt then. But doing this kind of hit-and-run patching, blindly trying to workaround a bug is really bad as it greatly increases the maintenance cost.
> >
> > Could you please ensure that no threads are involved? Add stuff like Q_ASSERT(QThread::currentThread() == thread()) to the dtor of the class.
> > If that does not help, then certainly there is a bug elsewhere, maybe inside Qt, and should be fixed there then.
>
> René J.V. Bertin wrote:
> I take it you're talking about the descructor for the classes I use `deleteLater` on in this 2nd patch?
>
> Is there a Qt call to check if an object has pending events or (better yet?) flush events for an objects fromt the queue?
>
> Milian Wolff wrote:
> Yes, and also in the first patch which should be reverted, I guess.
>
> And Qt removes pending events from the event queue when an object is deleted. *Only* when threads are involved there might be race conditions. But here, there should not be any threads.
>
> René J.V. Bertin wrote:
> We keep circling around this issue about pending events. Frankly, I shouldn't have asked about detecting or removing them, because my whole previous experience suggests that this is just not doable on OS X. That, and/or it's impossible to avoid getting what I'd recursive events, meaning you get in an event handler when in any number of previous/higher stack frames you're also in an event handler for that same object. Those are pending events too, after all - they haven't been handled and so theoretically at least they're still on the queue. And of course you cannot pop events that are already being handled.
>
> So what would you do? Check after every function call that might have resulted in event processing if your object is still valid, while there is no feature in C/C++ to check if a pointer points to valid memory? Also, remember how I said ObjC doesn't delete objects immediately when they're deleted, but instead leaves them in the NSAutoReleasePool *that goes with the current event loop*? The crash I've been seeing occurs in a function *that gets called from _unavoidable_ ObjC code*. And it's quite possible btw that the code that ought to do this "is this pointer still alive" check is part of that ObjC code.
>
> I really think we shouldn't be second-guessing the Qt authors and their knowledge of their own product. If they put a warning about preferring `deleteLater` when there are or might be pending events, it is because it's somehow impossible (reliably) to remove pending events for that object from the queue while deleting it. I'm tempted to add that `deleteLater` basically schedules the delete for after the exit of the current event loop - and that loop probably isn't so different from that other "loop", the one concerning threads, that is not managed by Qt ...
>
> I will do the thread checking, and I indeed planned to revert my earlier patch for that. We'll see what that teaches me. That aside, I suppose I'll wrap my modifications in Q_OS_MAC tags . I frankly don't see the problem doing a delete this way or that way, and I guess the Qt authors also would have preferred to use a direct rather than a deferred delete. But sometimes you just have to go with what you have, whether or not that leaves you wondering about whether you're dealing with design bugs or features ...
>
> BTW, I've asked about this on stackoverflow, no answer. And the Qt forum tells me it's in maintenance as soon as I log in, so that place is apparently off limits to me.
When is the last time you did a run through valgrind or with a malloc guard of the kde4 version, and specifically of production code (i.e. built with something like -O3 -g)?
I started kdevelop in gdb, with MallocGuardEdges=1 MallocScribble=1 MallocErrorAbort=1 MallocCheckHeapStart=1000 in the environment, and got this:
kdevelop(49685)/kdevelop (cpp support) Cpp::instantiateDeclarationAndContext: Resolved bad base-class "QMap< Key, T >" "QMap< Key, T >"
kdevelop.bin(49685,0x7fff70affcc0) malloc: at szone_check counter=120000
kdevelop.bin(49685,0x7fff70affcc0) malloc: at szone_check counter=130000
kdevelop.bin(49685,0x13f781000) malloc: at szone_check counter=140000
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
[Switching to process 49685]
KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, true, 0u, 1048576u>::dynamicItemFromIndexSimple (this=0x124fb9000, index=1413021696) at duchain/repositories/itemrepository.h:855
855 if(m_mappedData == m_data) {
(gdb) p m_data
No symbol "m_data" in current context.
(gdb) p m_mappedData
No symbol "m_mappedData" in current context.
(gdb) up
#1 KDevelop::IndexedQualifiedIdentifier::~IndexedQualifiedIdentifier (this=<value temporarily unavailable, due to optimizations>) at duchain/referencecounting.h:100
100 --ref;
(gdb) p ref
No symbol "ref" in current context.
(gdb) down
#0 KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, true, 0u, 1048576u>::dynamicItemFromIndexSimple (this=0x124fb9000, index=1413021696) at duchain/repositories/itemrepository.h:855
855 if(m_mappedData == m_data) {
(gdb) l
850 }
851
852 private:
853
854 void makeDataPrivate() {
855 if(m_mappedData == m_data) {
856 short unsigned int* oldObjectMap = m_objectMap;
857 short unsigned int* oldNextBucketHash = m_nextBucketHash;
858
859 m_data = new char[ItemRepositoryBucketSize + m_monsterBucketExtent * DataSize];
(gdb) bt
#0 KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, true, 0u, 1048576u>::dynamicItemFromIndexSimple (this=0x124fb9000, index=1413021696) at duchain/repositories/itemrepository.h:855
#1 0x00000001044c6934 in KDevelop::IndexedQualifiedIdentifier::~IndexedQualifiedIdentifier (this=<value temporarily unavailable, due to optimizations>) at /Volumes/Debian/MP6/var/macports/build/_Volumes_Debian_MP6_site-ports_kde_kdevplatform-git/kdevplatform-git/work/kdevplatform-git-1.7.60/language/duchain/identifier.cpp:1564
Previous frame inner to this frame (gdb could not unwind past this frame)
Sadly I don't know what file was being parsed, but it's clear this is most likely unrelated to the issue at hand and it also prevents me from using this technique to (maybe) pinpoint the exact location of our potential bug ...
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120150/#review66302
-----------------------------------------------------------
On Sept. 12, 2014, 1:28 p.m., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120150/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2014, 1:28 p.m.)
>
>
> Review request for KDE Software on Mac OS X, KDevelop and Olivier Goffart.
>
>
> Repository: kdevplatform
>
>
> Description
> -------
>
> In https://reviewboard.kde.org/r/120081/ I proposed an (accepted) approach to prevent kdevelop from crashing after closing the patch review ("git/show differences") toolview. I had another of those crashes after heavier-than-usual perusal of the toolview, despite the previous patch. The attached patch replaces all `delete`s of `QObject` derived class instances with `deleteLater()`.
>
>
> Diffs
> -----
>
> language/CMakeLists.txt 3c790a4
> plugins/patchreview/patchreview.cpp 18b63db
>
> Diff: https://git.reviewboard.kde.org/r/120150/diff/
>
>
> Testing
> -------
>
> kdevplatform git/kde4-legacy on KDE/MacPorts OS X 10.6.8 .
>
>
> Thanks,
>
> René J.V. Bertin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20140912/fae96ae3/attachment.html>
More information about the kde-mac
mailing list