Issue 118877

Summary: [From Symphony] Calc crashes when Redo refreshing data
Product: Calc Reporter: Yan Ji <yanji.yj>
Component: uiAssignee: AOO issues mailing list <issues>
Status: CLOSED FIXED QA Contact:
Severity: Critical    
Priority: P2 CC: debin.lei, hdu, jsc, leiw, polo8495
Version: 3.4.0 Beta (OOo)Flags: jsc: 3.4.1_release_blocker+
Target Milestone: 3.4.1   
Hardware: All   
OS: All   
Issue Type: DEFECT Latest Confirmation in: ---
Developer Difficulty: ---
Attachments:
Description Flags
Samples
none
patch for the issue
none
update the patch
none
sample file
none
update the patch none

Description Yan Ji 2012-02-07 02:31:43 UTC
Created attachment 77189 [details]
Samples

1. Open the sample file 
2. Select Data and Sort according to "Standard"
3. Select Data again and Sort according to "Luxury"
4. Change data value, say, change C5 to 9500
5. Select data range and refresh data by "Data->Refresh Range"
6. Undo twice
7. Redo twice

Defect:
Calc crashes at the second redo
Comment 1 Lei Debin 2012-05-11 07:21:33 UTC
The root cause is using an invalid pointer.
The source code is located at main\sc\source\core\data\table3.cxx
In void ScTable::SwapRow(SCROW nRow1, SCROW nRow2) method,
      const ScPatternAttr* pPat1 = GetPattern(nCol, nRow1);
      const ScPatternAttr* pPat2 = GetPattern(nCol, nRow2);
      if (pPat1 != pPat2)
      {
	SetPattern(nCol, nRow1, *pPat2, TRUE);
	SetPattern(nCol, nRow2, *pPat1, TRUE);
      }
sometimes, pPat1 will be deleted in SetPattern(nCol, nRow1, *pPat2, TRUE);(if pPat2 point to the same content as pPat1) So when execute SetPattern(nCol, nRow2, *pPat1, TRUE); it will use a deleted object, crashed !!!

The solution is to check the content refer by the pointer instead of the pointer itself. The ScPatternAttr had a function ==
int __EXPORT ScPatternAttr::operator==( const SfxPoolItem& rCmp ) const
{
    // #i62090# Use quick comparison between ScPatternAttr's ItemSets

    return ( EqualPatternSets( GetItemSet(), static_cast<const ScPatternAttr&>(rCmp).GetItemSet() ) &&
             StrCmp( GetStyleName(), static_cast<const ScPatternAttr&>(rCmp).GetStyleName() ) );
}
Comment 2 Lei Debin 2012-05-11 07:24:51 UTC
Created attachment 77528 [details]
patch for the issue
Comment 3 Lei Debin 2012-05-14 03:01:03 UTC
Created attachment 77540 [details]
update the patch

As many case the the address is the same one, we don't need to check point content at all.
The change will get more performance.
Comment 4 hdu@apache.org 2012-05-15 10:44:58 UTC
Excellent work. Thanks to you both for the easy to reproduce bug description and for devloping and providing the fix! The patch was applied as revision 1338635.
Comment 5 jsc 2012-05-30 14:28:55 UTC
set release blocker flag for 3.4.1
Comment 6 Wang Lei 2012-05-31 10:19:57 UTC
Created attachment 77866 [details]
sample file
Comment 7 Wang Lei 2012-05-31 10:21:26 UTC
The root cause is correct. But the fix has problem.

I can reproduce this problem in following steps:

1)Open attachment sample.ods file
2)Input 8 in cell B4
3)select B4
4)click Sort Ascending button on toolbar
5)Undo twice
6)redo twice

AOO will crash
Comment 8 Wang Lei 2012-05-31 10:22:28 UTC
Reopen it since the fix has problems
Comment 9 Wang Lei 2012-05-31 10:27:21 UTC
According to the root cause, the correct fix should like following:

if (pPat1 != pPat2)
{
	//Add Reference to avoid pPat1 to be deleted by merge same cell attributes for adjacent cells
        if( IsPooledItem( pPat1 ) ) pPat1->AddRef();
	SetPattern(nCol, nRow1, *pPat2, TRUE);
	SetPattern(nCol, nRow2, *pPat1, TRUE);
	if( IsPooledItem( pPat1 ) ) pPat1->ReleaseRef();
}
Comment 10 Lei Debin 2012-06-01 07:00:01 UTC
Created attachment 77886 [details]
update the patch

Here is the patch based on Wang Lei's suggestion, Thx a lot.
Comment 11 hdu@apache.org 2012-06-06 12:42:32 UTC
Thanks for the updated patch! It is now applied as 1346850 for trunk and 1346854 for AOO34.
Comment 12 Terry Yang 2012-06-18 08:37:10 UTC
verified fixed on trunk 1350879
Comment 13 Terry Yang 2012-06-26 03:21:06 UTC
Verify fixed on AOO 3.4.1 Dev snapshot rev.1351960
Suggest close this bug
Comment 14 Wang Lei 2012-06-26 05:20:05 UTC
close it as it is verified on trunk 1350879 and AOO 3.4.1 Dev snapshot rev.1351960