[fox88]

It was a rare event, but recently eMule crashed twice in a week; and I decided to take a look.

Everything was usual: clicked Disconnect, then clicked X system button at the top right corner, answered Yes, and in a short while got an offer to create a crash dump.
The crash dump points at the line 230 in MuleListCtrl.cpp
piArray[iCurrent] = piArray[iCurrent + 1];
in void CMuleListCtrl::ShowColumn(int iColumn)

That function was called from void CMuleListCtrl::SaveSettings()

My guess was that there is no need to call ShowColumn when application is exiting.

For that I made a small change in SaveSettings() - added application state checking.
The method now is like this:

void CMuleListCtrl::SaveSettings()
{
	ASSERT(!m_Name.IsEmpty());
	ASSERT(GetHeaderCtrl()->GetItemCount() == m_iColumnsTracked);

	if (m_Name.IsEmpty() || GetHeaderCtrl()->GetItemCount() != m_iColumnsTracked)
		return;

	CIni ini(thePrefs.GetConfigFile(), _T("ListControlSetup"));
	ShowWindow(SW_HIDE);
	int* piSortHist  = new int[MAX_SORTORDERHISTORY];
	int i=0;
	POSITION pos1, pos2;
	for (pos1 = m_liSortHistory.GetHeadPosition();( pos2 = pos1 ) != NULL;)
	{
		m_liSortHistory.GetNext(pos1);
		piSortHist[i++]=m_liSortHistory.GetAt(pos2)+1;
	}
	ini.SerGet(false, piSortHist, i, m_Name + _T("SortHistory"));
	// store additional settings
	ini.WriteInt(m_Name + _T("TableSortItem"), GetSortItem());
	ini.WriteInt(m_Name + _T("TableSortAscending"), GetSortType(m_atSortArrow));
	if (theApp.m_app_state == APP_STATE_RUNNING) { //+++ only when running
		int* piColWidths = new int[m_iColumnsTracked];
		int* piColHidden = new int[m_iColumnsTracked];
		INT *piColOrders = new INT[m_iColumnsTracked];
		for(i = 0; i < m_iColumnsTracked; i++)
		{
			piColWidths[i] = GetColumnWidth(i);
			piColHidden[i] = IsColumnHidden(i);
			ShowColumn(i);
		}
		GetHeaderCtrl()->GetOrderArray(piColOrders, m_iColumnsTracked);
		ini.SerGet(false, piColWidths, m_iColumnsTracked, m_Name + _T("ColumnWidths"));
		ini.SerGet(false, piColHidden, m_iColumnsTracked, m_Name + _T("ColumnHidden"));
		ini.SerGet(false, piColOrders, m_iColumnsTracked, m_Name + _T("ColumnOrders"));

		for(i = 0; i < m_iColumnsTracked; i++)
			if (piColHidden[i]==1)
				HideColumn(i);
	
		ShowWindow(SW_SHOW);

		delete[] piColOrders;
		delete[] piColHidden;
		delete[] piColWidths;
	}
	delete[] piSortHist;
}

I will keep watching how the fix works, but so far it was fine.

Probably application wraps up slightly faster now, though I did not try to measure that.

Edit. Unfortunatly, that was too simple: maybe it solves crash problem, but causes reordering of columns in lists. I'll take a closer look at tHeWiZaRdOfDoS's variant.

This post has been edited by fox88: 17 April 2013 - 06:19 AM

[tHeWiZaRdOfDoS]

Old thread, probably the same issue: http://forum.emule-p...rl&fromsearch=1

[fox88]

tHeWiZaRdOfDoS, on 15 April 2013 - 08:03 PM, said:

Old thread, probably the same issue

Even the line number is the same.
It could be the same, but I don't think my eMule ever crashed at that location during normal operation. Only on closing, when destructors were working and it could be risky to access objects using old pointers. Besides, there is no need to update GUI at the time.

[tHeWiZaRdOfDoS]

Yes, AFAIR the issue was encountered either during startup or during shutdown, anyways, it's time to fix this

[fox88]

Side effects found. Added that to the first post.

[fox88]

//for(; m_aColumns[iColumn].iLocation > m_aColumns[pHeaderCtrl->OrderToIndex(iCurrent + 1)].iLocation &&
// iCurrent < m_iColumnsTracked - 1; ++iCurrent)
for(; iCurrent < m_iColumnsTracked - 1 &&
m_aColumns[iColumn].iLocation > m_aColumns[pHeaderCtrl->OrderToIndex(iCurrent + 1)].iLocation; ++iCurrent)

tHeWiZaRdOfDoS, I've got a question.
The crash ocurs in the next assignment operation, not in the loop statement itself.
Is that really necessary to reorder checks in loop condition and replace postfix increment with the prefix one?
(Original eMule code uses postfix increment).

This post has been edited by fox88: 10 May 2013 - 01:46 PM

[tHeWiZaRdOfDoS]

IMHO, yes, because if iCurrent is >= m_iColumnsTracked, you may crash @ m_aColumns[pHeaderCtrl->OrderToIndex(iCurrent + 1)].iLocation already - switching the sequence of the checks prevents that.

[fox88]

tHeWiZaRdOfDoS, on 10 May 2013 - 05:37 PM, said:

IMHO, yes, because if iCurrent is >= m_iColumnsTracked, you may crash @ m_aColumns[pHeaderCtrl->OrderToIndex(iCurrent + 1)].iLocation already - switching the sequence of the checks prevents that.

Thanks.
The final iCurrent value could be <= m_iColumnsTracked-1.
Although eMule never crashed here, I probably agree that it would be safer.
To be extra safe we could reorder checks in the previous loop's condition too: integer comparison before function call.

This post has been edited by fox88: 10 May 2013 - 05:32 PM

[Enig123]

fox88,

You seemed to forget to mention if emule stopped crashing without further side-effect after applied tHeWiZaRdOfDoS's patch.

[fox88]

Enig123, on 11 May 2013 - 09:39 AM, said:

You seemed to forget to mention if emule stopped crashing without further side-effect after applied tHeWiZaRdOfDoS's patch.

What do you mean?
The tHeWiZaRdOfDoS's logic seems to be correct.
I applied tha patch only yesterday.

The crash was a rare case even before the patch.
If you know how to reproduce the crash oonditions, please tell, it would simplify the check.
Otherwise I can only watch and wait.