Issue 118877 - [From Symphony] Calc crashes when Redo refreshing data
Summary: [From Symphony] Calc crashes when Redo refreshing data
Status: CLOSED FIXED
Alias: None
Product: Calc
Classification: Application
Component: ui (show other issues)
Version: 3.4.0 Beta (OOo)
Hardware: All All
: P2 Critical (vote)
Target Milestone: 3.4.1
Assignee: AOO issues mailing list
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 02:31 UTC by Yan Ji
Modified: 2012-06-26 05:20 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation in: ---
Developer Difficulty: ---
jsc: 3.4.1_release_blocker+

Attachments
Samples (9.12 KB, application/vnd.oasis.opendocument.spreadsheet)
2012-02-07 02:31 UTC, Yan Ji 		no flags Details
patch for the issue (808 bytes, patch)
2012-05-11 07:24 UTC, Lei Debin 		no flags Details | Diff
update the patch (1014 bytes, patch)
2012-05-14 03:01 UTC, Lei Debin 		no flags Details | Diff
sample file (7.49 KB, application/vnd.oasis.opendocument.spreadsheet)
2012-05-31 10:19 UTC, Wang Lei 		no flags Details
update the patch (1.92 KB, patch)
2012-06-01 07:00 UTC, Lei Debin 		no flags Details | Diff
Show Obsolete (1) Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this issue.
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