[Libreoffice-commits] core.git: coverity#1323754 we apparently can survive std::abort for a while

[Stephan Bergmann]

On 09/11/2015 12:34 PM, Michael Meeks wrote:
> 	Good question; it may well be guaranteed - but - seemingly I saw this
> code-path continue; perhaps this is an artifact of the debugger under
> windows:
>
> 	https://msdn.microsoft.com/en-us/library/k089yyh0.aspx
>
> 	has some more details; but I'd swear to not having pressed ignore in my
> cases either so ... ;->
>
>>    So, like Coverity, I fail to
>> see how that line can ever be reached (and bAbortFired, of automatic
>> storage during in OpenGLWatchdogThread::execute, ever be true).
>
> 	=) well, me too - was gob-smacked etc. of course, in the ideal world
> this is true; perhaps I was just gotcha'd by the debugging environment.

But I doubt we want to make our code base more capricious than 
necessary, to shield us from behavior exhibited by the Windows debugging 
environment.

> 	Then again - during our abort handling - we spend a lot of time
> creating GUI dialogs and so on on the main thread (which is by now this
> one) - that could easily also wedge / lock-up ;-) that's particularly
> true wrt. the problem of getting the solar-mutex; my hope is that the
> abort handler is good with dropping that.
>
> 	Which thread would you expect the signal to be delivered to (I wonder)
> - it's all a bit interesting I suspect.

The case should be pretty clear for a synchronous, std::abort-generated 
SIGABRT (hopefully even on Windows).

> 	My hope was that the watchdog would carry on working in these cases &
> kill us again more aggressively if necessary if people insist on
> ignoring these guys.

But how should it do that?  Even if the SIGABRT-handling were done on 
another thread, the watchdog thread just couldn't progress past the 
std::abort() (notwithstanding cheating in a debugging environment).

>> really wanted to do is make bAbortFired static, and set it to true
>> /before/ calling std::abort()?
>
> 	I guess we could launch another watchdog thread in this case (if indeed
> we believe the that std::abort never returns ;-) in which case making
> that static would be useful indeed. Would love to see a patch like that.

So there's only a single instance of the watchdog thread supposed to 
ever run.  The odd "static bool bFired" in OpenGLWatchdogThrad::execute 
had fooled me to assume otherwise (for why else should the variable have 
static storage duration).

Anyway, generalizing that "watchdog the OpenGLWatchdogThread, in case 
our signal handler gets stuck" idea obviously leads to a "watchdog our 
signal handler, in case it gets stuck" feature, i.e., spawn a thread 
early in our signal handler (assuming spawning an additional thread 
doesn't make our violation of what a signal handler is supposed to be 
allowed to do any worse), which will call _exit after a fixed amount of 
time.  The question just is, what is a reasonable value for that amount 
of time.  Make it too short, and you'll prevent recovery of documents 
that take long to save and for which our document recovery would 
otherwise have happened to work fine.

And the true route ahead of course is to no longer put our document 
recovery strategy at the mercy of a brittle, undefined-behavior--riddled 
signal handler.