Re: Asynchronous structured exception handling (SEH) in TortoiseProc (/EHa)

On 19 February 2016 at 20:48, Stefan Kueng <[email protected]> wrote:
> On 19.02.2016 14:20, Ivan Zhakov wrote:
>>
>> On 17 February 2016 at 21:59, Stefan Küng <[email protected]> wrote:
>>>
>>> On 17.02.2016 11:57, Ivan Zhakov wrote:
>>>>
>>>> I'm currently investigating crashes in TortoiseProc like the one in
>>>> drdump problem 175614 [1].
>>>>
>>>> In all cases the stacktrace looks like this:
>>>> [[[
>>>>>
>>>>>      libapr_tsvn.dll!apr_pool_destroy(apr_pool_t * pool) Line 814    C
>>>>
>>>>        libapr_tsvn.dll!apr_pool_destroy(apr_pool_t * pool) Line 811    C
>>>>        [...]
>>>>        libapr_tsvn.dll!apr_pool_destroy(apr_pool_t * pool) Line 811    C
>>>>        VCRUNTIME140.dll!_CallSettingFrame() Line 50    Unknown
>>>>        VCRUNTIME140.dll!__FrameUnwindToState() Line 1094    C++
>>>>        VCRUNTIME140.dll!__InternalCxxFrameHandler() Line 393    C++
>>>>        VCRUNTIME140.dll!__CxxFrameHandler3() Line 215    C++
>>>>        ntdll.dll!RtlpExecuteHandlerForUnwind ()    Unknown
>>>>        ntdll.dll!RtlUnwindEx()    Unknown
>>>>        ntdll.dll!__C_specific_handler ()    Unknown
>>>>        ntdll.dll!RtlpExecuteHandlerForException ()    Unknown
>>>>        ntdll.dll!RtlDispatchException()    Unknown
>>>>        ntdll.dll!KiUserExceptionDispatch ()    Unknown
>>>>        libapr_tsvn.dll!allocator_alloc(apr_allocator_t * allocator,
>>>> unsigned __int64 in_size) Line 333    C
>>>>        libsvn_tsvn.dll!serf_bucket_mem_alloc() Line 216    C
>>>>        libsvn_tsvn.dll!serf_bucket_headers_setx() Line 103    C
>>>>        [...]
>>>>        libsvn_tsvn.dll!svn_client_update4() Line 722    C
>>>>        TortoiseProc.exe!SVN::Update() Line 404    C++
>>>> ]]]]
>>>>
[...]
>>> We use catch(...) in some places, that's why /EHa is enabled.
>>> Yes, it can be dangerous. But especially with the log cache it's better
>>> to catch the exception and then remove the corrupted cache than to crash
>>> over and over.
>>
>> Well, I see: that current log cache uses direct access to memory
>> mapped file with bounds checking etc. I could review the code and add
>> necessary checks to convert Access Violation errors to normal C++
>> exceptions, but I wonder why TortoiseSVN uses custom disk storage for
>> log caches instead of something like SQLite? Subversion libraries
>> expose nice API for working with sqlite databases.
>
>
> the log cache is optimized for speed. For example, you can show the log for
> the whole KDE or Apache repository (including all subproject). Then show the
> log again and you get *all* entries within about two/three seconds.
> But for more details, you have to ask Stefan Fuhrmann ;)

SQLite is very performant when there's little concurrency (as it is
with the log caching). I wonder, what was the reason of not using it
for this purpose and re-implementing everything manually.

>
>>> Also, for the shell extension using catch(...) is the only way to
>>> prevent the desktop process from crashing - too many buggy extensions
>>> are out there which can interfere with TSVN, and it's always TSVN that
>>> gets the blame...
>>
>> I'm currently focused on TortoiseProc (and maybe TSVNCache). I
>> understand that shell extension may require different handling and we
>> should try everything to avoid crashing explorer.exe.
>
>
> Yes. At least for all the status-fetching code.
>
Btw we started to convert some abort() calls in status-fetching code
to normal errors in Subversion.

>> I may suggest the following:
>> 1. I'll disable asynchronous SEH exception handling in TortoiseProc.exe
>> 2. I'll start reviewing/rewriting log cache storage not to crash in
>> case of corrupted database
>> 3. We backport change (1) to 1.9.x.  After that I'll investigate
>> drdump crashes related to the log cache storage and convert them to
>> C++ exceptions.
>>
>> How does it sound?
>
>
> That sounds good. If you have the time to do it, then please go ahead!
>

It took a little bit longer than I expected but as of r27210 log cache
should gracefully handle most common database corruptions. There are
still some cases when TortoiseSVN trunk will crash with a corrupted
log cache, but TortoiseSVN 1.9.3 may also crash in these cases. I'm
going to fix these edge cases later.

I disabled asynchronous structured exception handling (SEH) in
TortoiseProc in r27211.

What do you think about backporting these changes to 1.9.x? Changes
are not small, but they fix real crashes.

-- 
Ivan Zhakov

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3162819

To unsubscribe from this discussion, e-mail: [[email protected]].