Bug 132454 - kspread crash when opening a large opendocument spreadsheet
Summary: kspread crash when opening a large opendocument spreadsheet
Status: RESOLVED FIXED
Alias: None
Product: calligrasheets
Classification: Applications
Component: opendocument (show other bugs)
Version: 1.6.1
Platform: unspecified Linux
: HI crash
Target Milestone: ---
Assignee: Calligra Sheets (KSpread) Bugs
URL:
Keywords:
: 139920 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-08-15 18:54 UTC by Mjules
Modified: 2007-08-15 16:55 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
crash file (229.36 KB, application/octet-stream)
2006-08-15 18:55 UTC, Mjules
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mjules 2006-08-15 18:54:32 UTC
Version:           1.5.1 (using KDE KDE 3.5.3)
Installed from:    Unlisted Binary Package
Compiler:          gcc 4.0.1 
OS:                Linux

When I try to open a large opendocument spreadsheet (a lot of formula) with kspread, it hung all memory and CPU before crashing.

Openoffice 2.0.3 open this file without problem.

I'm using Mandriva 2006 x86_64 with koffice 1.5.1 from http://www.seerofsouls.com/
Comment 1 Mjules 2006-08-15 18:55:17 UTC
Created attachment 17381 [details]
crash file
Comment 2 Sebastian Sauer 2006-08-19 12:08:14 UTC
Bug confirmed.

Some investigations show, that KSpread first reads the file and once done (last lines printed on console are "kspread: *** Loading cell properties ***** at 14,1175\nkspread:  table:style-name: ce8"), it looks as KSpread freezes. Attaching gdb shows that KSpread spends it's time within the kspread_sheet.cc line 6965 for-loop where it copies over just all cells. Changing those lines to;

 for ( int newRow = backupRow; newRow < endRow; ++newRow )
 {
+    kdDebug() << "1==================> "<<newRow<<endl;
     Cell* target = nonDefaultCell( columnIndex, newRow );
     if (cell != target) {
+        kdDebug() << "2==================> "<<newRow<<endl;
         target->copyAll( cell );
     }
 }

prints;
kspread: 1==================> 11796
kspread: 2==================> 11796
kspread: 1==================> 11797
kspread: 2==================> 11797
kspread: 1==================> 11798
kspread: 2==================> 11798
kspread: 1==================> 11799
kspread: 2==================> 11799
kspread: 1==================> 11800
kspread: 2==================> 11800
kspread: 1==================> 11801
kspread: 2==================> 11801
kspread: 1==================> 11802
kspread: 2==================> 11802
and so on till ~30396 and then it continues with 1175,1176,1177,...

Following backtraces are catched during that loop;

backtrace #1;
#0  0xb62ac98f in KSpread::Sheet::nonDefaultCell (this=0x8392650, _column=81, _row=19349,
    _scrollbar_update=false, _style=0x0) at kspread_sheet.cc:1109
#1  0xb62adbd4 in KSpread::Sheet::loadRowFormat (this=0x8392650, row=@0xbffa5eb4, rowIndex=@0xbffa5edc,
    oasisContext=@0xbffa61c0, styleMap=@0xbffa60c4) at kspread_sheet.cc:6972

backtrace #2;
#0  0xb686db45 in operator new[] () from /usr/lib/libstdc++.so.6
#1  0xb715008f in operator<< () from /usr/lib/libqt-mt.so.3
#2  0xb715011f in QString::fromLatin1 () from /usr/lib/libqt-mt.so.3
#3  0xb71501f3 in QString::setLatin1 () from /usr/lib/libqt-mt.so.3
#4  0xb7150c92 in QString::setAscii () from /usr/lib/libqt-mt.so.3
#5  0xb7150cca in QString::operator= () from /usr/lib/libqt-mt.so.3
#6  0xb61bd138 in KSpread::Cell::setCellText (this=0x313c6958, _text=@0xbffa5c70, asText=false)
    at kspread_cell.cc:4627
#7  0xb61c4806 in KSpread::Cell::copyContent (this=0x313c6958, cell=0x14302380) at kspread_cell.cc:647
#8  0xb61c4885 in KSpread::Cell::copyAll (this=0x313c6958, cell=0x14302380) at kspread_cell.cc:632
#9  0xb62adbf1 in KSpread::Sheet::loadRowFormat (this=0x8392650, row=@0xbffa5eb4, rowIndex=@0xbffa5edc,
    oasisContext=@0xbffa61c0, styleMap=@0xbffa60c4) at kspread_sheet.cc:6975
#10 0xb62b4594 in KSpread::Sheet::loadOasis (this=0x8392650, sheetElement=@0xbffa60a0,
    oasisContext=@0xbffa61c0, styleMap=@0xbffa60c4) at kspread_sheet.cc:6479

backtrace #3;
#0  0xb61cab1b in KSpread::Cluster::insert (this=0x82a721c, cell=0x35e4e9d0, x=110, y=23341)
    at kspread_cluster.cc:127
#1  0xb62ac85f in KSpread::Sheet::insertCell (this=0x8392650, _cell=0x35e4e9d0) at kspread_sheet.cc:8030
#2  0xb62ac9b7 in KSpread::Sheet::nonDefaultCell (this=0x8392650, _column=110, _row=23341,
    _scrollbar_update=false, _style=0x0) at kspread_sheet.cc:1111
#3  0xb62adbd4 in KSpread::Sheet::loadRowFormat (this=0x8392650, row=@0xbffa5eb4, rowIndex=@0xbffa5edc,
    oasisContext=@0xbffa61c0, styleMap=@0xbffa60c4) at kspread_sheet.cc:6972
#4  0xb62b4594 in KSpread::Sheet::loadOasis (this=0x8392650, sheetElement=@0xbffa60a0,
    oasisContext=@0xbffa61c0, styleMap=@0xbffa60c4) at kspread_sheet.cc:6479

backtrace #4;
#0  0xb66bcf75 in free () from /lib/tls/libc.so.6
#1  0xb66bee7f in malloc () from /lib/tls/libc.so.6
#2  0xb686da18 in operator new () from /usr/lib/libstdc++.so.6
#3  0xb6268ec5 in KSpread::Value::detach (this=0x3d962490) at kspread_value.cc:728
#4  0xb626901f in KSpread::Value::setFormat (this=0x3d962490, fmt=KSpread::Value::fmt_None)
    at kspread_value.cc:626
#5  0xb61afcab in KSpread::Cell::copyFormat (this=0x3d962478, cell=0x14302380) at kspread_cell.cc:583
#6  0xb61c4873 in KSpread::Cell::copyAll (this=0x3d962478, cell=0x14302380) at kspread_cell.cc:631
#7  0xb62adbf1 in KSpread::Sheet::loadRowFormat (this=0x8392650, row=@0xbffa5eb4, rowIndex=@0xbffa5edc,
    oasisContext=@0xbffa61c0, styleMap=@0xbffa60c4) at kspread_sheet.cc:6975
#8  0xb62b4594 in KSpread::Sheet::loadOasis (this=0x8392650, sheetElement=@0xbffa60a0,
    oasisContext=@0xbffa61c0, styleMap=@0xbffa60c4) at kspread_sheet.cc:6479
Comment 3 Sebastian Sauer 2007-01-21 01:48:08 UTC
*** Bug 139920 has been marked as a duplicate of this bug. ***
Comment 4 Stefan Nikolaus 2007-08-15 16:55:20 UTC
SVN commit 700440 by nikolaus:

OpenDocument	Loading.
		Speed up for loading of repeated rows. Comments, conditions, styles and validities
		are stored in RectStorages and repeated rows often have only styles set, but no
		contents. Take advantage of this fact. For cells with contents it stays slow,
		because the iteration can't be avoided.

BUG: 132454


 M  +49 -98    Sheet.cpp  


--- trunk/koffice/kspread/Sheet.cpp #700439:700440
@@ -3159,7 +3159,6 @@
 {
 //    kDebug(36003)<<"Sheet::loadRowFormat( const KoXmlElement& row, int &rowIndex,const KoOasisStyles& oasisStyles, bool isLast )***********";
 
-    int backupRow = rowIndex;
     bool isNonDefaultRow = false;
 
     KoStyleStack styleStack;
@@ -3228,118 +3227,70 @@
         isNonDefaultRow = true;
     }
 
-    //number == number of row to be copy. But we must copy cell too.
-    for ( int i = 0; i < number; ++i )
+//     kDebug(36003)<<" create non defaultrow format :"<<rowIndex<<" repeate :"<<number<<" height :"<<height;
+    if (isNonDefaultRow)
     {
-       // kDebug(36003)<<" create non defaultrow format :"<<rowIndex<<" repeate :"<<number<<" height :"<<height;
-
-      const RowFormat* rowFormat;
-      if ( isNonDefaultRow )
-      {
-        RowFormat* rf = nonDefaultRowFormat( rowIndex );
-        rowFormat = rf;
-
-        if ( height != -1.0 )
-          rf->setHeight( height );
-        if ( visibility == Collapsed )
-          rf->setHidden( true );
-        else if (visibility == Filtered)
-            rf->setFiltered(true);
-      }
-      else
-      {
-        rowFormat = this->rowFormat( rowIndex );
-      }
-      ++rowIndex;
+        for (int r = 0; r < number; ++r)
+        {
+            RowFormat* rowFormat = nonDefaultRowFormat(rowIndex + r);
+            if (height != -1.0)
+                rowFormat->setHeight(height);
+            if (visibility == Collapsed)
+                rowFormat->setHidden(true);
+            else if (visibility == Filtered)
+                rowFormat->setFiltered(true);
+        }
     }
 
-    int columnIndex = 0;
-    KoXmlNode cellNode = row.firstChild();
-    int endRow = qMin(backupRow+number,KS_rowMax);
+    int columnIndex = 1;
+    const int endRow = qMin(rowIndex + number - 1, KS_rowMax);
 
-
-    while ( !cellNode.isNull() )
+    KoXmlElement cellElement;
+    forEachElement(cellElement, row)
     {
-        KoXmlElement cellElement = cellNode.toElement();
-        if ( !cellElement.isNull() )
-        {
-            columnIndex++;
-            QString localName = cellElement.localName();
+        if (cellElement.namespaceURI() != KoXmlNS::table)
+            continue;
+        if (cellElement.localName() != "table-cell" && cellElement.localName() != "covered-table-cell")
+            continue;
 
-            if ( ((localName == "table-cell") || (localName == "covered-table-cell")) && cellElement.namespaceURI() == KoXmlNS::table)
-            {
-                //kDebug(36003) <<"Loading cell #" << cellCount;
+        Cell cell(this, columnIndex, rowIndex);
+        cell.loadOasis(cellElement, oasisContext);
 
-                const bool cellHasStyle = cellElement.hasAttributeNS( KoXmlNS::table, "style-name" );
-                const QString styleName = cellElement.attributeNS( KoXmlNS::table , "style-name" , QString() );
-                if ( cellHasStyle && !styleName.isEmpty())
-                    cellStyleRegions[styleName] += QRect( columnIndex, backupRow, 1, 1 );
+        bool ok = false;
+        const int n = cellElement.attributeNS(KoXmlNS::table, "number-columns-repeated", QString()).toInt(&ok);
+        // Some spreadsheet programs may support more columns than
+        // KSpread so limit the number of repeated columns.
+        // FIXME POSSIBLE DATA LOSS!
+        const int numberColumns = ok ? qMin(n, KS_colMax - columnIndex + 1) : 1;
 
-                Cell cell = Cell( this, columnIndex, backupRow );
-                cell.loadOasis( cellElement, oasisContext );
+        // Styles are inserted at the end of the loading process, so check the XML directly here.
+        const QString styleName = cellElement.attributeNS(KoXmlNS::table , "style-name", QString());
+        if (!styleName.isEmpty())
+            cellStyleRegions[styleName] += QRect(columnIndex, rowIndex, numberColumns, number);
 
-                int cols = 1;
+        if (!cell.comment().isEmpty())
+            cellStorage()->setComment(Region(columnIndex, rowIndex, numberColumns, number, this), cell.comment());
+        if (!cell.conditions().isEmpty())
+            cellStorage()->setConditions(Region(columnIndex, rowIndex, numberColumns, number, this), cell.conditions());
+        if (!cell.validity().isEmpty())
+            cellStorage()->setValidity(Region(columnIndex, rowIndex, numberColumns, number, this), cell.validity());
 
-                // Copy this cell across & down, if it has repeated rows or columns, but only
-                // if the cell has some content or a style associated with it.
-                if ( (number > 1) || cellElement.hasAttributeNS( KoXmlNS::table, "number-columns-repeated" ) )
+        if (cell.isFormula() || !cell.userInput().isEmpty() || !cell.value().isEmpty())
+        {
+            for (int c = 0; c < numberColumns; ++c)
+            {
+                for (int r = rowIndex; r <= endRow; ++r)
                 {
-                    bool ok = false;
-                    int n = cellElement.attributeNS( KoXmlNS::table, "number-columns-repeated", QString() ).toInt( &ok );
-
-                    if (ok)
-                        // Some spreadsheet programs may support more columns than
-                        // KSpread so limit the number of repeated columns.
-                        // FIXME POSSIBLE DATA LOSS!
-                        cols = qMin( n, KS_colMax - columnIndex + 1 );
-
-                    if ( !cellHasStyle && ( cell.isEmpty() && Cell( this, columnIndex, backupRow ).comment().isEmpty() ) )
-                    {
-                        // just increment it
-                        columnIndex += cols - 1;
-                    }
-                    else
-                    {
-                        for ( int k = cols ; k ; --k )
-                        {
-                            if ( k != cols )
-                                columnIndex++;
-
-                            for ( int newRow = backupRow; newRow < endRow; ++newRow )
-                            {
-                                if ( !cell.isEmpty() )
-                                {
-                                    Cell target = Cell( this, columnIndex, newRow );
-                                    if ( !cell.compareData( target ) )
-                                        target.copyContent( cell );
-                                }
-                                // TODO Stefan: set the attributes in one go for the repeated range
-                                if ( cellHasStyle && !styleName.isEmpty())
-                                    cellStyleRegions[styleName] += QRect( columnIndex, newRow, 1, 1 );
-                                const QString comment = Cell( this, columnIndex, backupRow ).comment();
-                                if ( !comment.isEmpty() )
-                                    cellStorage()->setComment( Region(QPoint(columnIndex, newRow)), comment );
-                                const Conditions conditions = cellStorage()->conditions( columnIndex, backupRow );
-                                if ( !conditions.isEmpty() )
-                                    cellStorage()->setConditions( Region(QPoint(columnIndex, newRow)), conditions );
-                                const KSpread::Validity validity = cellStorage()->validity( columnIndex, backupRow );
-                                if ( !validity.isEmpty() )
-                                    cellStorage()->setValidity( Region(QPoint(columnIndex, newRow)), validity );
-                            }
-                        }
-                    }
+                    Cell target(this, columnIndex + c, r);
+                    target.setFormula(cell.formula());
+                    target.setUserInput(cell.userInput());
+                    target.setValue(cell.value());
                 }
-
-                // delete non-default cell, if it is empty
-                if ( cell.isEmpty() )
-                {
-                    d->cellStorage->take( cell.column(), cell.row() );
-                }
             }
         }
-        cellNode = cellNode.nextSibling();
+        columnIndex += numberColumns;
     }
-
+    rowIndex += number;
     return true;
 }