Issue 118986 - [RTF export] crashes when copy an empty line from Writer below a tif picture then paste into spreadsheet
[RTF export] crashes when copy an empty line from Writer below a tif picture ...
Status: VERIFIED FIXED
Product: Writer
Classification: Application
Component: save-export
3.4.0
PC Windows, all
: P3 normal (vote)
: 4.0.0
Assigned To: Armin Le Grand
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-28 07:57 UTC by louqle
Modified: 2013-07-16 11:52 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---
jsc: 4.0.0_release_blocker+


Attachments
tif image (699.18 KB, application/zip)
2012-02-28 08:48 UTC, louqle
no flags Details
Patch to reduce mem usage to something as before the RTF changes for AOO3.4 (60.24 KB, patch)
2013-07-04 14:04 UTC, Armin Le Grand
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description louqle 2012-02-28 07:57:18 UTC
build: r1291124 

1. new a Writer then insert the attached .tif image
2. locate cursor below the image, press Enter to insert an empty line
3. enter some text
4. press CTRL+A to select the empty line and the text
5. new a spreadsheet then press CTRL+V to paste

oo3.4 crashes..

while oo3.3 works fine.

I also tried on Ubuntu 10.04, Aoo3.4 takes long time to respond but finaly it succeeds.
Comment 1 louqle 2012-02-28 08:48:24 UTC
Created attachment 77257 [details]
tif image
Comment 2 Oliver-Rainer Wittmann 2012-02-28 16:00:51 UTC
I have reproduced the described crash under Windows 7 with OOo 3.4 Beta and latest AOO 3.4 developer snapshot, rev. 1293550
Comment 3 Oliver-Rainer Wittmann 2012-02-29 08:23:02 UTC
The problem is caused by the RTF export - RTF is used as the clipboard exchange format - when exporting the TIFF image.
Thus, the crash can also reproduced when saving the resulting text document to RTF.

In order to export the TIFF image to RTF certain amount of memory is needed which is due to the used encoding four times larger than the TIFF file itself. Depending on memory which is available in the system the memory gets allocated or not. If we are running out of memory AOO will crash.

It is a regression introduced in OOo 3.4 Beta by CWS vmiklos01, bug 113532

@vmiklos: Can you help here to fix the issue? If yes, please contact me - I have made a deeper investigation which I want to share with you.
Comment 4 Miklos Vajna 2012-03-05 11:59:53 UTC
[ Please don't take this as an LO-rant, I'm commenting this here as Oliver asked for it. ]

On Fri, Mar 02, 2012 at 10:05:07AM +0100, Oliver-Rainer Wittmann <orwittmann@googlemail.com> wrote:
> Is it possible to postpone the creation of the image data to the       
> point when they are needed instead of keeping them in memory?

That's exacly what I implemented. The idea is:

- create a wrapper around OStringBuffer, that can remember the arguments
  to a RtfAttributeOutput::FlyFrameGraphic() call
- instead of calling that method, just append the arguments to this
  RtfStringBuffer
- call FlyFrameGraphic() when we can write to the stream directly

FYI, here are the patches:

- refactoring for RtfStringBuffer:

http://cgit.freedesktop.org/libreoffice/core/commit/?id=85887d554141c33d4a9803ab2fd7765dadf7e64d
http://cgit.freedesktop.org/libreoffice/core/commit/?id=4ed3acc18e70a6c867e827c19f8ccf7fa925b9c5
http://cgit.freedesktop.org/libreoffice/core/commit/?id=9b6e7eb1bad4344be0e766f4c05f0a3f28cb737d

- the real fix:

http://cgit.freedesktop.org/libreoffice/core/commit/?id=622ce4681dbcddfdd5cbcd95ebbf1f14e93489ed

valgrind's massif says the peak memory usage is now 1.048GB -> 330.1MB,
which should be on par with the old export.
Comment 5 Armin Le Grand 2013-07-03 11:44:40 UTC
ALG: Pic is 4492x3176 pixels, makes a whole lot of mem. 14,2mb and orw says 4 times, whew.

Mac crashed after a while.
Win crashed after a while.
Linux took a lot of time, but went through it, empty and 2nd lice pasted, picture missing, no crash.

Seems to work in principle, but needing too much memory in preparing the transfer data, as orw wrote.
Comment 6 Armin Le Grand 2013-07-03 11:47:36 UTC
ALG: Copy/pasting simply the picture works well, another clipboard format is used. So there is a simple workaround: Copy/Paste text and picture separately. Pictures of that size are also not the default case.
Comment 7 Armin Le Grand 2013-07-04 14:02:01 UTC
ALG: Took a deeper look and best way will be to create the hex data from binary data at streaming time, holding sources for it in a stack for rtl::OStringBuffer. Tried a pretty straightforward implementation with pure virtual entries for string and binary, allows moving creation of string from binary to stream time for graphics, OLEs and PresentationData. Checked using a OOo3.3 that it produces the same binary output; also using a hextable for conversion, no longer creating each single value in a OString separately. Preparing patch to add.
Comment 8 Armin Le Grand 2013-07-04 14:04:38 UTC
Created attachment 80997 [details]
Patch to reduce mem usage to something as before the RTF changes for AOO3.4
Comment 9 Armin Le Grand 2013-07-04 15:16:44 UTC
ALG: Checked with various graphis, save as rtf, reload, compare with unchanged AOO4, looks good. This proves that this change does not change the output, thus goal seems reached and correctness seems good. Doing some more checks...
Comment 10 Armin Le Grand 2013-07-04 15:22:59 UTC
ALG: Checked loading with MS, looks the same (works and works not in the same cases). Also found a crash: New Writer, insert chart, save as RTF -> crash. Easily fixed in WW8SwAttrIter::HasTextItem, tough.
Comment 11 jsc 2013-07-05 08:54:29 UTC
grant showstopper flag, fix is in place
Comment 12 Armin Le Grand 2013-07-05 11:05:09 UTC
ALG: Checked again with the original description, works now on 32-bit systems (seems it worked with 64bit anyways due to more mem available). The reduced mem usage works, though. Also checked OOO3.3 and it behaves exactly the same. Still strange is the fact that the pictre is not inserted to calc, but this is also the same in both versions and another bug probably; this also happens with smaller non-tiff graphics, too.
Also checked save as RTF and reload, also the same as OOO3.3. The reload is *very* slow, this also needs to be profiled one day.
Preparing commit.
Comment 13 Armin Le Grand 2013-07-05 11:10:22 UTC
ALG: Wrote #122693# for the missing pic in calc after paste
Comment 14 Armin Le Grand 2013-07-05 11:15:02 UTC
ALG: Wrote #122694# as Follow-up for the slow import and errors with tiff images
Comment 15 SVN Robot 2013-07-05 11:15:39 UTC
"alg" committed SVN revision 1499965 into trunk:
i118986 reduced mem usage of RTF export
Comment 16 Armin Le Grand 2013-07-05 11:15:59 UTC
ALG: Comitted, done.
Comment 17 fanyuzhen 2013-07-09 05:54:22 UTC
The latest build I get is AOO 4.0 r1499347, wait for revision 1499965 or higher for verification
Comment 18 fanyuzhen 2013-07-16 11:52:01 UTC
I checked with RC Rev. 1502185 on Windows, do not see the crash, but with the fact that it is *very* slow to get the content pasted to Calc