Bug 86017 - Crash when pasting spreadsheet columns in DDE mode on writer
Summary: Crash when pasting spreadsheet columns in DDE mode on writer
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.2.0.0.alpha0+ Master
Hardware: All All
: high major
Assignee: Caolán McNamara
URL:
Whiteboard: target:5.1.0 target:5.0.0.1 target:4.4.5
Keywords: bibisected, bisected, haveBacktrace, regression
Depends on:
Blocks:
 
Reported: 2014-11-07 20:09 UTC by Arnaud Versini
Modified: 2016-10-25 19:20 UTC (History)
11 users (show)

See Also:
Crash report or crash signature:


Attachments
Spreadsheet to crash LibreOffice during paste DDE in Writer (27.92 KB, application/vnd.oasis.opendocument.spreadsheet)
2014-11-08 12:38 UTC, Arnaud Versini
Details
Screenshot after paste as DDE in writer (314.68 KB, image/png)
2014-11-08 13:48 UTC, m_a_riosv
Details
console bt (7.59 KB, text/plain)
2014-11-08 17:12 UTC, Julien Nabet
Details
Bibisect result of crash (2.16 KB, text/plain)
2014-11-09 14:39 UTC, Arnaud Versini
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arnaud Versini 2014-11-07 20:09:15 UTC
Hi,

Steps to reproduce :
- Create a spreadsheet with minimum 3 columns with data and save it
- Create a Writer file
- Copy columns 1 and 3 from Calc
- To Writer

Expected behavior, not a crash
Current behavior : crash
Comment 1 Arnaud Versini 2014-11-07 20:10:00 UTC
Reproduced on Windows 7 32 and on Linux 64.
Comment 2 Joel Madero 2014-11-07 20:59:51 UTC
For "copy columns 1 and 3 from Calc" if I understood correctly I just put some data in A1:A5, and C1:C5, then I selected these cells (omitting B column) by holding ctrl, copied and pasted into writer. No crash.
Comment 3 Joel Madero 2014-11-07 21:00:06 UTC
Should have included:
4.3.3.2 x64
Ubuntu 14.04
Comment 4 Arnaud Versini 2014-11-07 21:47:34 UTC
@jmadero DDE paste need to use special paste, did you ?
Comment 5 Arnaud Versini 2014-11-07 21:48:26 UTC
Sorry, so steps to reproduce :

Steps to reproduce :
- Create a spreadsheet with minimum 3 columns with data and save it
- Create a Writer file
- Copy columns 1 and 3 from Calc
- To Writer as DDE by using special paste

Expected behavior, not a crash
Current behavior : crash
Comment 6 m_a_riosv 2014-11-08 11:29:34 UTC
Hi @Arnaud, I can't reproduce,
Win7x64Ultimate
Version: 4.3.3.2
Build ID: 9bb7eadab57b6755b1265afa86e04bf45fbfc644

To be certain the reported issue is not related to corruption in the user profile, could you rename this LibreOffice user directory ( see https://wiki.documentfoundation.org/UserProfile#Default_location or http://ask.libreoffice.org/en/question/903/where-are-the-libreoffice-data-profile-files/ ) and re-test?
Comment 7 Arnaud Versini 2014-11-08 11:35:11 UTC
Done, crash again, I will test with master
Comment 8 m_a_riosv 2014-11-08 11:37:51 UTC
Please can you attach a spreadsheet file from which reproduce the issue.
Comment 9 Arnaud Versini 2014-11-08 12:38:52 UTC
Created attachment 109131 [details]
Spreadsheet to crash LibreOffice during paste DDE in Writer
Comment 10 m_a_riosv 2014-11-08 13:48:26 UTC
Created attachment 109132 [details]
Screenshot after paste as DDE in writer

Sorry Arnadu, but I can't reproduce the issue, please see attached image with data pasted in writer.

Maybe something specific about how LibreOffice is installed in your computer.
Comment 11 m_a_riosv 2014-11-08 13:49:47 UTC
Changed to unconfirmed.
Comment 12 Buovjaga 2014-11-08 14:00:13 UTC
No crash here.

Win 7 64-bit Version: 4.4.0.0.alpha2+
Build ID: c989f5e0e11e295b11ffc921b0d105869e037e47
TinderBox: Win-x86@42, Branch:master, Time: 2014-11-07_22:50:48
Comment 13 Arnaud Versini 2014-11-08 14:10:31 UTC
You pasted all columns, not only 1 and 3, this works for me also
Comment 14 Julien Nabet 2014-11-08 17:12:15 UTC
Created attachment 109133 [details]
console bt

On pc Debian x86-64 with master sources updated yesterday, I could reproduce this.

I attached console logs + bt (with symbols)
Comment 15 Julien Nabet 2014-11-08 17:12:53 UTC
Increase a bit the importance since it's a crash.
Comment 16 Arnaud Versini 2014-11-08 19:20:56 UTC
Hi,

Seems my information for this are not correct, with those instructions it crash always :

Steps to reproduce :
- Create a Writer file (before Calc document opening or creation)
- Create a spreadsheet with minimum 3 columns with data and save it
- Copy columns 1 and 3 from Calc
- To Writer

Expected behavior, not a crash
Current behavior : crash
Comment 17 Julien Nabet 2014-11-08 19:35:48 UTC
Arnaud: with a simple copy paste, it doesn't crash (at least with master sources) but copy nothing and I got this on console:
warn:legacy.osl:4168:1:sc/source/core/data/fillinfo.cxx:297: FillInfo: Range too big
Comment 18 m_a_riosv 2014-11-08 19:55:46 UTC
Reproducible, copying columns 1 + 3, copying only the ranges with data in columns 1 + 3 paste in writer three empty columns.

Win7x64Ultimate
Version: 4.3.2.2
Build ID: edfb5295ba211bd31ad47d0bad0118690f76407d

Copying columns 1+2 works fine, maybe it's in relation with multi range copy.
Comment 19 Arnaud Versini 2014-11-08 21:28:28 UTC
Julien, I think it's related to this message, I'm investigating about that now :

warn:legacy.osl:18599:1:sw/source/core/docnode/ndtbl.cxx:338: Table without line?
warn:legacy.osl:18599:1:sw/source/core/layout/tabfrm.cxx:89: SwTabFrm::SwTabFrm: No rows.
warn:legacy.osl:18599:1:sw/source/core/layout/tabfrm.cxx:167: No rows.
Comment 20 Julien Nabet 2014-11-08 21:34:59 UTC
Arnaud: indeed, adding some debug code to have the content of sTmp here http://opengrok.libreoffice.org/xref/core/sw/source/uibase/dochdl/swdtflvr.cxx#2163 shows a problem to retrieve the right data.
Comment 21 Arnaud Versini 2014-11-08 21:46:15 UTC
This test should avoid the issue, can't figure why it doesn't.

                // at least one column & row must be there
                if( !nRows || !nCols )


Are you on IRC ?
Comment 22 Julien Nabet 2014-11-08 22:57:57 UTC
On pc Debian x86-64 with master sources updated yesterday, if I just copy the 84 cells of the 2 columns (42 + 42) and special paste DD3, LO copies all the 106 cells! (as if I had selected all non empty cells of the 3 columns)
Comment 23 Urmas 2014-11-09 05:49:40 UTC
That is not surprising, as the source is 'unnamed1' instead of filebane, and the range is given simply as a rectangular block.
Comment 24 Laurent Balland 2014-11-09 12:31:05 UTC
Procedure used:
- Launch LibO with new profile
- Create new Writer document
- Open attachment 109131 [details]
- Click on header of column A, then Ctrl+Click on header of column C
- Copy with Ctrl+C
- Go back to Writer, click on menu of Paste button > DDE link

Crash reproduced with:
- Version: 4.2.0.0.alpha1+
Build ID: d366c9b20ec86f3fe521812a0c22def3bfd1f05e
TinderBox: Win-x86@47-TDF, Branch:master, Time: 2013-11-14_07:51:04

NO crash with:
- Version: 4.2.0.0.alpha0+
Build ID: cc2a405915e82c4b332dd25457f76704dc536d7f
TinderBox: Win-x86@39, Branch:master, Time: 2013-10-15_15:51:52

This crash may hide other bugs:
Instead, with this version of 2013-10-15, you've got a warning message which may be considered as a bug because cursor is not in a table:
"A table cannot be inserted into another table. However, you can paste the data into the document when the cursor is not in a table."
This message also appeared in Version: 4.2.0.0.alpha0+
Build ID: 4a8f7ddc290d1ea3131de6611b6833b77ac7ab1f (2013-08-28)
With prior versions to Version: 4.2.0.0.alpha0+
Build ID: 2f6cbe13e61c44d4bab8192a4708b698d3d9da33
TinderBox: Win-x86@6-debug, Branch:master, Time: 2013-07-25_00:00:21
the observed behavior is still different:
An empty field is inserted because it refers to source file "unnamed1".
This behavior was the same since LibO330.4 and was inherited from OOo.
Expected behavior:
Data should be inserted
Comment 25 Julien Nabet 2014-11-09 13:03:37 UTC
Kohei/Markus/Eike: Following last comments, I think you might be interested in this one. Indeed, it seems the more we dig here, the more we find problems/strange things at least.
However, perhaps all of these have the same root cause. If this is the case, it's not obvious for the moment.
Comment 26 Arnaud Versini 2014-11-09 14:39:12 UTC
I have this bibisect log for the crash, and I'm bisecting now.
Comment 27 Arnaud Versini 2014-11-09 14:39:46 UTC
Created attachment 109150 [details]
Bibisect result of crash
Comment 28 Matthew Francis 2015-04-13 08:03:50 UTC
This started at the below commit.
Adding Cc: to noelgrandin@gmail.com; Could you possibly take a look at this one? Thanks

(Before the commit, the DDE paste fails with a dialog error message - not seemingly a very pertinent one, but at least it fails nicely: "A table cannot be inserted into another table. However, you can paste the data into the document when the cursor is not in a table.")

commit 3d65dfa3c98e0f79c579cfac23d55092f7554244
Author: Noel Grandin <noel@peralex.com>
Date:   Mon Oct 28 13:20:52 2013 +0200

    convert xub_StrLen to sal_Int32
    
    Converts code that calls comphelper::string::getTokenCount() to
    use sal_Int32 to store the return value.
    
    Change-Id: I439605a39d29b1309649e30f3ff40dfa412efcde
Comment 29 Julien Nabet 2015-04-13 15:11:06 UTC
Thank you Matthew for your investigation.

I noticed this part in the commit:
--- a/sw/source/ui/dochdl/swdtflvr.cxx
+++ b/sw/source/ui/dochdl/swdtflvr.cxx
@@ -2121,11 +2121,11 @@ int SwTransferable::_PasteDDE( TransferableDataHelper& rData,
comphelper::string::getTokenCount(aExpand, '\t') ) )
{
OUString sTmp( aExpand );
- xub_StrLen nRows = comphelper::string::getTokenCount(sTmp, '\n');
+ sal_Int32 nRows = comphelper::string::getTokenCount(sTmp, '\n');
if( nRows )
--nRows;
sTmp = sTmp.getToken( 0, '\n' );
- xub_StrLen nCols = comphelper::string::getTokenCount(sTmp, '\t');
+ sal_Int32 nCols = comphelper::string::getTokenCount(sTmp, '\t');
// at least one column & row must be there
if( !nRows || !nCols )

but SwEditShell::InsertDDETable (see http://opengrok.libreoffice.org/xref/core/sw/source/uibase/dochdl/swdtflvr.cxx#2182) expects sal_uInt16 for nRows and nCols (see http://opengrok.libreoffice.org/xref/core/sw/source/core/edit/edtab.cxx#InsertDDETable)
perhaps just a replace in the args of InsertDDETable could make it, I'll give it a try.
Comment 30 Julien Nabet 2015-04-13 21:44:33 UTC
It's not sufficient to change SwEditShell::InsertDDETable, see:
Breakpoint 1, SwDoc::InsertTable (this=0x6e8c990, rInsTblOpts=..., rPos=SwPosition (node 9, offset 0), nRows=0, nCols=3, eAdjust=6, pTAFmt=0x0, pColArr=0x0, 
    bCalledFromShell=false, bNewModel=true) at /home/julien/compile-libreoffice/libreoffice/sw/source/core/docnode/ndtbl.cxx:338
338	    OSL_ENSURE( nRows, "Table without line?" );
(gdb) bt
#0  SwDoc::InsertTable (this=0x6e8c990, rInsTblOpts=..., rPos=SwPosition (node 9, offset 0), nRows=0, nCols=3, eAdjust=6, pTAFmt=0x0, pColArr=0x0, bCalledFromShell=false, 
    bNewModel=true) at /home/julien/compile-libreoffice/libreoffice/sw/source/core/docnode/ndtbl.cxx:338
#1  0x00002aaadaa9e64b in SwEditShell::InsertDDETable (this=0x6eedf80, rInsTblOpts=..., pDDEType=0x7252510, nRows=1048576, nCols=3, eAdj=6)
    at /home/julien/compile-libreoffice/libreoffice/sw/source/core/edit/edtab.cxx:234

So InsertTable seems also to need sal_Int32 for rows and cols (see http://opengrok.libreoffice.org/xref/core/sw/source/core/docnode/ndtbl.cxx#330)
Comment 31 Julien Nabet 2015-04-14 05:47:01 UTC
Here's a temporary fix:
diff --git a/sw/source/uibase/dochdl/swdtflvr.cxx b/sw/source/uibase/dochdl/swdtflvr.cxx
index 2ba22e4..e8ea063 100644
--- a/sw/source/uibase/dochdl/swdtflvr.cxx
+++ b/sw/source/uibase/dochdl/swdtflvr.cxx
@@ -2171,7 +2171,7 @@ bool SwTransferable::_PasteDDE( TransferableDataHelper& rData,
                 sal_Int32 nCols = comphelper::string::getTokenCount(sTmp, '\t');
 
                 // at least one column & row must be there
-                if( !nRows || !nCols )
+                if( !nRows || !nCols || nRows > SAL_MAX_UINT16)
                 {
                     if( bMsg )
                         MessageDialog(0, SW_RESSTR(STR_NO_TABLE), VCL_MESSAGE_INFO).Execute();

The real fix would be to calculate the number of non empty rows (as when cols selected are consecutive) or to change all the functions involved so they use sal_Int32 instead of sal_uInt16 for nRows (and also perhaps for nCols)
Comment 32 Commit Notification 2015-06-17 09:45:43 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=550cff762d816c336adaf015f481443af1c6edab

Resolves: tdf#86017 calc has more rows than writer tables can support

It will be available in 5.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 33 Commit Notification 2015-06-17 09:51:27 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=799eef80581f2b39025ab1f388d6f4d741eabeac&h=libreoffice-5-0

Resolves: tdf#86017 calc has more rows than writer tables can support

It will be available in 5.0.0.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 34 Caolán McNamara 2015-06-17 10:00:55 UTC
Yeah, lets fix the crash first anyway.

Writer tables presumably need to be expanded to have the same number of rows as calc to make this work in theory.

caolanm->erack:
I wonder why in sc/source/ui/docshell/impex.cxx Doc2Text why we only call ShrinkToDataArea for non-multi-range selections. Could we always call that if the starttab and endtab are the same ?
Comment 35 Eike Rathke 2015-06-17 11:58:37 UTC
@Caolan:
Apparently no specific reason, the existing shrink is a fix for some other case (Ctrl+A or some such), previously we assumed the user knew what s/he does.. the export code doesn't handle multi-selection at all, the overall range is exported.

So, for multi-selections it would be expected to shrink all selections individually and then use the surrounding bounds of all resulting selections, and while writing data omit completely empty columns and rows in that area if they were only leading/trailing the actual data of each individual selection? And also omit empty columns and rows if they were not selected?

Question remains, what if the user actually wanted to include empty rows or columns in the selection, maybe we should shrink the selections only if entire columns or rows are selected.

However, it is probably expected that not selected columns or rows are not included if the selections do not overlap in any dimension.
Comment 36 Commit Notification 2015-06-18 11:40:00 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-4-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ae4d51b8d835b47e70bc6a0d2f92c2f4057bce16&h=libreoffice-4-4

Resolves: tdf#86017 calc has more rows than writer tables can support

It will be available in 4.4.5.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 37 Robinson Tryon (qubit) 2015-12-17 08:39:01 UTC Comment hidden (obsolete)