> On July 27, 2014, 11:17 a.m., Thomas Lübking wrote: > > drkonqi/gdbhighlighter.cpp, line 74 > > <https://git.reviewboard.kde.org/r/119498/diff/1/?file=293510#file293510line74> > > > > an abort is not a crash ;-) > > > > If you hit this assert, the looked up (lineNr - 1) is somehow out of > > bounds, ie. there's no line with a key >= (lineNr -1) in the map. > > > > This should likely never happen on any system. > > Can you check what the lineNr actually is as compared to > > > > qDebug() << lineNr << lines.keys(); > > > > ? > > Ian Wadham wrote: > I did at one time have a few qDebug() statements to try and find what was > wrong with the algorithm. AFAICT it is trying to match one or more lines in > (const QString& text) with single (possibly long) lines of backtrace in > QMap<int, BacktraceLine>. I think it failed if one backtrace line was > formatted into three text lines or maybe if the last backtrace line was > formatted into two text lines. AFAICT (there is a real dearth of explanatory > comments) the code is merely introducing pretty colours, etc. into the > formatted text. As such, it should not abort Dr Konqi and lose the crash > report. > > Thomas Lübking wrote: > Of course it should not crash. > > The point is that the assert explicitly tells you that there's something > wrong with the code. > Skippping it won't fix the issu - that's like puting duct tape over a > warning sign on your cars dashboard to fix "no cooling water". > > > Since the lines map seems only used to map lines to textblocks, i assume > the issue is that the lines are eg. not "\n" terminated in gdb output on OSX > (no idea, though) and the only item in the map is that for the key "0" > > In this case that'd be the issue to fix. > > Ian Wadham wrote: > Please spare me the motoring analogies. To me, the ASSERT at this point, > is like bringing the car to a screeching halt or running it into the nearest > fence, simply because the glovebox light will not come on. I am a real-time > and O/S programmer from way back and have designed and written a few crash > procedures for different systems. They need to be rugged and simple (unlike > Dr Konqi IMHO) and to succeed no matter what. This used to be called failsafe > programming, graceful failure or graceful degradation. If an ASSERT and abort > technique has to be used in RT programming, to prevent potential database > corruption, it needs to be backed by an appropriate recovery and restart > procedure. > > Actually I think lines.end() is a legitimate return from > lines.lowerBound(lineNr - 1) in Dr K's algorithm (see > http://qt-project.org/doc/qt-5/qmap.html#lowerBound coding examples). The > lineNr can actually get ahead of the number of lines in the map by by too > much, if there are several multi-line entries in QString& text. Using an > ASSERT looks like lazy programming to me (Thinks: "I cannot see what to do in > this case, so I'll put in an ASSERT and see if it actually happens in > practice." ???). My solution is to re-use the last entry from the QMap. > Another might be to use "return;" instead of the ASSERT, leaving the last bit > of the highlighting incomplete, but not losing the valuable backtrace data. > > A better (more rugged) approach might be: > > get a line from "text" > if it is from the left-hand end of a backtrace entry (i.e. not a > continuation line) > get the next backtrace entry > if past last entry > return > re-format the line from "text" > > I would have used something like that in my patch, but I do not know > enough about the format of backtrace data to get the first "if" condition > right. How would I? > > Re "\n" characters, they occur as expected in the content of "QString& > text" on Apple OS X. > > RJVB Bertin wrote: > If it walks and talks like a crash ... we should not end up in the > discussion deadlock I once had with my boss who claimed embedded code cannot > crash (because there's no OS or whatelse to replace the application code). > > Anyway, I like Ian's suggestion to just return to the caller instead of > exitting (and I concur with his lazy programming analysis). > > So if I understood correctly, DrKonqi does some reformatting of the > backtrace before including it for upload with the bug report. What I haven't > understood is how important this reformatting is. Could it be a thought to > cancel the reformating procedure and return with the raw text, instead of > with a partly formated text, when the assert condition triggers? > > > About processing backtraces: recent OS X versions use lldb instead of > gdb. How have you tackled that, Ian - added a dependency on port:gdb ? > > Aaron J. Seigo wrote: > This is not lazy programming but programming by contract. That it reaches > the lasT of lines is a bug... it should nEver happen algorithmic Ally. The > bug appears to be lowerbound sage. It returns the next higher kety and I bet > the orig dev assumed it would return the next lower. So... perhaps che k > forst for lines.contains and if that fails the do lowerbound--. That is just > from a quick read of the code. I am not familiar with this class nor do we > have output from the affected system which would be rather helpful. > > RJVB Bertin wrote: > So the contractor was lazy, too lazy to specify how to handle this kind > of exception? ;) > > I suggested returning the raw text in part because it'd allow to upload > the offending output from affected systems ... > > (PS: Aaron, if you use an Android device for this kind of coding related > activities, you might want to turn off auto-correction and install the > Hacker's keyboard, > https://play.google.com/store/apps/details?id=org.pocketworkstation.pckeyboard) > > Thomas Lübking wrote: > @Aaron, the author seemed aware of the lowerBound behavior, see original > lines 74ff > > > The issue: > ------------- > The problem here is that the lines map is build from incoming strings. > The key indicates the linenumber of that string in \n metrics. > > If the last string starts at line i and has j > 1 lines (in terms of \n) > and the textDocument segments lines [i,i+j] into multiple lines, there's no > key >= i+k, 0 < k < j > > -> Therefore it is required to add a dummy element with the highest "l" > variable after the foreach loop in the constructor, so ::lowerBound will find > this and lines 74ff will lower the iterator. > This is different from assuming the ::end() iterator position to indicate > the post-last (what would be an option) in that it preserves the guarantee of > a sane lineNr. > > > > This is obviously not platform dependent, but a bug. > Asserts are there to detect bugs, throwing errors would probably be > nicer, but troublesome on Qt (not exception safe if you let the exception > escape to the eventloop) > > > On a general note: > ------------------- > An assert on a logic condition is either wrong in general or there's > actually a bug. Therefore it's either removed or the bug fixed. > "Fixing" by ignoring unpredicted conditions on "random" circumstances > (and the platform /is/ random in this regard) is completely unacceptable as > far as I am concerned. > > If a module causes trouble and you're running out of time it's of course > ok to skip the module and schedule the fix, but ignoring a conflict instead > of analyzing it is what /I/ was tought to consider lazy :-P > > PS: > ---- > The duct tape was btw. a reference to the Simpsons - generation gap, i > assume ;-) > > Ian Wadham wrote: > @Aaron: Welcome aboard! I think it must be Akademy 2008 in Belgium since > we last met. Nice to meet you again :-) > I have attached a log file containing (immediately before the ASSERT), > (lineNr - 1), it.key() and an abbreviation of the "text" line being processed. > > @René: Be careful who you are giving advice to... Aaron is leader of the > Plasma project :-) > > @Thomas: My sons used to watch the Simpsons. So did I, but not that > regularly. I was more into Buffy the Vampire Slayer :-) > > The ASSERT failure is definitely when the last backtrace entry has > 1 > line. See the attached file. I think adding a dummy line may be the best > solution. I still do not like using ASSERT and abort and losing the > backtrace. How about doing the same test and adding a warning message to the > outgoing backtrace text if there is any problem? Graceful failure... :-) > > Thomas Lübking wrote: > Usually an assert here would be perfectly fine, since it indicates an > unpredicted (implying "invalid" in this context) state, asserts only apply > when compiling in debug mode and since there's a bug, it's ok to take a core > dump and cause a bugreport. > > In the very specific context of DrKonqi however, i tend to agree that > aborting the crash dialog is better to be avoided in any possible case. > I'd suggest to have a QMessageBox (telling the poor soul it just hit two > bugs for one and how to report the second one) and immediately return in the > particular case. > > This is however up to DrKonqi maintainers (there multiple Q_ASSERT > calls), maybe explicitly attach Milian to this review. > > Ian Wadham wrote: > @René: RE "About processing backtraces: recent OS X versions use lldb > instead of gdb. How have you tackled that, Ian - added a dependency on > port:gdb ?" > > I haven't "tackled" it. Have a heart! But I have just established that > DrKonqi can accept various debuggers via KConfig files (*rc) in > share/apps/drkonqi/debuggers/internal (or .../external). I know nothing about > either lldb or gdb - I never use interactive debuggers as a rule. If they are > sufficiently compatible with each other, maybe someone could write a DrKonqi > config file for lldb. At present, DrKonqi's options are dbx, gdb and > kdbgwinrc (for Windows). Or maybe MacPorts could keep gdb around "forever", > as it does for other things that Apple OS X drops.
@Thomas: Yes, you just redescribed exactly what I did :) If the last line has more than 1 line, then it needs to not pick the NEXT member of lines, which is what lowerBound does, but lowerBound(key)--. Looking at the code, it seems this holds for all multi-line entries. So at least the fix looks easy enough: if it hits the end, then it needs to back the iterator up by one. A multi-line entry in the middle of the output should also be tested just to be certain it works as expected there (I suspect not) - Aaron J. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119498/#review63254 ----------------------------------------------------------- On July 30, 2014, 1:04 p.m., Ian Wadham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119498/ > ----------------------------------------------------------- > > (Updated July 30, 2014, 1:04 p.m.) > > > Review request for KDE Software on Mac OS X, KDE Runtime, kdelibs, and > Michael Pyne. > > > Repository: kde-runtime > > > Description > ------- > > When a KDE app crashes in Apple OS X, it just disappears from the screen. At > the most, the user is invited to report the crash to Apple. AFAIK this has > been a problem in KDE on Apple OS X for years, leading to frustration with > KDE among Apple users and MacPorts developers and an attitude among KDE > developers of "Why does nobody report the problem(s) on bugs.kde.org?" > > It is my strong belief that the failure to report crashes of KDE apps in > Apple OS X also exists in Frameworks. > > So far I have identified a number of portability bugs in KDE on Apple OS X: 1 > in KCrash, 1 in kdeinit4 and 5 in Dr Konqi. Three patches for Dr Konqi are > submitted in this review. Patches for KCrash and kdeinit4 are submitted in > part 1 of this review, against kdelibs. I am still investigating the other > two problems in Dr Konqi - and there could be more than two... > > In this review we have three portability problems: > > 1. On Apple OS X, Dr Konqi's dialog box hides itself underneath the main > window of the app that has just crashed, so is effectively useless. This > appears to be because Dr Konqi is started by a Linux/Unix method (fork() + > exec()?). If an app is started with the Apple OS X "open" command, it always > appears on top. The patch just raises the dialog box. > > 2. When formatting the backtrace output, Dr Konqi crashes (with an ASSERT) on > the last line. This appears to be an error in the algorithm used (i.e. also a > bug in Linux KDE), but the patch is treating it as an Apple OS X portability > problem for now. > > 3. Dr Konqi checks whether the user can save cookies in kcookiejar and, if > not, stops reporting the crash. On Apple OS X, cookies would be kept in > another browser (e.g. Safari or Firefox) and not in KDE's default browser > (Konqueror) and cookie jar. IMHO, Dr K should report the crash no matter > what, as long as it can connect to bugs.kde.org and log in. > > > Diffs > ----- > > drkonqi/reportassistantpages_bugzilla.cpp 86ca327 > drkonqi/gdbhighlighter.cpp 7cd0aa9 > drkonqi/main.cpp 75e060e > > Diff: https://git.reviewboard.kde.org/r/119498/diff/ > > > Testing > ------- > > Using Apple OS X 10.7.5 (Lion) on a MacBook Pro, I have installed KDE libs > via MacPorts (at version 4.12.5) and I have adapted kdesrc-build to run in an > Apple OS X environment and used it to test against the KDE 4.13 branch. I > have been testing with a KDE app that I can crash at will and using stderr > and Apple OS X Console log output to determine the outcome. > > Please note that I am the -only- KDE developer who has this kind of setup, > but I am NOT a KDE core developer. My experience before now has been in KDE > Games. However I used to be a UNIX and database guru before I retired in 1998. > > I NEED HELP from KDE -core- developers to proceed further. These problems > will also exist in Dr Konqi for KF 5, but I am as yet unable to build or test > Frameworks on Apple OS X and I cannot find Dr Konqi among the Frameworks > repositories. I am sure there are many more Apple OS X portability problems > in Dr Konqi and other KDE software. > > Without my patches, Dr Konqi, on Apple OS X, remains invisible to the user, > often fails to complete the backtrace report and then fails to connect to > bugs.kde.org. > > With my patches, Dr Konqi on Apple OS X can generate a full crash report, > including the backtrace and the results of the dialog with the user. > Sometimes, however, it fails to submit the completed report to bugs.kde.org. > This problem is still under investigation. > > I would not have got this far without help from Michael Pyne, Thomas Lübking > and several of the MacPorts developers, as well as the unfailing enthusiasm > and encouragement of my friend Marko Käning. > > > File Attachments > ---------------- > > Log of Dr K ASSERT problem > > https://git.reviewboard.kde.org/media/uploaded/files/2014/07/30/a3f99f00-94df-4b10-bc47-66b1c966f893__DrKonqiASSERT.kcrash.txt > > > Thanks, > > Ian Wadham > >
