#1381 TortoiseShell crashes with many cores

[Arthur Barrett]

The TortoiseShell shipped with the current CVS Suite is based on TortoiseCVS 1.10. It includes a crash handler that sends crash dumps to our server. We regularly see crash dumps that show that g_Locale->GetString(str); is called by two threads simultaneously. This should be impossible because of the call to g_csLocale.Enter();

We've found this fix:

In SyncUtils.cpp:

CriticalSection::CriticalSection()
{

ifdef MARCH_HARE_BUILD

m_Inside=false;

//InitializeCriticalSection(&m_CS);
if (!InitializeCriticalSectionAndSpinCount(&m_CS,0x80000400))
return;

else

InitializeCriticalSection(&m_CS);

endif

};

In ShellExt.cpp:
// Get translated string
const wxChar GetString(const wxChar str)
{
const wxChar* result = str;
g_MiniDumper->setstring(str);
ASSERT(g_Locale);
// wxLocale.GetString() normally complains if translation is not available,
// we don't want it
if ((str!=NULL)&&(g_Locale!=NULL))
{
CSHelper csh(g_csLocale, true);
wxLogNull logNo;
//g_csLocale.Enter();
result = g_Locale->GetString(str);
csh.Leave();
//g_csLocale.Leave();
}
return result;
}

Now TortoiseCVS 12.5 uses g_msgCatalog->GetString(str); - but otherwise the patch may be helpful.

Microsoft's notes about the effect of InitializeCriticalSectionAndSpinCount() on multiprocessor systems is here:
http://msdn.microsoft.com/en-us/library/ms683476(v=vs.85).aspx

The customer who reported this originally reported it on the TortoiseCVS newsgroup as a bug in 12.5 - but oddly only for non-Admin (non-installer) users. So that bug may be completely unrelated. But when they installed CVS Suite 2009 - this is the bug we found... Customer was using Windows 7 x64 with a quad-core CPU.

[Torsten Martinsen]

Arthur,

I actually don't believe that this patch is correct:

1) At work, I use TortoiseCVS 1.12.5 on a quad-core box, under Win7 x64. Never seen a crash.

2) There is no difference between InitializeCriticalSectionAndSpinCount() and InitializeCriticalSection() with regard to preventing simultaneous access to a resource - the difference is purely related to performance.

[Arthur Barrett]

I completely agree that this is an impossible problem and an impossible fix.

We regularly see crash dumps that show that g_Locale->GetString(str); is called by two threads simultaneously. This should be impossible because of the call to g_csLocale.Enter();

I also forgot to mention that some of Ashtu's crash logs also showed g_Locale as NULL at the call to g_Locale.GetString() - which should also be impossible.

I think by definition - impossible bugs require impossible solutions ;)

We have thousands of downloads of CVS Suite every week, but only a handfull of these crash dumps - which indicates that the problem is not widespread, but it is also not rare.

The problem Ashtu experienced in 12.5 may be completely unrelated to this - since that bug only appeared when TortoiseShell was used by a non-admin user (who did not install the software). However TortoiseShell is very very reliable, so I wouldn't think there would be too many crash bugs in it.

Deciding on what your policy should be about putting fixes in code when the fix is not well understood is a genuinely difficult problem. In the 'community' codeline we always avoided this, but in the 'commercial' codeline we are a lot more pragmatic - we need to have solutions that work for customers - whether they are understood or not - if it gets the customer working then it's a good solution.

I wanted to document our solution here, so that it's clearly publicly documented - if Ashtu or anyone else wants to put this change into 12.5 then he is of course able to download the 12.5 sources and apply the patch and build/test himself.